Create a proper MSBuild ToolTask based VSTestTask

[mcartoixa]

Description

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]

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]

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

[Haplois]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[mcartoixa]

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

[MarkKharitonov]

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

[mcartoixa]

@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]

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]

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]

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]

Any update on the status of this request?

[Nirmal4G]

nit: file should be renamed too!

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

[mcartoixa]

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

[Nirmal4G]

I think they are delaying this because of some internal politics. I'm sure the team wants this more than anyone. Even @KirillOsenkov is enthusiastic about this!! 😉

[KirillOsenkov]

I can't speak for the MSBuild team but there are only a handful of people on the team, they're all fantastic, but super busy and overloaded with constant firefighting. I apologize for the delay but hopefully they will have time for this at some point soon. Just wanted to clarify that it's not the team's fault that their response times are like this, they're just overloaded with work.

[Nirmal4G]

I'm very interested in this. It's bit harder to review the source to check if nothing's changed, that's why I'm asking for two logical commits, one for the rename and other is the ToolTask based task implementation. I'll re-review the source once that's done.

[mcartoixa]

@Nirmal4G I can't believe I got feedback on this PR (and on a weekend, too)... Thanks! 🥲

My setup is a bit wonky: I can run the build but I can't compile on Visual Studio (hence the several forced commits after every merge...), so I will try and do my best under these conditions.

I think I understand your reasons for 2 commits. But as the goal of this change is to have 2 tasks that do the same thing in a different way their common behaviour has been gathered in an extension method, and it has also been slightly refactored to make better use of the MSBuild framework. So if I simply rename the existing task in the first commit, there will still be a lot going on in the second commit (interface, refactoring, extension method). Maybe I could try to rename and refactor the existing task in the first commit, then introduce the new one and simply move the common behaviour in the extension method in the second commit. What do you think?

I'll also refactor the targets file as indicated (though the ultimate fate of these Message tasks is to disappear IMO).

[mcartoixa]

@Nirmal4G I have finally settled on 3 commits. Tell me what you think.

[mcartoixa]

@Evangelink Thanks for the review. 🥲

Unfortunately I just lost my main computer. I hope it's just an issue with the PSU 🤞🏻 I've ordered a new one but I may take some time before addressing your suggestions... Most of them are on the legacy code though 😉

[Evangelink]

@mcartoixa No worries, take your time. We need to discuss and validate this change quite extensively anyways.

[mcartoixa]

@Evangelink I think I have addressed most of your points in the latest commits. Let me know what you think.

[Evangelink]

I have run some tests locally and so far I have only seen some "problem" when there are errors running tests.

For example,

Before:

Test run for C:\XXX\TestProject1\TestProject3\bin\Debug\net6.0\TestProject3.dll (.NETCoreApp,Version=v6.0)
Microsoft (R) Test Execution Command Line Tool Version 17.3.0-preview-20220523-03 (x64)
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
Html Logger Error : '', hexadecimal value 0xFFFE, is an invalid character. Line 1, position 461.

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: < 1 ms - TestProject3.dll (net6.0)

After:

C:\XXX\artifacts\testArtifacts\dotnet\sdk\7.0.100-preview.4.22252.9\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets(216,5): message NETSDK1057: You are using a preview v
ersion of .NET. See: https://aka.ms/dotnet-support-policy [C:\XXX\TestProject1\TestProject3\TestProject3.csproj]
  TestProject3 -> C:\XXX\TestProject1\TestProject3\bin\Debug\net6.0\TestProject3.dll
C:\XXX\artifacts\testArtifacts\dotnet\sdk\7.0.100-preview.4.22252.9\Microsoft.TestPlatform.targets(41,5): error : Html Logger Error : '?', hexadecimal value 0xFFFE, is an invalid character. Line 1, pos
ition 461. [C:\XXX\TestProject1\TestProject3\TestProject3.csproj]

[mcartoixa]

@Evangelink This behaviour does not surprise me. I assume you are testing the dotnet test command here. In this PR, legacy behaviour is guarantied only if the $(VSTestUseConsole) property is set to true (cf. the https://github.com/dotnet/sdk/pull/15376 PR). This is obviously debatable, but the correct behaviour should be by default and IMHO the correct behaviour of a MSBuild task is to conform to the MSBuild standard: less workarounds and more predictability. If you SET VSTestUseConsole=True before running your tests you should see no difference in the output. As I indicated (a long time ago), the $(VSTestUseConsole) property should then be properly initialized inthe dotnet test command itself. MSBuild is very limited in its ability to colorize outputs. But its ouputs are very clearly organized and you can see more or less by playing with the verbosity level. Errors are the highest level, which is why you can still see them in the output.

So to recap: in the short term, dotnet test should set the $(VSTestUseConsole) property to true to kee the current behaviour. In the long term it would be a good idea IMHO if dotnet test did not depend on MSBuild but on dotnet vstest.console.dll directly. This way you can keep the colorized output, and the MSBuild task can be used when it is beneficial. In the context of Continuous Integration for instance.

[Evangelink]

@mcartoixa It's not easy to have the full picture at first so thank you so much for the explanation! I will make more manual tests and ping the team to have more reviews.

I confirm that setting VSTestUseConsole to true restore current behavior.

[mcartoixa]

This commit fixed the integration tests by setting the VSTestUseConsole property btw 😉 Anyone knows why the microsoft.vstest.pr checks don't run anymore?

[Evangelink]

/azp run

[azure-pipelines[bot]]

Pull request contains merge conflicts.

[Evangelink]

@mcartoixa Not sure but from bot message it looks like this is due to merge conflicts (we had to touch the file in some other PR).

[Evangelink]

@mcartoixa Again, thank you for your commitment to this PR!

[mcartoixa]

Thanks. The first 18 months were the hardest (it felt pretty lonely around here) 😉