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...

nohwnd · 2023-11-16T08:54:52Z

/azp run

azure-pipelines · 2023-11-16T08:55:04Z

Azure Pipelines successfully started running 1 pipeline(s).

mcartoixa · 2023-11-17T11:01:04Z

Great to see something done after 3 years! 👏🏻

As a user, I still can't wrap my head around this line though: if I use MSBuild, I have to set VsTestUseMSBuildOutput=True to gain MSBuild compatibility?

nohwnd · 2023-11-17T18:14:31Z

Yes, for now, I want to get feedback first before we ship as the default.

…

-jakub

________________________________ Od: Mac ***@***.***> Odesláno: Friday, November 17, 2023 12:01:15 PM Komu: microsoft/vstest ***@***.***> Kopie: Jakub Jares ***@***.***>; Mention ***@***.***> Předmět: Re: [microsoft/vstest] Create a proper MSBuild ToolTask based VSTestTask (#2702) Great to see something done after 3 years! 👏🏻 As a user, I still can't wrap my head around this line<https://github.com/microsoft/vstest/blob/main/src/Microsoft.TestPlatform.Build/Microsoft.TestPlatform.targets#L18> though: if I use MSBuild, I have to set VsTestUseMSBuildOutput=True to gain MSBuild compatibility? — Reply to this email directly, view it on GitHub<#2702 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABLYLYNB2ZTJBG3AGZXV2P3YE47XXAVCNFSM4WFIUWFKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBRGYYTMMRWGE2Q>. You are receiving this because you were mentioned.Message ID: ***@***.***>

KirillOsenkov · 2023-11-17T19:36:56Z

Thank you both so much @mcartoixa and @nohwnd! Lots of hard work went into this. Very happy to see it merged.

nohwnd · 2023-11-20T13:53:51Z

Thanks, I will update here once we release it to somewhere. :)

nohwnd · 2023-11-27T10:07:30Z

Not yet in 9.0.100-alpha.1.23564.26.

nohwnd · 2024-01-23T12:54:43Z

@mcartoixa @KirillOsenkov this is now default (as well as terminal logger, and reusing nodes) in net9 9.0.100-alpha.1.24073.1 and newer. Feedback is welcome, and collected here #4843 😊

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

nohwnd added 4 commits November 15, 2023 13:15

Revert console logger

d46898c

Revert integration tests

f2df5fa

Revert change in integration tests

9f17c68

Fix test

47058ee

Revert test unignore

bafd154

nohwnd approved these changes Nov 16, 2023

View reviewed changes

microsoft-github-policy-service bot merged commit b99e5fa into microsoft:main Nov 16, 2023
7 checks passed

nohwnd mentioned this pull request Jan 3, 2024

Improve output of dotnet test #3842

Open

This was referenced Jan 19, 2024

Enable msbuild terminal logger for dotnet test dotnet/sdk#38098

Merged

Terminal logger integration #4843

Closed

nohwnd mentioned this pull request Jul 8, 2024

dotnet test should forward msbuild errors through standard error stream #2706

Closed

mcartoixa mentioned this pull request Jul 22, 2024

dotnet msbuild /t:VSTest does not work with terminal logger #5156

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a proper MSBuild ToolTask based VSTestTask #2702

Create a proper MSBuild ToolTask based VSTestTask #2702

mcartoixa commented Jan 16, 2021 •

edited by nohwnd

Loading

mcartoixa commented Feb 6, 2021

Haplois commented Feb 6, 2021

Haplois commented Feb 6, 2021

azure-pipelines bot commented Feb 6, 2021

mcartoixa commented May 26, 2021 •

edited

Loading

KirillOsenkov Jun 30, 2021

mcartoixa Jun 30, 2021 •

edited

Loading

KirillOsenkov Jun 30, 2021

mcartoixa Jun 30, 2021

KirillOsenkov Jun 30, 2021

mcartoixa Jun 30, 2021

MarkKharitonov commented Jun 30, 2021

mcartoixa commented Jun 30, 2021

mcartoixa commented Oct 24, 2021

mcartoixa commented Feb 8, 2022

mcartoixa commented Apr 22, 2022 •

edited

Loading

mcartoixa commented Jun 9, 2022

Nirmal4G commented Jun 28, 2022

Nirmal4G left a comment

mcartoixa commented Jun 28, 2022

nohwnd commented Nov 16, 2023

azure-pipelines bot commented Nov 16, 2023

mcartoixa commented Nov 17, 2023

nohwnd commented Nov 17, 2023 via email

KirillOsenkov commented Nov 17, 2023

nohwnd commented Nov 20, 2023

nohwnd commented Nov 27, 2023

nohwnd commented Jan 23, 2024

Create a proper MSBuild ToolTask based VSTestTask #2702

Create a proper MSBuild ToolTask based VSTestTask #2702

Conversation

mcartoixa commented Jan 16, 2021 • edited by nohwnd Loading

Description

Related issue

mcartoixa commented Feb 6, 2021

Haplois commented Feb 6, 2021

Haplois commented Feb 6, 2021

azure-pipelines bot commented Feb 6, 2021

mcartoixa commented May 26, 2021 • edited Loading

KirillOsenkov Jun 30, 2021

Choose a reason for hiding this comment

mcartoixa Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

KirillOsenkov Jun 30, 2021

Choose a reason for hiding this comment

mcartoixa Jun 30, 2021

Choose a reason for hiding this comment

KirillOsenkov Jun 30, 2021

Choose a reason for hiding this comment

mcartoixa Jun 30, 2021

Choose a reason for hiding this comment

MarkKharitonov commented Jun 30, 2021

mcartoixa commented Jun 30, 2021

mcartoixa commented Oct 24, 2021

mcartoixa commented Feb 8, 2022

mcartoixa commented Apr 22, 2022 • edited Loading

mcartoixa commented Jun 9, 2022

Nirmal4G commented Jun 28, 2022

Nirmal4G left a comment

Choose a reason for hiding this comment

mcartoixa commented Jun 28, 2022

nohwnd commented Nov 16, 2023

azure-pipelines bot commented Nov 16, 2023

mcartoixa commented Nov 17, 2023

nohwnd commented Nov 17, 2023 via email

KirillOsenkov commented Nov 17, 2023

nohwnd commented Nov 20, 2023

nohwnd commented Nov 27, 2023

nohwnd commented Jan 23, 2024

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