Build all managed coreclr tests on OSX in CI #35783

sdmaclea · 2020-05-04T05:09:33Z

Remove concept of targetGeneric and targetSpecific

Add OSX corelcr managed tests builds to all pipes
Remove all other platform coreclr managed test build

Remove property TestUnsupportedOutsideWindows
Rename target conditional DisableProjectBuild to CLRTestTargetUnsupported

Refactor tests to allow all managed tests to be built on OSX.

Split managed tests which based on target properties conditionally:

<Compile Include/>
<DefineConstant/>

This creates a separate test for each target configuration permutation.

Add <CLRTestTargetUnsupported/> conditions to select these at build time.

Add issues to issues.targets to exclude incompatible tests at run time.

Append split tests with _Target* to identify intended test target

For tests which depend on target specific internal details of System.Private.Corlib,
expose a dummy implementation for unsupported targets.

Clean up

Remove <TraitTags/>
Remove <CLRTestNeedTarget/>
Remove managedOSXBuild
Remove managedTestBuildOsGroup
Remove managedTestBuildOsSubGroup
"

ghost · 2020-05-04T05:09:46Z

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

sdmaclea · 2020-05-04T05:17:42Z

This is a fairly significant change in CoreCLR test strategy, but I think it is a big win.

It will only create one test build leg which will build all tests for all targets
It will not add a managed build step to run-test-job
It will not incur extra delay while rerunning tests
It should save 10+ minutes per target
It fixes multiple issues with missing tests
It simplifies things

All pipelines will eventually need some testing.
I triggered outerloop https://dev.azure.com/dnceng/public/_build/results?buildId=629477&view=results

trylek · 2020-05-04T18:09:31Z

Most of this looks good to me. Apart from the above pipeline failures that need investigating and fixing as they seem clearly related to your change, I have a hard time figuring out how the test split works with your change. If I'm not mistaken, you're removing the targetGeneric and targetSpecific switches from build-test, replacing them with a single flag allTargets. Can you please explain to me how is this supposed to cater for the following three different scenarios?

Building target-agnostic tests from build-test-job;
Building target-specific tests from run-test-job;
Building all tests when running build-test in local developer workflow.

Thanks

Tomas

sdmaclea · 2020-05-04T19:55:33Z

Can you please explain to me how is this supposed to cater for the following three different scenarios?

Building target-agnostic tests from build-test-job;
2 .Building target-specific tests from run-test-job;

Building all tests when running build-test in local developer workflow.

The idea is that the category targetSpecific test exists for a few reasons:

A test couldn't be built on all platforms (Windows historically, OSX currently)
A test conditionally included different source based on properties of the target
A test conditionally set DefineConstant ( based on properties of the target) and therefore affected #if code in C#

This patch eliminates the targetSpecific category, so any test can be built on any platform without changing the content or behavior of the test.

So this really changes the three scenarios. We now have

Build tests for all targets (option allTargets)
Build tests for the developer's target. (default)

The change relies on the bash/batch wrapper scripts to disable tests which do not run on the specific target

trylek · 2020-05-04T20:02:53Z

I think I'm getting the general idea but I still don't fully understand how we manage to only build platform-specific tests in run-test-job. Is the logic solely based on test build "incrementalism" that ends up not rebuilding tests that have already been built - i.o.w. we first download the target-agnostic tests and that by itself ensures that those tests won't be built again in run-test-job? If that is the case, it might be worth a comment in the yml file as it seems non-trivial to me and possible future changes to the test build process can easily break this.

sdmaclea · 2020-05-04T20:19:39Z

We are not building managed tests in run-test-job. There is a stray comment that I missed when I deleted that code. I'll fix it, but that may be the source of confusion.

So we are now building all tests for all target on OSX. If a test used to have multiple configurations, it now has a separate name for each configuration. So all tests can be built simulataneously.

We are not building any managed tests during run-test-job.

The target specific tests are being skipped in helix because of the <BashCLRTestEnvironmentCompatibilityCheck Condition="'$(CLRTestTargetUnsupported)' == 'true'"> changes to src/coreclr/tests/src/CLRTest.Execute.Bash.targets & src/coreclr/tests/src/CLRTest.Execute.Batch.targets.

trylek · 2020-05-04T20:51:03Z

OK, I finally got it, thanks for your patience with me!

BruceForstall · 2020-05-04T22:44:04Z

