Use dotnet test #48686

jaredpar · 2020-10-16T18:02:46Z

This change is migrating us away from the xUnit console runner and onto
the dotnet test command instead. This has the following advantages:

xunit console runner is not a supported product hence we really need to move off it anyways.
Can take advantage of all the crash dump and blame analysis that they are doing in the dotnet test tool
Simplifies our story a bit because it means we are dealing with a single way to invoke tests across all the different configurations, OS we run. It's dotnet test all the time. This is true even when we are running .NET Framework tests.
One step closer to removing all of the restore logic from our test jobs

This change is migrating us away from the xUnit console runner and onto the `dotnet test` command instead.

Dotnet-GitSync-Bot · 2020-10-16T18:02:50Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

RikkiGibson · 2020-10-16T18:41:08Z

eng/Versions.props

@@ -266,5 +266,7 @@
      rather explicitly override it.
    -->
    <UsingToolMicrosoftNetCompilers Condition="'$(BootstrapBuildPath)' == ''">true</UsingToolMicrosoftNetCompilers>
+
+    <UseVSTestRunner>true</UseVSTestRunner>


do we also need to update MicrosoftNETTestSdkVersion in this file?

What relationship does that have to how the tests run here?

I had the impression that Microsoft.Net.Test.Sdk actually controls what version of the test host gets loaded and that updating it is required to e.g. benefit from the change that makes the test host actually multi-target (or whatever was blocking us from moving to dotnet test before).

jaredpar · 2020-10-19T20:16:01Z

@dotnet/roslyn-infrastructure PTAL

RikkiGibson

LGTM with some comments and nits

RikkiGibson · 2020-10-19T22:11:58Z

src/Tools/Source/RunTests/ProcessTestExecutor.cs

-            {
-                var traits = Options.Trait.Split(new char[] { ';' }, StringSplitOptions.RemoveEmptyEntries);
-                foreach (var trait in traits)
+                void MaybeAddSeparator(char separator = '|')


Please use the camelCase local function naming convention.

I believe the naming style for this directory is configured for PascalCase

This directory is PascalCased yes.

We need to flip the repo to be consistent here now that .NET runtime came out with their recomendations so we can avoid this in the future.

RikkiGibson · 2020-10-19T22:20:33Z

src/Workspaces/Core/Portable/FindSymbols/ReferenceLocationExtensions.cs

@@ -39,6 +39,7 @@ internal static class ReferenceLocationExtensions
                        AddSymbols(semanticModel, documentGroup, result);
                    }

+                    // Keep compilation alive so that GetSemanticModelAsync remains cheap


Was this intended to be included by this PR?

Yes. Just a comment update based on an investigation I did while looking at some of the test runs.

RikkiGibson · 2020-10-19T22:22:27Z

src/Tools/Source/RunTests/ProcessTestExecutor.cs

-            builder.AppendFormat(@"""{0}""", assemblyInfo.AssemblyPath);
-            builder.AppendFormat(@" {0}", assemblyInfo.ExtraArguments);
-            builder.AppendFormat($@" -xml ""{xmlResultsFilePath}""");
+            builder.Append($@"test");


might be worth commenting that this is about coming up with arguments to dotnet CLI in order to run this partition's tests, I had to blink at this for a sec to remember why we were passing the bare string "test" as a command line argument.

RikkiGibson · 2020-10-19T22:25:22Z

eng/Versions.props

@@ -266,5 +266,7 @@
      rather explicitly override it.
    -->
    <UsingToolMicrosoftNetCompilers Condition="'$(BootstrapBuildPath)' == ''">true</UsingToolMicrosoftNetCompilers>
+
+    <UseVSTestRunner>true</UseVSTestRunner>


I had the impression that Microsoft.Net.Test.Sdk actually controls what version of the test host gets loaded and that updating it is required to e.g. benefit from the change that makes the test host actually multi-target (or whatever was blocking us from moving to dotnet test before).

jaredpar · 2020-10-20T19:10:58Z

@sharwell do we need to look at the integration test here? Seems like a known issue.

sharwell · 2020-10-20T19:26:51Z

@jaredpar Second run hit a variation of the bug I mostly worked around with 6b43c88. Third run hit #48548. First run hit both of these plus a lower-priority flaky test that doesn't fail the double-pass often.

jaredpar · 2020-10-20T19:27:36Z

@sharwell so are we good to merge or do we need to retry again?

sharwell · 2020-10-20T19:30:32Z

@jaredpar looks good to me

jaredpar · 2020-10-20T19:32:47Z

The retry mechanism on integration tests is predicated on not having the `--html` option passed to RunTests. PR dotnet#48686 changed it to be unconditionally passed which ended up breaking retry. Undoing the unconditional passing of `--html` to fix retry.

* Restore retry on integration tests The retry mechanism on integration tests is predicated on not having the `--html` option passed to RunTests. PR #48686 changed it to be unconditionally passed which ended up breaking retry. Undoing the unconditional passing of `--html` to fix retry.

Use dotnet test

7f82077

This change is migrating us away from the xUnit console runner and onto the `dotnet test` command instead.

RikkiGibson reviewed Oct 16, 2020

View reviewed changes

jaredpar added 4 commits October 16, 2020 11:45

Fixes

d4042b0

More

fb70278

More

15c27ec

fix IDE tests

91a242f

jaredpar marked this pull request as ready for review October 19, 2020 20:15

jaredpar requested review from a team as code owners October 19, 2020 20:15

RikkiGibson self-assigned this Oct 19, 2020

RikkiGibson approved these changes Oct 19, 2020

View reviewed changes

jaredpar merged commit 83e4631 into dotnet:master Oct 20, 2020

ghost added this to the Next milestone Oct 20, 2020

jaredpar deleted the dntest branch October 20, 2020 19:32

jaredpar mentioned this pull request Oct 21, 2020

Restore retry on integration tests #48812

Merged

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dotnet test #48686

Use dotnet test #48686

jaredpar commented Oct 16, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Oct 16, 2020

RikkiGibson Oct 16, 2020

jaredpar Oct 16, 2020

RikkiGibson Oct 19, 2020

jaredpar commented Oct 19, 2020

RikkiGibson left a comment

RikkiGibson Oct 19, 2020

sharwell Oct 19, 2020

jaredpar Oct 19, 2020

RikkiGibson Oct 19, 2020

jaredpar Oct 19, 2020

RikkiGibson Oct 19, 2020

RikkiGibson Oct 19, 2020

jaredpar commented Oct 20, 2020

sharwell commented Oct 20, 2020

jaredpar commented Oct 20, 2020

sharwell commented Oct 20, 2020

jaredpar commented Oct 20, 2020

Use dotnet test #48686

Use dotnet test #48686

Conversation

jaredpar commented Oct 16, 2020 • edited Loading

Dotnet-GitSync-Bot commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Oct 19, 2020

RikkiGibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Oct 20, 2020

sharwell commented Oct 20, 2020

jaredpar commented Oct 20, 2020

sharwell commented Oct 20, 2020

jaredpar commented Oct 20, 2020

jaredpar commented Oct 16, 2020 •

edited

Loading