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

Attempt to make functional tests more reliable #5874

Merged
merged 6 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions Directory.Solution.targets
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@
<VSTestNoLogo Condition="'$(VSTestNoLogo)' == ''">true</VSTestNoLogo>
<VSTestVerbosity Condition="'$(SYSTEM_DEBUG)' == 'true'">detailed</VSTestVerbosity>
<VSTestVerbosity Condition="'$(VSTestVerbosity)' == ''">quiet</VSTestVerbosity>
<VSTestSetting Condition="'$(VSTestSetting)' == '' And '$(CI)' == 'true'">$([System.IO.Path]::Combine('$(MSBuildThisFileDirectory)', 'build', 'xunit.runsettings'))</VSTestSetting>
<VSTestSetting Condition="'$(VSTestSetting)' == ''">$([System.IO.Path]::Combine('$(MSBuildThisFileDirectory)', 'build', 'xunit.runsettings'))</VSTestSetting>
<VSTestResultsDirectory Condition="'$(VSTestResultsDirectory)' == ''">$([System.IO.Path]::Combine('$(MSBuildThisFileDirectory)', 'TestResults'))</VSTestResultsDirectory>
<VSTestLogger Condition="'$(VSTestLogger)' == ''">console%3Bverbosity=$(VSTestVerbosity)</VSTestLogger>

<!--
Build tests for all supported target frameworks by default, otherwise build tests for just the specified target framework.
Expand Down Expand Up @@ -188,15 +187,24 @@

<ItemGroup>
<!--
Splits the command-line arguments for the test runner into an item group.
Adds the console logger with verbosity and then adds any specified from the command-line.
-->
<VSTestLogger Include="$(VSTestLogger)" />
<VSTestLogger Include="console%3Bverbosity=$(VSTestVerbosity);$(VSTestLogger)" />
</ItemGroup>

<ItemGroup>
<VSTestCommand Include="dotnet test" />
<VSTestCommand Include="@(TestAssembly->'&quot;%(Identity)&quot;', ' ')" />
<VSTestCommand Include="--nologo" Condition="'$(VSTestNoLogo)' == 'true'" />
<VSTestCommand Include="--settings &quot;$(VSTestSetting)&quot;" Condition="'$(VSTestSetting)' != ''" />
<VSTestCommand Include="--ResultsDirectory &quot;$(VSTestResultsDirectory)&quot;" Condition="'$(VSTestResultsDirectory)' != ''" />
<VSTestCommand Include="--diag &quot;$(BinlogDirectory)vstest.diag.log&quot; " Condition="'$(BinlogDirectory)' != ''" />
</ItemGroup>

<!--
Runs the tests via 'dotnet test'.
-->
<Exec Command="dotnet test @(TestAssembly->'&quot;%(Identity)&quot;', ' ') --logger &quot;console;verbosity=$(VSTestVerbosity)&quot; --logger @(VSTestLogger->'&quot;%(Identity)&quot;', ' --logger ') --settings &quot;$(VSTestSetting)&quot; --ResultsDirectory &quot;$(VSTestResultsDirectory)&quot;"
<Exec Command="@(VSTestCommand->'%(Identity)', ' ') --logger @(VSTestLogger->Distinct()->'&quot;%(Identity)&quot;', ' --logger ')"
IgnoreStandardErrorWarningFormat="true"
IgnoreExitCode="true"
LogStandardErrorAsError="false">
Expand Down
7 changes: 0 additions & 7 deletions build/TestShared/xunit.runner.json

This file was deleted.

2 changes: 1 addition & 1 deletion build/build.proj
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@
============================================================
RunTestsOnProjects
Finds all test assemblies and allows Xunit to run them as
efficiently as the xunit.runner.json settings allow.
efficiently as the xunit.runsettings allow.
============================================================
-->
<Target Name="RunTestsOnProjects">
Expand Down
10 changes: 2 additions & 8 deletions build/test.targets
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
</ItemGroup>

<PropertyGroup>
<TestResultsDirectory>$(BuildCommonDirectory)TestResults\</TestResultsDirectory>
<VSTestResultsDirectory>$(RepositoryRootDirectory).test\TestResults\</VSTestResultsDirectory>
<RunSettingsFilePath Condition="'$(RunSettingsFilePath)' == ''">$(MSBuildThisFileDirectory)xunit.runsettings</RunSettingsFilePath>
</PropertyGroup>

<!-- Workaround for test projects not automatically creating binding redirects -->
Expand All @@ -25,11 +26,4 @@
<GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>
</PropertyGroup>

