microsoft / vstest

Visual Studio Test Platform is the runner and engine that powers test explorer and vstest.console.
MIT License
900 stars 323 forks source link

[Bug]: dotnet test does not report failed test(s) information if terminal logger disabled #10358

Open rainersigwald opened 1 month ago

rainersigwald commented 1 month ago

Issue moved from dotnet/msbuild#10682


From @martincostello on Saturday, September 21, 2024 3:42:19 PM

Issue Description

If the terminal logger is disabled (e.g. when running in CI in GitHub Actions) and a test fails, no visible output about the state of the test run is output to the console. Only an error code of 1 is returned: example

The user has to depend on an additional logger (such as GitHubActionsTestLogger) to see which tests failed and why, or has to re-run the tests locally with the terminal logger enabled to see what the failure is (which is problematic for CI-only failures).

Steps to Reproduce

To repro the exact failure above locally:

  1. Clone https://github.com/martincostello/openapi-extensions/pull/103/commits/0a12b92320d59954193a4ef067baedbfd974bfb6
  2. Edit build.ps1 to add --tl:off (code)
  3. Run build.ps1

Alternatively, run dotnet test --tl:off for a test suite using the latest .NET 9 RC2 build where one of the tests fails.

Expected Behavior

Some sort of message showing what tests failed is printed to the console. Ideally this would be similar to .NET 8's output modulo any improvements to the output of the total test counts etc.

Actual Behavior

Nothing is emitted that gives a hint as to whether tests are being run, or what the outcome was.

Analysis

A hunch tells me this has regressed at some point since preview 7.

Versions & Configurations

I can repro this with both 9.0.100-rc.1.24452.12 and 9.0.100-rc.2.24468.2 (a build from the last 24 hours).

nohwnd commented 1 month ago

oh god that's bad. @martincostello I am looking into this.

nohwnd commented 1 month ago

In the old flow where you specify -p:VSTestUseMSBuildOutput=false to disable the new output, we parse it out from that execution parameters inside of dotnet test command (so we process it way before MSBuild),

https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/commands/dotnet-test/Program.cs#L61

And we set: MSBUILDENSURESTDOUTFORTASKPROCESSES=1, before calling msbuild. That gets picked up here:

https://github.com/dotnet/msbuild/blob/main/src/MSBuild/XMake.cs#L251

And it sets up the child processes in a special way so vstest can write to standard output and show errors.

But with the new approach of detecting -tl:false,

https://github.com/dotnet/msbuild/blob/main/src/MSBuild/XMake.cs#L2529-L2533

we don't set MSBUILDENSURESTDOUTFORTASKPROCESSES env variable.
This makes all the vstest.console output to be lost.


Looking at the logic of detecting if TL should be enabled and it is pretty complex. So the only option I see is that we change MSBuild to be aware of vstest, by moving the check for -tl:false in xmake above the detection of MSBUILDENSURESTDOUTFORTASKPROCESSES, and check if targets include VSTest. and set MSBUILDENSURESTDOUTFORTASKPROCESSES=1.

But that seems dirty, do you see any other options @rainersigwald or @MichalPavlik ?

rainersigwald commented 1 month ago

I wouldn't want to regress the performance of folks who opt out of -tl by setting MSBUILDENSURESTDOUTFORTASKPROCESSES all the time when opted out. @baronfel do you see an easy way forward?

baronfel commented 1 month ago

Is this yet another case where it would be useful if the CLI knew what the TL status was? Do we need to duplicate/capture that logic in the SDK at this point?

rainersigwald commented 1 month ago

It seems like it.

nohwnd commented 1 month ago

Yes, if we’d know in CLI then we can just set MSBUILDENSURESTDOUTFORTASKPROCESSES for the old flow as we did until now.

nohwnd commented 1 month ago

@baronfel does this mean you will be exporting the functionality from MSBuild so we can use it in sdk? :)

nohwnd commented 1 month ago

fixed in https://github.com/dotnet/sdk/pull/43871