-
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
Alter amount of tests that run per PR for mono mobile configurations #60727
Conversation
In an effort to better utilize CI resources, this change will only run the System.Runtime library test per PR for Android and Wasm configurations. The full libraries and runtime tests will move to only run on the post-PR validation pipeline. Note: iOS/tvOS/MacCatalyst are excluded until dotnet#59503 lands
Tagging subscribers to this area: @directhex Issue DetailsIn an effort to better utilize CI resources, this change will only run the System.Runtime library test per PR for Android and Wasm configurations. The full libraries and runtime tests will move to only run on the post-PR validation pipeline. Note: iOS/tvOS/MacCatalyst are excluded until #59503 lands
|
This will cause a lot of regressions to be discovered post PR. |
eng/pipelines/runtime.yml
Outdated
@@ -323,7 +323,9 @@ jobs: | |||
buildConfig: Release | |||
runtimeFlavor: mono | |||
platforms: | |||
- Browser_wasm | |||
# BuildWasmApps should only happen on the rolling build. No need to duplicate the build on PR's | |||
${{ if eq(variables['isFullMatrix'], true) }}: |
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.
Maybe instead of this, we can have tighter constraints on which changes trigger running Wasm.Build.Tests? Eg. src/mono
, src/tests/BuildWasmApps
, src/tasks
, etc? though it will get kinda detailed, because we would want to include the various nugets consumed for workloads.
Just fyi, this job does not run the library tests as we pass TestAssemblies=false
, so it builds, and then runs only the Wasm.Build.Tests .
cc @lewing
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.
I'd rather drop Scenario='normal' from Browser_wasm and keep EAT with Scenario='normal', I think this is actually a little beyond guidance and it would catch a lot of test configuation changes that would otherwise cause us problems.
eng/pipelines/runtime.yml
Outdated
@@ -415,7 +417,7 @@ jobs: | |||
jobParameters: | |||
testGroup: innerloop | |||
nameSuffix: AllSubsets_Mono_AOT | |||
buildArgs: -s mono+libs+host+packs+libs.tests -c $(_BuildConfig) /p:ArchiveTests=true /p:EnableAggressiveTrimming=true /p:BuildAOTTestsOnHelix=true /p:RunAOTCompilation=true | |||
buildArgs: -s mono+libs+host+packs+libs.tests -c $(_BuildConfig) $(_runSmokeTestsOnlyArg) /p:ArchiveTests=true /p:EnableAggressiveTrimming=true /p:BuildAOTTestsOnHelix=true /p:RunAOTCompilation=true |
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.
If we don't want to run EAT/AOT tests for every PR, then it would be useful to have some way to opt-in, or trigger these builds on a PR.
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.
Yeah, we need a way to trigger aot manually for sure, it would be nice to auto trigger it with any src/mono/mono change as well.
I feel the same as Larry. Are we experiencing any machine shortage or PR running for too long? |
I just thought about it again. Maybe it is okay, since you are only disabling runs on Android. (I am not sure how much it would impact WASM, since WASM is actively experiencing big changes.) The catch is that we will need to keep close eye on the rolling builds. |
There has been some offline discussion where I think it makes sense to pull this back for the time being. I plan on resubmitting soon. |
If we have the ability to select particular libraries to test pre commit, we could fault in more as and when we see regressions leak. |
dd96e90
to
f601e38
Compare
f601e38
to
d01b7b5
Compare
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.
@steveisok should we have an issue to track the future adjustments/refinements? |
In an effort to better utilize CI resources, this change will only run the System.Runtime library test per PR for Android and Wasm configurations. The full libraries and runtime tests will move to only run on the post-PR validation pipeline.
The summary of changes are:
Note: iOS/tvOS/MacCatalyst are excluded until #59503 lands