Create a proper MSBuild ToolTask based VSTestTask #2702

mcartoixa · 2021-01-16T13:38:15Z

Description

Update from nohwnd:

A new task VSTestTask2 and a logger MSBuildLogger (for internal use only) was added. And can be enabled by setting /p:VsTestUseMSBuildOutput=true.

This is not enabled anywhere by default, neither in dotnet test, or under vstest, because I want to collect some feedback first.

The current console forwarding task has been renamed VSTestForwardingTask. The new VSTestTask, based on ToolTask, properly integrates into the MSBuild framework.
If the $(VSTestUseConsole) property is set to True during the build, the old console forwarding VSTestForwardTask is used: logs are lost but output is colorized (like today). Otherwise the new VSTestTask is used: logs are fine but colorization is lost (except for errors, that are properly recognized by MSBuild).

A new PR will be submitted right away on the dotnet/sdk project so that dotnet test sets the $(VSTestUseConsole) property.

In effect:

users that favor a colorized console output can use dotnet test (like today).
users that favor a proper MSBuild integration can use dotnet msbuild -Target:VSTest.

This PR feels more like a workaround than a proper fix: it still feels weird to depend on a MSBuild task (VSTestForwardingTask) that is not entirely MSBuild friendly... But it seems like the easiest fix that is compatible with the old behavior.
A proper fix would probably center around the implementation of dotnet test which could call dotnet exec vstest.console.dll directly (in addition to MSBuild, for the build part only). This would render current workarounds like for dotnet/msbuild#3130, or AllowFailureWithoutError definitely obsolete.

Related issue

Fixes (or at least gives an option around) #680, #1503, #2224, dotnet/sdk#9481, dotnet/msbuild#5387 and probably more...
This is a new attempt after #2437.

mcartoixa · 2021-02-06T08:49:06Z

Please note that I would be happy to answer any questions and/or reservations, and/or discuss any suggestions or improvements on this PR.

Haplois · 2021-02-06T22:21:48Z

Thank you for the contribution @mcartoixa, I'll check this during the week and ask my questions, if any.

Haplois · 2021-02-06T22:22:10Z

/azp run

azure-pipelines · 2021-02-06T22:22:19Z

Azure Pipelines successfully started running 1 pipeline(s).

mcartoixa · 2021-05-26T08:03:11Z

I just rebased this PR against the main branch so that it would not be an excuse for not merging it 😉

KirillOsenkov · 2021-06-30T01:53:10Z

src/Microsoft.TestPlatform.Build/Tasks/VSTestForwardingTask.cs

+
+                activeProcess.WaitForExit();
+                Tracing.Trace("VSTest: Exit code: " + activeProcess.ExitCode);
+                return activeProcess.ExitCode == 0;


We should only return !Log.HasLoggedErrors. The task should fail if and only if it has logged any errors.

I believe this is the way it's usually done in MSBuild tasks. Does vstest.console.exe return an error code even in cases of success?
Anyway this task is a copy (mostly left unchanged) of the current one, kept here for compatibility purposes. The goal of this PR is rather to introduce a new task that would be more respectful of MSBuild, see VSTestTask (the diff is rather useless on that one, though).

KirillOsenkov · 2021-06-30T01:54:47Z

src/Microsoft.TestPlatform.Build/Tasks/VSTestForwardingTask.cs

