Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable some browser legs on the extra-platforms pipeline #64065

Merged
merged 3 commits into from
Jan 21, 2022
Merged

Enable some browser legs on the extra-platforms pipeline #64065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a0d981
Enable some browser legs on the extra-platforms pipeline
safern Jan 20, 2022
6f0a1be
Flow platform parameter from helix queues templates
safern Jan 20, 2022
d9b409b
Fix another condition
safern Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eng/pipelines/common/global-build-job.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ jobs:
df -h
displayName: Disk Usage before Build

- ${{ if contains(parameters.nameSuffix, 'Windows_wasm') }}:
- ${{ if eq(parameters.platform, 'Browser_wasm_win') }}:
# Update machine certs
- task: PowerShell@2
displayName: Update machine certs
Expand Down
15 changes: 5 additions & 10 deletions eng/pipelines/runtime-extra-platforms.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,7 @@ jobs:
platforms:
# BuildWasmApps should only happen on the extra platforms build as we already have coverage for this build on PRs.
- Browser_wasm
# disable until https://github.com/dotnet/runtime/issues/63987 is fixed
# - Browser_wasm_win
- Browser_wasm_win
variables:
# map dependencies variables to local variables
- name: wasmbuildtestsContainsChange
Expand Down Expand Up @@ -169,8 +168,7 @@ jobs:
buildConfig: release
runtimeFlavor: mono
platforms:
# disable until https://github.com/dotnet/runtime/issues/63987 is fixed
# - Browser_wasm_win
- Browser_wasm_win
variables:
# map dependencies variables to local variables
- name: librariesContainsChange
Expand Down Expand Up @@ -257,8 +255,7 @@ jobs:
runtimeFlavor: mono
platforms:
- Browser_wasm
# disable until https://github.com/dotnet/runtime/issues/63987 is fixed
# - Browser_wasm_win
- Browser_wasm_win
variables:
# map dependencies variables to local variables
- name: librariesContainsChange
Expand Down Expand Up @@ -541,8 +538,7 @@ jobs:
condition: >-
or(
eq(dependencies.evaluate_paths.outputs['SetPathVars_wasmdebuggertests.containsChange'], true),
eq(variables['isManualOrIsNotPR'], true),
eq(variables['isFullMatrix'], true))
eq(variables['isRollingBuild'], true))
Comment on lines -544 to +541
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mean that when runtime-extra-platforms is being run, then it will run only if the changes were found. But isn't the expectation that when running this pipeline manually, everything should run?
Some of the other jobs in this file also seem to have the same issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would expect at least the configurations that don't run full tests on PR's to run all when manually triggered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is now not longer a mono specific pipeline like the runtime-manual was, we are still using the evaluate-paths logic for PRs but scheduled will run all.

However, we can adjust these conditions as convenient, for example instead of depending on the wasm debugger path subset, we can just make this less granular and depend on any mono change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That kinda defeats the purpose of running it manually, IMHO. IIUC, for PRs we run a limited set by default, like smoke tests for libraries. And we had added this separate pipeline to allow running all the tests, so we could check in the PR if anything broke. Otherwise, we would find out only on a rolling/scheduled build.

Please correct me if I'm wrong.

Copy link
Member Author

@safern safern Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm not being clear. If you change a path that applies to any of the needed tests, it will run all the tests, but only if you changed paths that need those tests. Inside this pipeline smoke tests will never run. You can confirm that by looking at this condition:

runtime/eng/pipelines/common/xplat-setup.yml

Line 52 in 97fec41

value: '/p:RunSmokeTestsOnly=$(isNotExtraPlatformsBuild)'

and

runtime/eng/pipelines/common/variables.yml

Line 18 in 97fec41

- name: isNotExtraPlatformsBuild

However, the mono tests will only run if you changed libraries or mono subset for example. Or the wasm debugger tests will only run if you changed the wasm debugger path subset.

If the PR contains changes to runtime tests then the runtime tests will run, etc. Same as the main PR pipeline works with the evaluate changed paths, but this pipeline will run all the tests, not smoke tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that's why it should happen only for rolling builds, or when run manually.

But we only have either rolling or manual trigger with this pipeline. We don't have per PR run by default, only if invoked manually.

Can we split the wasm ones from this pipeline then? Mobile ones probably also want to run similarly. Here are some examples of cases where issues got caught in the rolling build instead of the PR itself, thus affecting many more people than just the PR author.

We have been using the evaluate-changed-paths ever since the repo was created and we have found very fue issues because of that but a huge gain on resource saving. I don't think we should throw in and create a pipeline for mono and one for non mono, that will cause more and more confusion to developers. Running based on changed paths is the same as if it were part of the main PR pipeline, even the smoke tests run only if mono or libraries are changed.

Rather than throwing more pipelines into the equation I would just update the legs that depend on wasm debugger or build browser paths, to instead depend on mono paths + debugger paths/build browser paths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we only have either rolling or manual trigger with this pipeline. We don't have per PR run by default, only if invoked manually.

Oh, then it would mean that the browser/windows jobs will run only on rolling builds, or manual runs. Was that an oversight?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would expect at least the configurations that don't run full tests on PR's to run all when manually triggered.

I missed we were setting evaluate paths. When triggered manually, we would be getting the same experience before we started running smoke tests only. That should be acceptable for most cases.

For changes that cause no legs to run, I think we either do what santi suggested and adjust specific paths or there is always the option to trigger manually from azdo. You can set any yml variable and force a full run if you wanted to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, then it would mean that the browser/windows jobs will run only on rolling builds, or manual runs. Was that an oversight?

They will run on manual runs or scheduled runs for the extra-platforms pipeline. If we want them to run smoke tests on the main runtime pipeline, then we can do that as well.

For changes that cause no legs to run, I think we either do what santi suggested and adjust specific paths or there is always the option to trigger manually from azdo. You can set any yml variable and force a full run if you wanted to.

Yeah, however if you set the mono subsets path + the wasm debugger and the build browser paths, I think it will be very very few cases where you will need to run it manually from the pipeline UI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want them to run smoke tests on the main runtime pipeline, then we can do that as well

Yeah, runtime-staging. I can open a PR for that

# extra steps, run tests
extraStepsTemplate: /eng/pipelines/libraries/helix.yml
extraStepsParameters:
Expand All @@ -559,8 +555,7 @@ jobs:
buildConfig: Release
runtimeFlavor: mono
platforms:
# disable until https://github.com/dotnet/runtime/issues/63987 is fixed
# - Browser_wasm_win
- Browser_wasm_win
variables:
# map dependencies variables to local variables
- name: wasmdebuggertestsContainsChange
Expand Down