From 0a5c7c6c22707f990867e4799ee3d2cc0d50c28b Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Tue, 5 Oct 2021 13:29:34 -0700 Subject: [PATCH 1/6] Only report MSB4181 when not on warnandcontinue. Always set hasloggederrors when logging an error --- .../BackEnd/Components/RequestBuilder/TaskBuilder.cs | 11 +---------- .../BackEnd/Components/RequestBuilder/TaskHost.cs | 2 +- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs index 5fdd1a3e145..b0e56370379 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs @@ -940,16 +940,7 @@ private async Task ExecuteInstantiatedTask(ITaskExecutionHost ta IBuildEngine be = host.TaskInstance.BuildEngine; if (taskReturned && !taskResult && !taskLoggingContext.HasLoggedErrors && (be is TaskHost th ? th.BuildRequestsSucceeded : false) && (be is IBuildEngine7 be7 ? !be7.AllowFailureWithoutError : true)) { - if (_continueOnError == ContinueOnError.WarnAndContinue) - { - taskLoggingContext.LogWarning(null, - new BuildEventFileInfo(_targetChildInstance.Location), - "TaskReturnedFalseButDidNotLogError", - _taskNode.Name); - - taskLoggingContext.LogComment(MessageImportance.Normal, "ErrorConvertedIntoWarning"); - } - else + if (_continueOnError != ContinueOnError.WarnAndContinue) { taskLoggingContext.LogError(new BuildEventFileInfo(_targetChildInstance.Location), "TaskReturnedFalseButDidNotLogError", diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs index 154ace42f96..90abe787f10 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs @@ -455,8 +455,8 @@ public void LogErrorEvent(Microsoft.Build.Framework.BuildErrorEventArgs e) { e.BuildEventContext = _taskLoggingContext.BuildEventContext; _taskLoggingContext.LoggingService.LogBuildEvent(e); - _taskLoggingContext.HasLoggedErrors = true; } + _taskLoggingContext.HasLoggedErrors = true; } } From 5eac868524d8ed326aa7977f600b8cf2301c220b Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Wed, 6 Oct 2021 10:49:41 -0700 Subject: [PATCH 2/6] Modify tests for new behavior --- .../WarningsAsMessagesAndErrors_Tests.cs | 25 ++----------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index d83d61361fa..553d18e77ed 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -417,28 +417,6 @@ public void TaskReturnsFailureButDoesNotLogError_ShouldCauseBuildFailure() } } - [Fact] - public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_WarnAndContinue() - { - using (TestEnvironment env = TestEnvironment.Create(_output)) - { - TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" - - - - - - "); - - MockLogger logger = proj.BuildProjectExpectSuccess(); - - logger.WarningCount.ShouldBe(1); - - logger.AssertLogContains("MSB4181"); - } - } - [Fact] public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True() { @@ -455,7 +433,8 @@ public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True() MockLogger logger = proj.BuildProjectExpectSuccess(); - logger.AssertLogContains("MSB4181"); + // When ContinueOnError is true, we no longer log MSB4141 (your task returned false but didn't log an error) + logger.AssertLogDoesntContain("MSB4181"); } } From 8232e48409fd2207af57eab2ff9cb3ea044c3bc6 Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Thu, 7 Oct 2021 16:13:49 -0700 Subject: [PATCH 3/6] Update src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs Co-authored-by: Forgind --- src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index 553d18e77ed..295b89b2c6d 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -433,7 +433,7 @@ public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True() MockLogger logger = proj.BuildProjectExpectSuccess(); - // When ContinueOnError is true, we no longer log MSB4141 (your task returned false but didn't log an error) + // When ContinueOnError is true, we no longer log MSB4181 (your task returned false but didn't log an error) logger.AssertLogDoesntContain("MSB4181"); } } From fabbb79c1a6f61c9ebb4c68ec9c393f2c1e58184 Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Mon, 11 Oct 2021 09:25:25 -0700 Subject: [PATCH 4/6] Continue logging MSB4181 when warnandcontinue is set --- .../WarningsAsMessagesAndErrors_Tests.cs | 2 +- .../BackEnd/Components/RequestBuilder/TaskBuilder.cs | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index 553d18e77ed..857c35fc06b 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -434,7 +434,7 @@ public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True() MockLogger logger = proj.BuildProjectExpectSuccess(); // When ContinueOnError is true, we no longer log MSB4141 (your task returned false but didn't log an error) - logger.AssertLogDoesntContain("MSB4181"); + logger.AssertLogContains("MSB4181"); } } diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs index b0e56370379..9676a855e7e 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs @@ -940,7 +940,17 @@ private async Task ExecuteInstantiatedTask(ITaskExecutionHost ta IBuildEngine be = host.TaskInstance.BuildEngine; if (taskReturned && !taskResult && !taskLoggingContext.HasLoggedErrors && (be is TaskHost th ? th.BuildRequestsSucceeded : false) && (be is IBuildEngine7 be7 ? !be7.AllowFailureWithoutError : true)) { - if (_continueOnError != ContinueOnError.WarnAndContinue) + + if (_continueOnError == ContinueOnError.WarnAndContinue) + { + taskLoggingContext.LogWarning(null, + new BuildEventFileInfo(_targetChildInstance.Location), + "TaskReturnedFalseButDidNotLogError", + _taskNode.Name); + + taskLoggingContext.LogComment(MessageImportance.Normal, "ErrorConvertedIntoWarning"); + } + else { taskLoggingContext.LogError(new BuildEventFileInfo(_targetChildInstance.Location), "TaskReturnedFalseButDidNotLogError", From a05519164a319daac86f28875d55f2b1ff68449b Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Mon, 18 Oct 2021 10:08:30 -0700 Subject: [PATCH 5/6] Add regression tests --- .../WarningsAsMessagesAndErrors_Tests.cs | 50 +++++++++++++++++++ .../Components/RequestBuilder/TaskHost.cs | 3 +- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs index ea8c27096cd..e8ead4b2e98 100644 --- a/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs +++ b/src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs @@ -417,6 +417,56 @@ public void TaskReturnsFailureButDoesNotLogError_ShouldCauseBuildFailure() } } + /// + /// Test that a task that returns false without logging anything reports MSB4181 as a warning. + /// + [Fact] + public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_WarnAndContinue() + { + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" + + + + + + "); + + MockLogger logger = proj.BuildProjectExpectSuccess(); + + logger.WarningCount.ShouldBe(1); + + logger.AssertLogContains("MSB4181"); + } + } + + /// + /// Test that a task that returns false after logging an error->warning does NOT also log MSB4181 + /// + [Fact] + public void TaskReturnsFailureAndLogsError_ContinueOnError_WarnAndContinue() + { + using (TestEnvironment env = TestEnvironment.Create(_output)) + { + TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@" + + + + + + + "); + + MockLogger logger = proj.BuildProjectExpectSuccess(); + + // The only warning should be the error->warning logged by the task. + logger.WarningCount.ShouldBe(1); + logger.AssertLogContains("MSB1234"); + } + } + [Fact] public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True() { diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs index 90abe787f10..0c07dae6d6f 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskHost.cs @@ -456,7 +456,8 @@ public void LogErrorEvent(Microsoft.Build.Framework.BuildErrorEventArgs e) e.BuildEventContext = _taskLoggingContext.BuildEventContext; _taskLoggingContext.LoggingService.LogBuildEvent(e); } - _taskLoggingContext.HasLoggedErrors = true; + + _taskLoggingContext.HasLoggedErrors = true; } } From c903c9c0f6ba4bd6faf872ce8670ad517c56e43b Mon Sep 17 00:00:00 2001 From: Ben Villalobos <4691428+BenVillalobos@users.noreply.github.com> Date: Mon, 25 Oct 2021 08:37:50 -0700 Subject: [PATCH 6/6] Satisfy the extra line haters --- src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs index 9676a855e7e..5fdd1a3e145 100644 --- a/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs +++ b/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs @@ -940,7 +940,6 @@ private async Task ExecuteInstantiatedTask(ITaskExecutionHost ta IBuildEngine be = host.TaskInstance.BuildEngine; if (taskReturned && !taskResult && !taskLoggingContext.HasLoggedErrors && (be is TaskHost th ? th.BuildRequestsSucceeded : false) && (be is IBuildEngine7 be7 ? !be7.AllowFailureWithoutError : true)) { - if (_continueOnError == ContinueOnError.WarnAndContinue) { taskLoggingContext.LogWarning(null,