When you say "Build all managed coreclr tests on OSX", is that proxy for "Build all managed coreclr tests exactly once, and that happens to be macOS because our macOS build servers are the fastest ones we have available to us, and getting the build out fast unblocks subsequent parallel test run steps"?

Would this enable or make easier a dev inner-loop scenario where devs download a recent test build instead of building tests themselves (if desired)?

jashook · 2020-05-04T22:47:00Z

When you say "Build all managed coreclr tests on OSX", is that proxy for "Build all managed coreclr tests exactly once, and that happens to be macOS because our macOS build servers are the fastest ones we have available to us, and getting the build out fast unblocks subsequent parallel test run steps"?

Yes

Would this enable or make easier a dev inner-loop scenario where devs download a recent test build instead of building tests themselves (if desired)?

I do not see why not

jashook · 2020-05-04T22:48:00Z

I answered too soon. you would still need the native components of the tests. Those are built in the build job. You would need to download two artifacts

jashook · 2020-05-04T22:48:27Z

There is probably some more work required to improve the innerloop via download, but this makes things easier.

sdmaclea · 2020-05-05T21:46:21Z

Looks to me like things are now working like they are expected.

There are still some failing tests. These were likely regressions introduced because of the CI coverage outage.

Rather than continuing to amend this patch, I have put these recent regressions in #35860

sdmaclea · 2020-05-05T21:52:25Z

Also note the documentation doesn't actually represent the current implementation. To allow the PR to merge, I am not updating that here yet either.

Also if we are concerned with helix payload increases, we can look alternatives... I can think of a few....

naricc · 2020-05-05T22:09:03Z

@naricc I am just tagging my self here because I am working on making sure the runtime tests work on Mono (we already have a lane for OSX up and running) and want to follow this.

naricc · 2020-05-05T22:22:06Z

eng/pipelines/runtime.yml

@@ -610,47 +610,6 @@ jobs:
    jobParameters:
      testGroup: innerloop
      liveLibrariesBuildConfig: ${{ variables.debugOnPrReleaseOnRolling }}


I think we can incorporate the mono build-test-job in here too: https://github.com/dotnet/runtime/blob/5335f94a9747a64bd300ce06a4ce36a60f4ea71f/eng/pipelines/runtime.yml#L663

Although I can do that as a separate PR later if you like.

I would suggest adding this in a follow up pr

jashook · 2020-05-05T22:41:16Z

Adding @jkoritzinsky and @AaronRobinsonMSFT for the COM specific bits

jashook · 2020-05-05T23:27:53Z

After some discussion we have decided to revert the existing change. The rest of the work will happen in its own branch as it is clear each change will have follow up work.

jashook · 2020-05-05T23:29:05Z

#35868

AaronRobinsonMSFT

Interop tests look good to me.

Remove concept of targetGeneric and targetSpecific Add OSX corelcr managed tests builds to all pipes Remove all other platform coreclr managed test build Remove property `TestUnsupportedOutsideWindows` Rename conditional `DisableProjectBuild` to `CLRTestTargetUnsupported` Refactor tests to allow all managed tests to be built on OSX. Split managed tests which based on target properties conditionally: + `<Compile Include/>` + `<DefineConstant/>` This creates a separate test for each target configuration permutation. Add `<CLRTestTargetUnsupported/>` conditions to select these at build Revise CLRTest.Execute.Bash.targets & CLRTest.Execute.Batch.targets to disable on `<CLRTestTargetUnsupported/>`. Append split tests with `_Target*` to identify intended test target For tests which depend on target specific internal details od System.Private.Corlib, expose a dummy implementation for unsupported targets. Clean up Remove <TraitTags/> Remove <CLRTestNeedTarget/> Remove managedOSXBuild Remove managedTestBuildOsGroup Remove managedTestBuildOsSubGroup Use issues.target to disable expected failures Some tests are configured to run on a targets with specific charecteristics. Use issues.targets to conditionally disable tests not intended to run on all targets.

sdmaclea · 2020-05-06T06:40:27Z

I just squashed and rebased on the origin/master after the #35868 revert.

sdmaclea · 2020-05-06T06:51:12Z

Design choices....

I wanted to build all tests in one place because it appeared it would essentially be free in terms of execution time.
The execution cost of building the targetSpecific tests seemed to be grossly out of proportion with the number of tests to build.
When I drafted this patch I hadn't realized the scripts associated with each test was built with the test. That meant target specific script changes would be based on OSX, not on the target.
Urgency led me to solve this with issues.targets

