Skip to content
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.

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

Forward standard output of testhost #4998

Merged
merged 5 commits into from May 2, 2024
Merged

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Apr 25, 2024

Description

When Tests write output using Console.WriteLine it may or might not be seen by the user, depending on which target framework they are using, which OS, and which testing framework.

When it is seen:

User is running .NET Framework tests in-process, is using vstest.console 17.6.3 or earlier, and is not using MSTest.
User is running tests on Linux.

When it is not seen:

  • User uses .NET on Windows.
  • User uses MSTest pre 3.3.2, or 3.3.2+ without <MSTest><CaptureTraceOutput>false</CaptureTraceOutput></MSTest> option.
  • User uses vstest.console 17.7.0 or newer with .NET Framework in-process or out of process.
  • User is running in isolation on Windows:
    • always under dotnet test
    • always under Visual Studio
    • for .NET workloads under vstest.console

Solution

This pull request standardizes the output capturing and passing to parent processes via Informational TestMessage. This same mechanism is used by xUnit to send their [xUnit.net 00:00:00.11] Starting: xunit001 messages.

Two options are added and enabled by default:

<RunSettings>
    <RunConfiguration>
        <CaptureStandardOutput>true</CaptureStandardOutput>
        <ForwardStandardOutput>true</ForwardStandardOutput>
    </RunConfiguration>
</RunSettings>

CaptureStandardOutput enables output capturing by VSTest. When disabled the behavior returns to the one of 17.6.3 and earlier.
ForwardStandardOutput enables output forwarding. When enabled it forwards output to VS.

When disabled, but CaptureStandardOutput is enabled it will suppress all output and return to behavior in 17.7.0 and newer.

Additionally two feature flags are added to allow easier global opt-out:

VSTEST_DISABLE_STANDARD_OUTPUT_CAPTURING
VSTEST_DISABLE_STANDARD_OUTPUT_FORWARDING

Setting them to 1 has the same effect as setting the above options to FALSE.

Related issue

Fix #799
Fix #4828
Fix #4947

@nohwnd nohwnd marked this pull request as draft April 25, 2024 16:21
{
_communicationManager = communicationManager;
_dataSerializer = dataSerializer;
_platformEnvironment = platformEnvironment;
_testSessionMessageLogger = TestSessionMessageLogger.Instance;
_forwardOutput = !FeatureFlag.Instance.IsSet(FeatureFlag.VSTEST_DISABLE_STANDARD_OUTPUT_FORWARDING);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have runsettings here yet, this is just to be extra sure this behavior can be disabled. Normally user would provide settings to disable the forwarder downstream, and there would be no data to forward.

@@ -16,7 +16,7 @@ public ExtensionDecoratorFactory(IFeatureFlag featureFlag)

public ITestExecutor Decorate(ITestExecutor originalTestExecutor)
{
return _featureFlag.IsSet(FeatureFlag.DISABLE_SERIALTESTRUN_DECORATOR)
return _featureFlag.IsSet(FeatureFlag.VSTEST_DISABLE_SERIALTESTRUN_DECORATOR)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for mixing changes, but having the names defined as prefix VSTEST_ and and name DISABLE_SERIALTESTRUN_DECORATOR that you actually see here in the code was confusing me on multiple occasions where I forgot to put the vstest_ before the flag.

This is not a breaking change, because the flags are string constants so they are emitted directly into the consuming assembly, and the content of this const did not change.

@nohwnd
Copy link
Member Author

nohwnd commented Apr 25, 2024

Draft because I want to add a test or two.

@nohwnd
Copy link
Member Author

nohwnd commented Apr 29, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nohwnd nohwnd marked this pull request as ready for review April 29, 2024 08:39
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised by the added test that is disabled directly. Can we run it manually?

Could be worth having a test asset for xUnit and NUnit just as some safetynet that this works for all 3.

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving again. I still think it'd be worth being belt and braces with an xUnit and NUnit test assets.

@nohwnd
Copy link
Member Author

nohwnd commented Apr 30, 2024

We are capturing process output, if the test framework lets the output to pass through we will capture it. There is no cooperation from the framework needed, as long as the output is able to reach the process output.

@nohwnd nohwnd enabled auto-merge (squash) April 30, 2024 09:30
@nohwnd nohwnd disabled auto-merge May 2, 2024 12:26
@nohwnd nohwnd merged commit 1f9905a into main May 2, 2024
7 checks passed
@nohwnd nohwnd deleted the capture-and-forward-output branch May 2, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants