Skip to content

Revert "Log an error when a Task returns false but didn't log an error" (#4940) #5246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Forgind merged 1 commit into from
Apr 8, 2020
Merged

Revert "Log an error when a Task returns false but didn't log an error" (#4940) #5246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Build.UnitTests/BackEnd/BuildResult_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
using Microsoft.Build.Framework;
using Microsoft.Build.Shared;
using Microsoft.Build.Unittest;
using Shouldly;
using Xunit;

using TaskItem = Microsoft.Build.Execution.ProjectItemInstance.TaskItem;

namespace Microsoft.Build.UnitTests.BackEnd
Expand Down
49 changes: 12 additions & 37 deletions src/Build.UnitTests/BackEnd/LoggingContext_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@
using Microsoft.Build.BackEnd;
using Microsoft.Build.BackEnd.Logging;
using Microsoft.Build.Shared;
using Shouldly;
using System;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.Build.UnitTests.BackEnd
{
Expand All @@ -16,36 +14,29 @@ namespace Microsoft.Build.UnitTests.BackEnd
/// </summary>
public class LoggingContext_Tests
{
private readonly ITestOutputHelper _output;

public LoggingContext_Tests(ITestOutputHelper outputHelper)
{
_output = outputHelper;
}

/// <summary>
/// A few simple tests for NodeLoggingContexts.
/// </summary>
[Fact]
public void CreateValidNodeLoggingContexts()
{
NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(_output.WriteLine), 1, true);
context.IsInProcNode.ShouldBeTrue();
context.IsValid.ShouldBeTrue();
NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(), 1, true);
Assert.True(context.IsInProcNode);
Assert.True(context.IsValid);

context.LogBuildFinished(true);
context.IsValid.ShouldBeFalse();
Assert.False(context.IsValid);

context.BuildEventContext.NodeId.ShouldBe(1);
Assert.Equal(1, context.BuildEventContext.NodeId);

NodeLoggingContext context2 = new NodeLoggingContext(new MockLoggingService(_output.WriteLine), 2, false);
context2.IsInProcNode.ShouldBeFalse();
context2.IsValid.ShouldBeTrue();
NodeLoggingContext context2 = new NodeLoggingContext(new MockLoggingService(), 2, false);
Assert.False(context2.IsInProcNode);
Assert.True(context2.IsValid);

context2.LogBuildFinished(true);
context2.IsValid.ShouldBeFalse();
Assert.False(context2.IsValid);

context2.BuildEventContext.NodeId.ShouldBe(2);
Assert.Equal(2, context2.BuildEventContext.NodeId);
}

/// <summary>
Expand All @@ -58,25 +49,9 @@ public void InvalidNodeIdOnNodeLoggingContext()
{
Assert.Throws<InternalErrorException>(() =>
{
_ = new NodeLoggingContext(new MockLoggingService(), -2, true);
NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(), -2, true);
}
);
}

[Fact]
public void HasLoggedErrors()
{
NodeLoggingContext context = new NodeLoggingContext(new MockLoggingService(_output.WriteLine), 1, true);
context.HasLoggedErrors.ShouldBeFalse();

context.LogCommentFromText(Framework.MessageImportance.High, "Test message");
context.HasLoggedErrors.ShouldBeFalse();

context.LogWarningFromText(null, null, null, null, "Test warning");
context.HasLoggedErrors.ShouldBeFalse();

context.LogErrorFromText(null, null, null, null, "Test error");
context.HasLoggedErrors.ShouldBeTrue();
}
}
}
}
26 changes: 0 additions & 26 deletions src/Build.UnitTests/BackEnd/ReturnFailureWithoutLoggingErrorTask.cs
View file
Open in desktop

This file was deleted.