<ItemGroup Condition=" '$(UseParallelXunit)' == 'true' ">
<None Include="$(BuildCommonDirectory)TestShared\xunit.runner.json">
<Link>xunit.runner.json</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
</ItemGroup>

</Project>
60 changes: 42 additions & 18 deletions build/xunit.runsettings
Original file line number Diff line number Diff line change
@@ -1,20 +1,44 @@
<RunSettings>
<RunConfiguration>
<!-- Use as many testhosts as logical processors -->
<MaxCpuCount>0</MaxCpuCount>
<!-- Consider it an error if no tests are discovered -->
<TreatNoTestsAsError>true</TreatNoTestsAsError>
</RunConfiguration>
<DataCollectionRunSettings>
<DataCollectors>
<DataCollector friendlyName="blame" enabled="True">
<Configuration>
<!-- Enables crash dump-->
<CollectDump DumpType="Mini" />
<!-- Enables hang dump-->
<CollectDumpOnTestSessionHang TestTimeout="15min" HangDumpType="Mini" />
</Configuration>
</DataCollector>
</DataCollectors>
</DataCollectionRunSettings>
<!-- https://learn.microsoft.com/visualstudio/test/configure-unit-tests-by-using-a-dot-runsettings-file#runconfiguration-element -->
<RunConfiguration>
<!--
This setting controls the level of parallelism on process-level. Use 0 to enable the maximum process-level parallelism.

This setting determines the maximum number of test DLLs, or other test containers that can run in parallel. Each DLL runs in its own testhost process, and is isolated on the process level from the tests in other test DLLs. This setting doesn't force tests in each test DLL to run in parallel. Controlling the parallel execution within a DLL (on the thread-level) is up to the test framework such as MSTest, XUnit or NUnit.

The default value is 1, meaning that only one testhost runs at the same time. A special value 0 allows as many testhosts as you have logical processors (for example, 6, for a computer with 6 physical cores without multi-threading, or 12, for a computer with six physical cores with multi-threading).

The number of distinct DLLs in the run determines the actual number of testhosts started.
-->
<MaxCpuCount>4</MaxCpuCount>

<!-- Specify a Boolean value, which defines the exit code when no tests are discovered. If the value is true and no tests are discovered, a nonzero exit code is returned. Otherwise, zero is returned. -->
<TreatNoTestsAsError>true</TreatNoTestsAsError>

<EnvironmentVariables>
<!-- Changes the default timeout in VSTest for shutdown to 1000ms instead of 100ms which can cause intermittent failures in CI. -->
<VSTEST_TESTHOST_SHUTDOWN_TIMEOUT>1000</VSTEST_TESTHOST_SHUTDOWN_TIMEOUT>
</EnvironmentVariables>
</RunConfiguration>

<!-- https://xunit.net/docs/runsettings -->
<xUnit>
<!-- Set this value to true to include diagnostic information during test discovery and execution. Each runner has a unique method of presenting diagnostic messages. -->
<DiagnosticMessages>true</DiagnosticMessages>

<!-- Set this value to enable long-running (hung) test detection. When the runner is idle waiting for tests to finished, it will report that fact once the timeout has passed. Use a value of 0 to disable the feature, or a positive integer value to enable the feature (time in seconds). -->
<LongRunningTestSeconds>120</LongRunningTestSeconds>

<!-- Set this to override the maximum number of threads to be used when parallelizing tests within this assembly. Use a value of 0 or "default" to indicate that you would like the default behavior; use a value of -1 or "unlimited" to indicate that you do not wish to limit the number of threads used for parallelization. -->
<MaxParallelThreads>4</MaxParallelThreads>

<!-- Set this to true if this assembly is willing to participate in parallelization with other assemblies. Test runners can use this information to automatically enable parallelization across assemblies if all the assemblies agree to it. -->
<ParallelizeAssembly>false</ParallelizeAssembly>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this setting nullifies the MaxcpuCount (4) property because as per the comment it looks like MaxcpuCount specifies the number of distinct DLLs in the run determines the actual number of testhosts started.. Given that the multiple assemblies are not running parallel then my understanding is that only one test host process is started. I guess we should change to true so that 4 DLLs can run in parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since MaxCpuCount is set to 4 (here), there will be 4 vstest.console.exe processes spawned. I think it then launches each test assembly in those processes in parallel. Then with MaxParallelThreads set to 4, each test assembly runs with 4 threads (if there are enough CPUs).

This is really confusing since there are multiple test runners. You can run tests under xunit.runner.exe, vstest.console.exe, etc. I think the ParallelizeAssembly tells xunit specifically that it should run tests from multiple assemblies at the same time when running under certain test runners. But since we're running the tests under vstest.console.exe, it seems to me that we should not be using xunit's parallelization. I spent a lot of time read https://xunit.net/docs/runsettings but only experimenting with these numbers did I find a good balance between speed and reliability. Do you know more about how all of this works?