Open issues:

This PR affects every coreclr pipeline only inner and outloops have been tested

Open issues (deferrable):

The documentation associated with this change is slightly out of date.
issues.targets
- Long term it would be ideal for a developer to be able to download the test artifacts and have them just work. In part that is a separate feature, but the script not properly disabling test is a blocking this feature
- The disabling of tests which are not intended to run on specific targets using issues.targets is misleading. We need a better solution
- I believe the script issue can be solved it just requires some design discussion.
- Possibly even a requirements discussion

sdmaclea · 2020-05-06T17:27:13Z

My current design thought is to leverage the --subset work to solve the test selection.

The project disable code would change to ...

<!-- Test unsupported outside of windows -->
<CLRTestTargetUnsupported>+Windows+</CLRTestTargetUnsupported>

<!-- Test unsupported outside of windows -->
<!-- IJW is not supported on ARM64 -->
<CLRTestTargetUnsupported>+Windows+arm64+</CLRTestTargetUnsupported>

The build of projects would be disabled based on <CLRTestTargetUnsupported/> unless built with allTargets.

The script targets would get added code to selectively disable the script

<BashCLRTestEnvironmentCompatibilityCheck Condition="'$(CLRTestTargetUnsupported.Contains('+arm64+)">

For OS specific disables, we would rely on

Batch == Windows
Bash == Unix, we would use uname -s to get the kernel name Linux, Darwin ... for more fine grained disables

For Arch specific disables

We could use an environment variable COMPlus_TargetArch
Abort the test if the environment variable is unset

ViktorHofer · 2020-05-06T17:59:57Z

cc @safern for test selection. doesn't that sound very similar to what we are doing with xunit in libraries?

sdmaclea · 2020-05-06T19:05:55Z

I started drafting the disable code. For reference this is the set of targetSpecific tests and the cases in which they must be disabled.

$ git grep -h CLRTestTargetUnsupported | sort | uniq -c | sort -n -r
    224     <CLRTestTargetUnsupported>+Unix+</CLRTestTargetUnsupported>
     20     <CLRTestTargetUnsupported>+arm+x86+</CLRTestTargetUnsupported>
     16     <CLRTestTargetUnsupported>+arm64+x64+</CLRTestTargetUnsupported>
      7     <CLRTestTargetUnsupported>+Unix+arm64+</CLRTestTargetUnsupported>
      4     <CLRTestTargetUnsupported>+Windows+</CLRTestTargetUnsupported>
      4     <CLRTestTargetUnsupported>+arm64+x64+x86+</CLRTestTargetUnsupported>
      4     <CLRTestTargetUnsupported>+arm64+x64+arm+</CLRTestTargetUnsupported>
      2     <CLRTestTargetUnsupported>+arm+</CLRTestTargetUnsupported>
      2     <CLRTestTargetUnsupported>+arm64+arm+x86+</CLRTestTargetUnsupported>
      1     <CLRTestTargetUnsupported>+Unix+arm+</CLRTestTargetUnsupported>
      1     <CLRTestTargetUnsupported>+arm64+</CLRTestTargetUnsupported>

sdmaclea · 2020-05-11T23:54:26Z

Merged to the dev/infrastructure branch as #36253

sdmaclea requested review from jashook, trylek, hoyosjs and safern May 4, 2020 05:09

Dotnet-GitSync-Bot added the area-Infrastructure label May 4, 2020

sdmaclea added area-Infrastructure-coreclr and removed area-Infrastructure labels May 4, 2020

sdmaclea self-assigned this May 4, 2020

sdmaclea force-pushed the OsxBuildsAllTargets branch from fc440bc to 29a9d8e Compare May 4, 2020 16:15

This was referenced May 4, 2020

Stack overflow while building NestedGeneric tests #35796

Closed

Most CoreCLR tests not running in CI. #35798

Closed

This was referenced May 5, 2020

Fix target specific download to not clean the unpack dir #35699

Closed

Remove CoreCLR TargetSpecific test build jobs #35645

Closed

sdmaclea force-pushed the OsxBuildsAllTargets branch 3 times, most recently from 5849d6c to 648e2ff Compare May 5, 2020 06:41

This was referenced May 5, 2020

