Change corefx test process#16263
Conversation
Instead of using the `/p:CoreCLROverridePath` switch when building corefx, to bring in coreclr, build corefx normally. This builds against the coreclr packages that corefx has already been validated against in the CI, so the build should always work. Then, copy over our built coreclr bits before testing. This method is expected to have better behavior in most breaking change scenarios.
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
Once this is merged, could you please submit a test PR that introduces a break e.g. in some rarely used reflection API and verify that the corefx legs will fail as expected? |
| # We used to pass the argument: | ||
| # -- /p:CoreCLROverridePath=<path-to-core_root> | ||
| # which causes the corefx build to overwrite its built core_root with the binaries from | ||
| # the coreclr build. However, this often causes build failures which breaking changes are |
| # the coreclr build. However, this often causes build failures which breaking changes are | ||
| # in progress (e.g., a breaking change is made in coreclr that has not yet had compensating | ||
| # changes made in the corefx repo). Instead, build corefx normally. This should always work | ||
| # since corefx is protected by a CI testing system. Then, overwrite the built corex |
| # Override the built corefx runtime (which it picked up by copying from packages determined | ||
| # by its dependencies.props file). Note that we always build Release corefx. | ||
| # TODO: it might be cleaner to encapsulate the knowledge of how to do this in the | ||
| # corefx msbuild files somewhere. |
There was a problem hiding this comment.
👍 Could you open an issue in CoreFx explaining our scenario and see if you can get someone to add something like CoreCLROverridePath that only uses the overriding runtime for running, not building?
There was a problem hiding this comment.
@jkotas Like removing an API? Can you suggest one? Note that my runs using the current (old) script fail to build due to dotnet/corefx#26933 (that is, until that PR is merged) |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
We still need a way to disable tests when running corefx tests using this script/system, in the CI. The corefx tests are invoked with the "build-tests" script in corefx, which eventually invokes xunit, something like: We can't touch the corefx tests, since we want the disabling to happen purely on the coreclr side. That would imply some kind of "exclusion file". xunit doesn't have such a mechanism now (and can we do work to add one?). Possibly we could add a simple per-assembly exclusion list by adding magic to the corefx generated RunTests.cmd wrappers to check, "Am I disabled?" and skipping if so. I've gotten lots of pushback before adding things to this file. Or, maybe the wrapper generator itself could look up exclusions based on a provided file (kind of like how the coreclr issues.targets file works?) |
xunit driver that we are using to run CoreFX tests is our own (https://github.com/dotnet/buildtools/tree/master/src/xunit.console.netcore), so we can presumably teach it to have exlusion list command line option. Even better would be a build-time xunit driver optimized for runtime developers that does not discover tests using reflection. It would make things way a lot easier to debug. |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
AssemblyName.SetPublicToken |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
@jkotas I'm seeing ~3500 XML-related corefx test failures here, but it turns out they're also in the baseline (without this change to the corefx test process), e.g.: or the cron job: It isn't just Ubuntu; happens on Windows, too. Have you seen this? |
|
Seems like lots of errors are of the form: and I can't find those input files on my machine, either. |
|
btw, the console output on Windows is practically unintelligible due to interleaving multiple xunit invocations. Is that because msbuild is doing the invoking and running multiple threads? Is there a way to better serialize the output? (Does it actually make sense for msbuild to be parallelizing while xunit is also parallelizing?) |
These files are created by Do you see the same failure when you run it locally?
I expect that you would see pretty measurable slowdown if you disabled msbuild level parallelization. A good chunk of xunit process runs single threaded. |
|
It looks like a recent regression: dotnet/corefx#26940 (comment) |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
The XML failures are gone. The only remaining one is System.Tests.MathFTests:Cbrt. |
|
fyi, the failure is tracked as https://github.com/dotnet/coreclr/issues/16272 |
| # TODO: it might be cleaner to encapsulate the knowledge of how to do this in the | ||
| # corefx msbuild files somewhere. | ||
|
|
||
| fx_runtime = os.path.join(fx_root, |
There was a problem hiding this comment.
I don't think this is a good idea. It will likely break whenever we change the layout of corefx output. We don't want to be responsible for updating random scripts like this that are taking a hard-coded dependency on the output of corefx repo.
If the options of CoreCLROverridePath isn't doing what you need it to then we should fix that or provide another public hook instead of depending on these paths here.
There was a problem hiding this comment.
Agreed; hence https://github.com/dotnet/corefx/issues/26944
4 participants
Instead of using the
/p:CoreCLROverridePathswitch when buildingcorefx, to bring in coreclr, build corefx normally. This builds
against the coreclr packages that corefx has already been validated
against in the CI, so the build should always work. Then, copy
over our built coreclr bits before testing. This method is expected
to have better behavior in most breaking change scenarios.