+
+            if (!string.IsNullOrEmpty(this.VSTestFramework))
+            {
+                Console.WriteLine(Resources.TestRunningSummary, this.TestFileFullPath, this.VSTestFramework);


A task shouldn't use Console.WriteLine, but instead use Log.LogMessage:
https://source.dot.net/#Microsoft.Build.Tasks.Core/Copy.cs,255

Agreed in general, but this has been left unchanged for compatibility with current behavior. The goal of this PR is rather to introduce a new task that would be more respectful of MSBuild, see VSTestTask.

KirillOsenkov · 2021-06-30T01:55:23Z

src/Microsoft.TestPlatform.Build/Tasks/VSTestForwardingTask.cs

+            // Avoid logging "Task returned false but did not log an error." on test failure, because we don't
+            // write MSBuild error. https://github.com/dotnet/msbuild/blob/51a1071f8871e0c93afbaf1b2ac2c9e59c7b6491/src/Framework/IBuildEngine7.cs#L12
+            var allowfailureWithoutError = BuildEngine.GetType().GetProperty("AllowFailureWithoutError");
+            allowfailureWithoutError?.SetValue(BuildEngine, true);


we should really not do this. If there was an error, log an MSBuild error. If there wasn't an error, don't log it.

Let's remove this reflection logic.

This has been left unchanged for compatibility with current behavior. The goal of this PR is rather to introduce a new task that would be more respectful of MSBuild, see VSTestTask.

MarkKharitonov · 2021-06-30T03:57:50Z

Just to make it clear to me. The VSTestUseConsole build property is introduced by this PR, which has not been merged yet, right?

mcartoixa · 2021-06-30T04:49:54Z

@MarkKharitonov Right.
And to ensure compatibility would require the reopening of dotnet/sdk#15376 to initialize the VSTestUseConsole property in the dotnet test command.

mcartoixa · 2021-10-24T07:52:05Z

Just rebased again against main (like I seem to do every 5 months or so) just in case somebody suddenly were to be interested in getting this PR merged. 😉

mcartoixa · 2022-02-08T07:26:49Z

When can we expect some kind of decision about the future of this PR? As mentioned before quite a few bugs could be affected by this.

Is it going to be rejected, and why?
Is it going to be merged, and when?

mcartoixa · 2022-04-22T19:37:56Z

It took me 2 days to set up a new barely working development environment, but I think I managed to rebase this PR against main. Again.

mcartoixa · 2022-06-09T13:27:26Z

Any update on the status of this request?

Nirmal4G · 2022-06-28T07:48:32Z

nit: file should be renamed too!

test/Microsoft.TestPlatform.Build.UnitTests/VsTestTaskTests.cs -> VSTestTaskTests.cs

Nirmal4G

Overall I like it. It's a welcome change but a huge breaking change nonetheless, so its up to the owners, I think!

mcartoixa · 2022-06-28T08:34:49Z

@Nirmal4G Thanks for the review! There is nothing more that I would like than having a discussion with the owners.

Meanwhile it seems that I will have to rebase this PR again...

mcartoixa mentioned this pull request Jan 16, 2021

Add the $(VSTestUseConsole) property to the dotnet test command dotnet/sdk#15376

Closed

mcartoixa mentioned this pull request May 17, 2021

dotnet test is incompatible with MSBuild node reuse #1503

Closed

mcartoixa force-pushed the vstesttask-complete branch 2 times, most recently from 569e421 to cefe13d Compare May 26, 2021 06:40

nohwnd mentioned this pull request Jun 29, 2021

Better fix for MSBuild task failed but did not log an error #2953

Closed

KirillOsenkov reviewed Jun 30, 2021

View reviewed changes

mcartoixa force-pushed the vstesttask-complete branch from cefe13d to 7acb932 Compare October 24, 2021 07:47

mcartoixa mentioned this pull request Feb 8, 2022

VSTestTask only logs to the console #680

Closed

mcartoixa force-pushed the vstesttask-complete branch from 7acb932 to b6dcdc8 Compare April 22, 2022 19:35

mcartoixa requested review from nohwnd and MarcoRossignoli as code owners April 22, 2022 19:35

mcartoixa force-pushed the vstesttask-complete branch 2 times, most recently from d1e30d4 to 1b4b548 Compare April 22, 2022 20:00

nohwnd self-assigned this Apr 22, 2022

Nirmal4G approved these changes Jun 28, 2022

View reviewed changes

Create a proper MSBuild ToolTask based VSTestTask #2702

Create a proper MSBuild ToolTask based VSTestTask #2702

Uh oh!

Conversation

mcartoixa commented Jan 16, 2021 • edited by nohwnd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issue

Uh oh!

mcartoixa commented Feb 6, 2021

Uh oh!

Haplois commented Feb 6, 2021

Uh oh!

Haplois commented Feb 6, 2021

Uh oh!

azure-pipelines bot commented Feb 6, 2021

Uh oh!

mcartoixa commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KirillOsenkov Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

mcartoixa Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirillOsenkov Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

mcartoixa Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

KirillOsenkov Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

mcartoixa Jun 30, 2021

Choose a reason for hiding this comment

Uh oh!

MarkKharitonov commented Jun 30, 2021

Uh oh!

mcartoixa commented Jun 30, 2021

Uh oh!

mcartoixa commented Oct 24, 2021

Uh oh!

mcartoixa commented Feb 8, 2022

Uh oh!

mcartoixa commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcartoixa commented Jun 9, 2022

Uh oh!

Nirmal4G commented Jun 28, 2022

Uh oh!

Nirmal4G left a comment

Choose a reason for hiding this comment

Uh oh!

mcartoixa commented Jun 28, 2022

Uh oh!

Uh oh!

mcartoixa commented Jan 16, 2021 •

edited by nohwnd

Loading

mcartoixa commented May 26, 2021 •

edited

Loading

mcartoixa Jun 30, 2021 •

edited

Loading

mcartoixa commented Apr 22, 2022 •

edited

Loading