Errors installing the SDK during builds #34015

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

sdmaclea force-pushed the OsxBuildsAllTargets branch from 648e2ff to 3791f79 Compare May 5, 2020 15:26

sdmaclea force-pushed the OsxBuildsAllTargets branch from 3791f79 to 5335f94 Compare May 5, 2020 18:10

sdmaclea mentioned this pull request May 5, 2020

Disable tests failing apparently due to coverage outage regressions #35860

Closed

naricc reviewed May 5, 2020

View reviewed changes

sdmaclea mentioned this pull request May 5, 2020

Refactor Build All OSX tests to reduce redundant data entry. #35865

Closed

jashook requested review from AaronRobinsonMSFT and jkoritzinsky May 5, 2020 22:41

sdmaclea mentioned this pull request May 5, 2020

Revert "Restore Seperate test builds, but fix broken pipe (#35378)" #35868

Merged

AaronRobinsonMSFT approved these changes May 6, 2020

View reviewed changes

sdmaclea force-pushed the OsxBuildsAllTargets branch from 5335f94 to 3264204 Compare May 6, 2020 06:33

Merge remote-tracking branch 'origin/master' into OsxBuildsAllTargets

9d52535

safern mentioned this pull request May 6, 2020

Test failure: Assertion failed '(jitIntrinsic->gtSIMDSize == 12) || (jitIntrinsic->gtSIMDSize == 16)' #35877

Closed

safern mentioned this pull request May 7, 2020

Enable Common Runtime Tests for Mono on Linux-Arm64 in CI #35366

Merged

naricc mentioned this pull request May 7, 2020

Remove extra runtime test build step from mono runtime test runs #35979

Closed

sdmaclea closed this May 11, 2020

sdmaclea deleted the OsxBuildsAllTargets branch May 11, 2020 23:55

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build all managed coreclr tests on OSX in CI #35783

Build all managed coreclr tests on OSX in CI #35783

sdmaclea commented May 4, 2020 •

edited

Loading

ghost commented May 4, 2020

sdmaclea commented May 4, 2020 •

edited

Loading

trylek commented May 4, 2020

sdmaclea commented May 4, 2020 •

edited

Loading

trylek commented May 4, 2020

sdmaclea commented May 4, 2020

trylek commented May 4, 2020

BruceForstall commented May 4, 2020

jashook commented May 4, 2020

jashook commented May 4, 2020

jashook commented May 4, 2020

sdmaclea commented May 5, 2020

sdmaclea commented May 5, 2020

naricc commented May 5, 2020

naricc May 5, 2020

jashook May 5, 2020

jashook commented May 5, 2020

jashook commented May 5, 2020

jashook commented May 5, 2020

AaronRobinsonMSFT left a comment

sdmaclea commented May 6, 2020

sdmaclea commented May 6, 2020

sdmaclea commented May 6, 2020

ViktorHofer commented May 6, 2020

sdmaclea commented May 6, 2020

sdmaclea commented May 11, 2020

Build all managed coreclr tests on OSX in CI #35783

Build all managed coreclr tests on OSX in CI #35783

Conversation

sdmaclea commented May 4, 2020 • edited Loading

ghost commented May 4, 2020

sdmaclea commented May 4, 2020 • edited Loading

trylek commented May 4, 2020

sdmaclea commented May 4, 2020 • edited Loading

trylek commented May 4, 2020

sdmaclea commented May 4, 2020

trylek commented May 4, 2020

BruceForstall commented May 4, 2020

jashook commented May 4, 2020

jashook commented May 4, 2020

jashook commented May 4, 2020

sdmaclea commented May 5, 2020

sdmaclea commented May 5, 2020

naricc commented May 5, 2020

naricc May 5, 2020

Choose a reason for hiding this comment

jashook May 5, 2020

Choose a reason for hiding this comment

jashook commented May 5, 2020

jashook commented May 5, 2020

jashook commented May 5, 2020

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

sdmaclea commented May 6, 2020

sdmaclea commented May 6, 2020

sdmaclea commented May 6, 2020

ViktorHofer commented May 6, 2020

sdmaclea commented May 6, 2020

sdmaclea commented May 11, 2020

sdmaclea commented May 4, 2020 •

edited

Loading

sdmaclea commented May 4, 2020 •

edited

Loading

sdmaclea commented May 4, 2020 •

edited

Loading