119 changes: 2 additions & 117 deletions src/Build.UnitTests/WarningsAsMessagesAndErrors_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
using System.Linq;
using Microsoft.Build.Framework;
using Microsoft.Build.UnitTests;
using Shouldly;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.Build.Engine.UnitTests
{
Expand All @@ -15,13 +13,6 @@ public sealed class WarningsAsMessagesAndErrorsTests
private const string ExpectedEventMessage = "03767942CDB147B98D0ECDBDE1436DA3";
private const string ExpectedEventCode = "0BF68998";

ITestOutputHelper _output;

public WarningsAsMessagesAndErrorsTests(ITestOutputHelper output)
{
_output = output;
}

[Fact]
public void TreatAllWarningsAsErrors()
{
Expand All @@ -38,7 +29,7 @@ public void TreatAllWarningsAsErrors()
[Fact]
public void TreatWarningsAsErrorsWhenBuildingSameProjectMultipleTimes()
{
using (TestEnvironment testEnvironment = TestEnvironment.Create(_output))
using (TestEnvironment testEnvironment = TestEnvironment.Create())
{
TransientTestProjectWithFiles project2 = testEnvironment.CreateTestProjectWithFiles($@"
<Project xmlns=""msbuildnamespace"">
Expand Down Expand Up @@ -132,7 +123,7 @@ public void TreatWarningsAsMessagesWhenSpecified()
[Fact]
public void TreatWarningsAsMessagesWhenBuildingSameProjectMultipleTimes()
{
using (TestEnvironment testEnvironment = TestEnvironment.Create(_output))
using (TestEnvironment testEnvironment = TestEnvironment.Create())
{
TransientTestProjectWithFiles project2 = testEnvironment.CreateTestProjectWithFiles($@"
<Project xmlns=""msbuildnamespace"">
Expand Down Expand Up @@ -272,111 +263,5 @@ private string GetTestProject(bool? treatAllWarningsAsErrors = null, string warn
</Target>
</Project>";
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ShouldCauseBuildFailure()
{

using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<Target Name='Build'>
<ReturnFailureWithoutLoggingErrorTask/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectFailure();

logger.AssertLogContains("MSB4132");
}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_WarnAndContinue()
{

using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<Target Name='Build'>
<ReturnFailureWithoutLoggingErrorTask
ContinueOnError=""WarnAndContinue""/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectSuccess();

logger.WarningCount.ShouldBe(1);

logger.AssertLogContains("MSB4132");
}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_True()
{

using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<Target Name='Build'>
<ReturnFailureWithoutLoggingErrorTask
ContinueOnError=""true""/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectSuccess();

logger.AssertLogContains("MSB4132");
}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_ErrorAndStop()
{

using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<Target Name='Build'>
<ReturnFailureWithoutLoggingErrorTask
ContinueOnError=""ErrorAndStop""/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectFailure();

logger.AssertLogContains("MSB4132");
}
}

[Fact]
public void TaskReturnsFailureButDoesNotLogError_ContinueOnError_False()
{

using (TestEnvironment env = TestEnvironment.Create(_output))
{
TransientTestProjectWithFiles proj = env.CreateTestProjectWithFiles($@"
<Project>
<UsingTask TaskName = ""ReturnFailureWithoutLoggingErrorTask"" AssemblyName=""Microsoft.Build.Engine.UnitTests""/>
<Target Name='Build'>
<ReturnFailureWithoutLoggingErrorTask
ContinueOnError=""false""/>
</Target>
</Project>");

MockLogger logger = proj.BuildProjectExpectFailure();

logger.AssertLogContains("MSB4132");
}
}
}
}
1 change: 0 additions & 1 deletion src/Build/BackEnd/Components/Logging/BuildLoggingContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ internal void LogFatalTaskError(Exception exception, BuildEventFileInfo file, st
{
ErrorUtilities.VerifyThrow(IsValid, "must be valid");
LoggingService.LogFatalTaskError(BuildEventContext, exception, file, taskName);
_hasLoggedErrors = true;
}
}
}
11 changes: 0 additions & 11 deletions src/Build/BackEnd/Components/Logging/LoggingContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ internal class LoggingContext
/// </summary>
private bool _isValid;

protected bool _hasLoggedErrors;

/// <summary>
/// Constructs the logging context from a logging service and an event context.
/// </summary>
Expand All @@ -43,7 +41,6 @@ public LoggingContext(ILoggingService loggingService, BuildEventContext eventCon
_loggingService = loggingService;
_eventContext = eventContext;
_isValid = false;
_hasLoggedErrors = false;
}

/// <summary>
Expand Down Expand Up @@ -109,8 +106,6 @@ protected set
}
}

internal bool HasLoggedErrors { get { return _hasLoggedErrors; } set { _hasLoggedErrors = value; } }

/// <summary>
/// Helper method to create a message build event from a string resource and some parameters
/// </summary>
Expand Down Expand Up @@ -144,7 +139,6 @@ internal void LogError(BuildEventFileInfo file, string messageResourceName, para
{
ErrorUtilities.VerifyThrow(_isValid, "must be valid");
_loggingService.LogError(_eventContext, file, messageResourceName, messageArgs);
_hasLoggedErrors = true;
}

/// <summary>
Expand All @@ -158,7 +152,6 @@ internal void LogErrorWithSubcategory(string subcategoryResourceName, BuildEvent
{
ErrorUtilities.VerifyThrow(_isValid, "must be valid");
_loggingService.LogError(_eventContext, subcategoryResourceName, file, messageResourceName, messageArgs);
_hasLoggedErrors = true;
}

/// <summary>
Expand All @@ -173,7 +166,6 @@ internal void LogErrorFromText(string subcategoryResourceName, string errorCode,
{
ErrorUtilities.VerifyThrow(_isValid, "must be valid");
_loggingService.LogErrorFromText(_eventContext, subcategoryResourceName, errorCode, helpKeyword, file, message);
_hasLoggedErrors = true;
}

/// <summary>
Expand All @@ -184,7 +176,6 @@ internal void LogInvalidProjectFileError(InvalidProjectFileException invalidProj
{
ErrorUtilities.VerifyThrow(_isValid, "must be valid");
_loggingService.LogInvalidProjectFileError(_eventContext, invalidProjectFileException);
_hasLoggedErrors = true;
}

/// <summary>
Expand All @@ -198,7 +189,6 @@ internal void LogFatalError(Exception exception, BuildEventFileInfo file, string
{
ErrorUtilities.VerifyThrow(_isValid, "must be valid");
_loggingService.LogFatalError(_eventContext, exception, file, messageResourceName, messageArgs);
_hasLoggedErrors = true;
}

/// <summary>
Expand Down Expand Up @@ -247,7 +237,6 @@ internal void LogFatalBuildError(Exception exception, BuildEventFileInfo file)
{
ErrorUtilities.VerifyThrow(IsValid, "must be valid");
LoggingService.LogFatalBuildError(BuildEventContext, exception, file);
_hasLoggedErrors = true;
}
}
}
24 changes: 0 additions & 24 deletions src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,6 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
if (taskType == typeof(MSBuild))
{
MSBuild msbuildTask = host.TaskInstance as MSBuild;

ErrorUtilities.VerifyThrow(msbuildTask != null, "Unexpected MSBuild internal task.");

var undeclaredProjects = GetUndeclaredProjects(msbuildTask);
Expand Down Expand Up @@ -944,29 +943,6 @@ private async Task<WorkUnitResult> ExecuteInstantiatedTask(ITaskExecutionHost ta
}
}

// When a task fails it must log an error. If a task fails to do so,
// that is logged as an error. MSBuild tasks are an exception because
// errors are not logged directly from them, but the tasks spawned by them.
IBuildEngine be = host.TaskInstance.BuildEngine;
if (taskReturned && !taskResult && !taskLoggingContext.HasLoggedErrors && (be is TaskHost th ? th.BuildRequestsSucceeded : false))
{
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",
_taskNode.Name);
}
}

// If the task returned attempt to gather its outputs. If gathering outputs fails set the taskResults
// to false
if (taskReturned)
Expand Down
Loading