Switch to VSTest in CI #39923
Switch to VSTest in CI #39923
Conversation
|
Tagging subscribers to this area: @safern, @ViktorHofer |
|
Are any markdown edits necessary? eg in https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/libraries/** ? I thnk use of xunit there is just for the library. |
|
Will this also fix #1664? |
|
Yes this will fix the issue linked above but only for libraries. CoreClr is an entirely different beast and can't speak to that. Yeah, I'll update the markdown files but need to figure the failures out first. |
|
I believe failures in the net48 leg are due to the testhost being compiled against an older .NETFramework configuration and compat switches not being respected. The .NETCoreApp failures looks like a missing TraceEvent subscription. @safern do you have some time to help me with this? I believe we need to invest some time to triage the issues and speak with the VSTest team (cc @nohwnd) to see what we can do about the failures. |
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| function BuildAndTestBinary | |||
There was a problem hiding this comment.
no idea. I don't think anyone. Maybe we should delete it...
There was a problem hiding this comment.
Seems it was used to help stablize networking tests: dotnet/corefx#7937
@dotnet/ncl does your team use this script?
There was a problem hiding this comment.
@ManickaP @scalablecory is the above handle not maintained by your team? Can you answer above's question about who uses this script?
There was a problem hiding this comment.
Personally, I don't think so, it seems to have been used manually and maybe by some old pipeline.
@wfurt have you ever used this?
|
Might be an amateur question, but will it be possible to debug library tests with VSCode after this change? Couldn't figure out how to make it work with |
|
I believe with dotnet test you need to set an environment variable for the dotnet test host to wait for you to attach. It would print the PID in the console.
However you can also use the VSCode test extension to debug a specific test or collection of tests. |
| @@ -0,0 +1,37 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
It is confusing to have wasm under mobile. wasm != mobile. I would call the directory custom or something like that.
There was a problem hiding this comment.
I wondered about that as well and @akoeplinger told me that wasm is declared as mobile:
runtime/eng/Configurations.props
Line 68 in a109254
There was a problem hiding this comment.
What is the definition of "mobile" in plain English that we are using in this context?
I see TargetsMobile used for multiple different meanings that happen to work today, but that are not robust against changes in the platforms matrix. Unless there is non-ambiguous definition of what this means, we should not be promoting use of this name throughout the tree.
There was a problem hiding this comment.
Maybe we can open an issue for this and bring that discussion there? Changing this on the current PR would make the PR diff way bigger than it is as we already use the TargetsMobile property in MSBuild across multiple places in the repo.
There was a problem hiding this comment.
The usage of the term "mobile" traces back a few years to mono/mono where we essentially had "desktop" and "mobile" for turning on/off certain runtime features and when wasm was brought up it was closer to the existing "mobile" config :)
In dotnet/runtime we essentially mean all the targets that are not using the traditional desktop dotnet hosting and instead embed the (mono) runtime in a special way. I agree it's not totally clear though, happy for suggestions.
There was a problem hiding this comment.
Maybe we can open an issue for this and bring that discussion there?
Fine with me. This PR is renaming a number of files to mobile. Should that rename be undone until we agree on the right naming? (Undoing the rename should make the PR smaller.)
|
@ViktorHofer what are we going to do regarding: #39923 (comment) I just grepped locally and we have over 1200 usages of |
Yeah this is going to be confusing. In addition to docs we should also print a message about the option to enable console output in RunTests.sh/cmd to raise visibility. |
|
Just curious, I know that opposite is true; using System;
using Xunit;
public class UnitTest1
{
[Fact]
public void Test1()
{
Console.WriteLine("foo");
Assert.True(true); // foo is also printed if it comes after the passing assert.
}
} |
|
I have much the same perspective as @stephentoub. It seems like xunit poorly satisfies either need - the dev productivity and the platform bringup. As @stephentoub points out, it has fat dependencies. We can do better. @jkotas I have not had the experience of bringing up a new platform (and would be interested to learn at some point about how it is typically done). Would an approach like StaticTestGenerator (that can run, say., 90%+ of the tests in a rather inflexible way, but with minimal run-time dependencies) be an improvement over xunit for you for that purpose? Even if so, I think we want to continue to reduce dependencies of vstest. We do not want to find ourselves in a position where we have got a bug in the Http stack but we cannot run the tests to help root cause it. StaticTestGenerator would certainly not be acceptable for everyday dev work, even temporarily. If we can make the super-minimal solution good enough (and protected in CI), and trim vstest some more, perhaps we would then collectively feel confident enough to delete the xunit.console paths. BTW, speaking generally, this vstest initiative has been in progress for a long time -- it is core to our test diagnostics effort @ViktorHofer and myself have been running. I simply did not have sufficient imagination to realize that the runtime devs would have an interest in this or I would have brought more folks in. In future, I will be sure to consider that aspect. |
Why not? I kind of like the StaticTestGenerator approach. |
FWIW, we used closed source equivalent of the static test generator for everyday during .NET Native development. It worked quite nicely. |
Hmm, fair to challenge this. If the cost of doing the generation step is amortized, and we make it easy to run on a stable runtime, and it is not too slow (includes parallelism?) -- and it doesn't skip too many tests (because they are too difficult to support?) -- and you can work without Test Explorer (?) -- perhaps it could be. I like simple things as they are easy to reason about. I have enjoyed using simple hackable runners in the past. I am happy to commit to working towards making it that easy, if we follow this plan. That reduces the vstest risk somewhat. |
I do not see where the inflexibility comes in. I think it is not that hard to push the static runner to 100%, by either restricting what you can do in theories or by extending how the generator works. Yes, I think it would be an improvement. I would enable us to work towards using the same system to run majority of the core runtime tests too (src/tests). |
|
@ericstj do you have any thoughts to add? Does this two-pronged two-phase approach seem reasonable? |
|
One could imagine writing it as a source-generator that ran during the unit test build to automatically generate runners for optional use. But I assume (I really do not know) that it would be a bunch of work to get a good VS experience -- it would not be a good use of time when we can use our own product for this that another team maintains for us. |
I think it is perfectly fine to have vstest and test explorer with all its bells and whistles as an option for people that it works well for. |
Yes, this seems reasonable and I support it. I think using VSTest is a marked improvement over the unsupported xunit.console and a step in the right direction. I'm also supportive of investing in the lighter-weight runner so long as we can make sure we don't give up features we gain by using VSTest, nor take on too much engineering burden ourselves for a non-shipping component. What does the VSTest team think about a source-generated approach? I'd be curious to understand if we could incubate something that could make its way back into the product. I don't see those concerns as blocking this PR, but in support of a "two-pronged" approach as you mention. |
|
The heavy weight runner makes the tests run about 3x longer to run for me. Some data (Windows x64, Current master: This change: I am not sure what's the right way to run just the tests without the msbuild overhead with this change. There are no easy to follow steps printed like with the current runner. It is safe to say that it takes several times longer. EDIT: The data above are not accurate. I have run different version of the tests. The question still remains. I am hearing that we are hitting the capacity limits and being asked to reduce how much capacity we need. Do we understand the impact of the heavy weight runner on the capacity required by the test runs? |
|
@jkotas, here's what worked for me: I found this by running Old runner: New runner Looks to me like it's about the same amount of time. |
|
What Eric said.
|
|
The time reported by the test runner is net time to run the tests. It does not include the test runner overhead. We need to be looking at the wall-clock time of the whole invocation. I see the wall-clock time with the heavy weight runner is substantially more with non-Release runtime and/or libraries. Thanks for the
The net time to run the test is same (9s) in both cases. It makes sense since the exact same tests run in both cases. However, the total time is a lot more for the heavy weight runner (16s vs. 29s). Can we estimate what is the total machine time for a typical dotnet/runtime PR test mix today and how much is it going to increase with this change? |
|
@ViktorHofer, we had problems previously with dotnet test where it was doing pre-enumeration of tests and that was contributing significantly to the overhead of running tests (e.g. a suite that should have take 3 seconds instead taking 15 seconds). We fixed that with #35837, but is that fix still applying with all of your changes? |
Yes, that fix is still in branch, pre-enumerations aren't happening. The numbers above don't match as the the first one invokes xunit.console directly where-as the second invocation goes through a csproj which then invokes the VSTest target which then invokes the runner. I agree that VSTest could be faster during start-up (and unrelated, it logs too much) and would encourage someone to open an issue in https://github.com/microsoft/vstest, because talking about this will benefit not just us but our customers and their CI pipelines as well.
If you are talking about invoking all test assemblies then the numbers should be more close together as we invoke test assemblies in parallel which means the start-up cost should be less visible. If we would want to optimize for that we could even make this much faster than with xunit.console as VSTest supports invoking multiple test assemblies together. |
This machine capacity still needs to come from somewhere. My question is: How many extra machines we are going to need to provision for Helix, to process the same amount of test jobs we are processing today? |
I assumed you were talking about local invocation of test assemblies. On Helix we invoke the test assembly directly (without msbuild in play), hence the cost is much lower than your numbers above. |
I disagree. The msbuild overhead is about 1 second in my number above by watching the screen scroll. If you can tell me the command line to invoke the test assembly directly just like we do it in Helix, I will be happy to run it to get the exact number. |
|
Closing this since work is not active here. We will definitely do this but we only want active PR's to be open. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #37953
Fixes #35752
Unblocks #960
Also moving code that isn't specific to xunit out of the xunit.props and xunit.targets files. I considered grouping not just by the test framework (xunit, nunit, mstest, etc.) but also by the runner (vstest vs custom). I didn't do that as I hope that we will be able to use VSTest for mobile targets at some point as well.
Additionally I'm enabling test hang dump support.