Conversation
This change is migrating us away from the xUnit console runner and onto the `dotnet test` command instead.
|
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. |
| --> | ||
| <UsingToolMicrosoftNetCompilers Condition="'$(BootstrapBuildPath)' == ''">true</UsingToolMicrosoftNetCompilers> | ||
|
|
||
| <UseVSTestRunner>true</UseVSTestRunner> |
There was a problem hiding this comment.
do we also need to update MicrosoftNETTestSdkVersion in this file?
There was a problem hiding this comment.
What relationship does that have to how the tests run here?
There was a problem hiding this comment.
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).
|
@dotnet/roslyn-infrastructure PTAL |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM with some comments and nits
| { | ||
| var traits = Options.Trait.Split(new char[] { ';' }, StringSplitOptions.RemoveEmptyEntries); | ||
| foreach (var trait in traits) | ||
| void MaybeAddSeparator(char separator = '|') |
There was a problem hiding this comment.
Please use the camelCase local function naming convention.
There was a problem hiding this comment.
I believe the naming style for this directory is configured for PascalCase
There was a problem hiding this comment.
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.
| AddSymbols(semanticModel, documentGroup, result); | ||
| } | ||
|
|
||
| // Keep compilation alive so that GetSemanticModelAsync remains cheap |
There was a problem hiding this comment.
Was this intended to be included by this PR?
There was a problem hiding this comment.
Yes. Just a comment update based on an investigation I did while looking at some of the test runs.
| builder.AppendFormat(@"""{0}""", assemblyInfo.AssemblyPath); | ||
| builder.AppendFormat(@" {0}", assemblyInfo.ExtraArguments); | ||
| builder.AppendFormat($@" -xml ""{xmlResultsFilePath}"""); | ||
| builder.Append($@"test"); |
There was a problem hiding this comment.
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.
| --> | ||
| <UsingToolMicrosoftNetCompilers Condition="'$(BootstrapBuildPath)' == ''">true</UsingToolMicrosoftNetCompilers> | ||
|
|
||
| <UseVSTestRunner>true</UseVSTestRunner> |
There was a problem hiding this comment.
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).
|
@sharwell do we need to look at the integration test here? Seems like a known issue. |
|
@sharwell so are we good to merge or do we need to retry again? |
|
@jaredpar looks good to me |
|
|
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.
This change is migrating us away from the xUnit console runner and onto
the
dotnet testcommand instead. This has the following advantages: