-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Testmerging jit64 #83151
Testmerging jit64 #83151
Conversation
Before:
Now:
|
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata Issue DetailsThis another pr merging JIT tests as #81969. It starts with a commit removing args from test entrypoint, and then applies ILTransform with the arguments specified on each commit in the same directory (baseservices/threading). This is the version of the tool I used.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis another pr merging JIT tests as #81969. It starts with a commit removing args from test entrypoint, and then applies ILTransform with the arguments specified on each commit in the same directory (baseservices/threading). This is the version of the tool I used.
|
It seems I missed a few cases in issue.targets |
@markples please take care of this PR. Thanks @BrianBohe for working on this. |
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
Otherwise, a merged test group's wrapper's exit code will override any out-of-proc test in the group. This should be safe because other places that set CLRTestExpectedExitCode are either - Later in the file so will continue to override this - In src\tests\baseservices\TieredCompilation\RunBasicTestWithMcj.cmd/sh but setting/using it locally This will break setting it in the environment and overriding all occurrences, but such usage wouldn't have the expected per-test expected exit values.
/azp run runtime-coreclr outerloop, runtime-extra-platforms |
Azure Pipelines successfully started running 2 pipeline(s). |
@trylek Thanks for approving an earlier iteration of this. I've made a few additional changes that will hopefully be a quick re-review.
I also added an unrelated fix. I noticed that Regression's b16102 was testing an empty Main method, so merged test conversion lost its purpose. First undo the change and use RequiresProcessIsolation and ReferenceXUnitWrapperGenerator. Then unconditionally set CLRTestExpectedExitCode in .cmd/.sh wrappers so that the merged wrapper expected value doesn't override embedded tests. I couldn't find anything in dotnet/runtime that appears to depend on the conditional. If this requires discussion, then I can separate it out in order to merge jit64. Thanks! |
I've added a review to this since Brian started the work and I reviewed those portions, but I authored the final changes so obviously my review doesn't work for those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Mark for finishing Brian's change, your change still looks great to me.
fyi - there are a lot of existing failures, but I went through (multiple iterations for some of) the runtime, outerloop, and extra-platforms jobs and did not find anything new. |
This another pr merging JIT tests as #81969. It starts with a commit removing args from test entrypoint, and then applies ILTransform with the arguments specified on each commit in the same directory (baseservices/threading). This is the version of the tool I used.
Last two commits are a patch on the test merging approach to avoid deduplicating test classes on il tests. These are taken from #81969.