Copy link
Contributor

@kartheekp-ms kartheekp-ms Jun 28, 2024

Choose a reason for hiding this comment

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

It looks like a similar issue has been discussed here. I think we are good because it appears that xUnit parallelization is ignored by the dotnet test command. One interesting thing I learned from this link is that executing dotnet test without specifying assemblies runs test assemblies in parallel, whereas dotnet test assembly1.dll assembly2.dll does not run test assemblies in parallel. I am not sure if this is accurate in .NET 8, but at least that was the case earlier because the linked comment explained the different behaviors with an example.


<!-- Set this to true if the assembly is willing to run tests inside this assembly in parallel against each other. Tests in the same test collection will be run sequentially against each other, but tests in different test collections will be run in parallel against each other. Set this to false to disable all parallelization within this test assembly. -->
<ParallelizeTestCollections>false</ParallelizeTestCollections>
Copy link
Member

Choose a reason for hiding this comment

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

I compared the CI run for this PR to a different CI run, because I would have expected that setting ParallelizeTestCollections and ParallelizeAssembly both to false would make the tests run very slowly. But somehow this PR's CI run was margionally faster then the dev branch run.

It's totally not intuitive to me why, but I didn't see this mentioned in the PR description or anywhere else, so I wanted to point out that apparently the CI is not going to take a long longer with this PR merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I struggle to understand it as well. Its still running tests assemblies in parallel with each other, but I guess not running tests themselves in parallel? A machine with only 2 cores would only be able to run two tests at the same time anyway so I think the adjustments don't affect the maximum parallelization we could already achieve on CI agents. It really just lowers the amount of parallelization that's queued up waiting for CPU.


<!-- Set this to true to use shadow copying when running tests in separate App Domains; set to false to run tests without shadow copying. When running tests without App Domains, this value is ignored. -->
<ShadowCopy>false</ShadowCopy>
</xUnit>
</RunSettings>
2 changes: 1 addition & 1 deletion eng/pipelines/pr.job.yml
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ jobs:
condition: succeeded()
inputs:
command: test
arguments: --no-restore --restore:false --no-build --framework $(TestTargetFramework) --blame --logger trx --binarylogger:"$(BinlogDirectory)02-RunTests.binlog"
Copy link
Member

Choose a reason for hiding this comment

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

Finally, I've disabled the crash dump helper when running tests since all of the memory dumps it provided were always for the main vstest.console.exe which wasn't helpful since the tests themselves are running in a different process

Have you talked to the team who owns dotnet test? This sounds very surprising so sounds like something that should be fixed for all customers, or if we're doing something wrong, they can help us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see a few issues in the vstest repo:
microsoft/vstest#3936
microsoft/vstest#2593
microsoft/vstest#4376
microsoft/vstest#2952

Nothing specific to the problem I was seeing. Supposedly the blame stuff also doesn't supply enough command-line arguments to capture other exceptions than StackOverflowException and AccessViolationException

https://github.com/microsoft/vstest/blob/main/src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpArgsBuilder.cs#L33

Although now there's an env var we can set to include more arguments to proc dump. The main point in all of this is it seems like adding --diag to the vstest.console.exe execution is better. I don't think we have that many "test crashes". But if we feel like we need the memory dumps back, we can do so at a later time in my opinion.

arguments: --no-restore --restore:false --no-build --framework $(TestTargetFramework) --binarylogger:"$(BinlogDirectory)02-RunTests.binlog"
testRunTitle: ${{ parameters.osName }} $(TestType) Tests ($(TestTargetFramework))

- task: PublishPipelineArtifact@1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@
<Reference Include="System.IO.Compression" />
</ItemGroup>

<ItemGroup>
<None Include="xunit.runner.json"
CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>

<Target Name="CopyFinalNuGetExeToOutputPath"
AfterTargets="PrepareForRun"
Inputs="$(ArtifactsDirectory)$(VsixOutputDirName)\NuGet.exe"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>$(NETFXTargetFramework)</TargetFramework>
<UseParallelXunit>true</UseParallelXunit>
<Description>An end-to-end test suite for NuGet.CommandLine. Contains tests for every nuget.exe CLI command. Overlaps in tests with NuGet.CommandLine.FuncTest.</Description>
<TestProjectType Condition="'$(IsXPlat)' != 'true'">Functional</TestProjectType>
</PropertyGroup>
Expand Down
Loading
Loading