-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
tests: parallelize all the tests #11009
Conversation
This comment has been minimized.
This comment has been minimized.
- run: yarn type-check | ||
- run: yarn lint | ||
- run: yarn test-lantern | ||
- run: yarn test-legacy-javascript |
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.
lantern can take 1m
legacy js can take 3m
idk about lhci dogfood.
so if we want to keep it under 10 minutes always we should move these to their own job.
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 can dig it
@@ -123,18 +123,19 @@ jobs: | |||
with: | |||
node-version: 10.x | |||
|
|||
- name: Define ToT chrome path | |||
if: matrix.chrome-channel == 'ToT' | |||
run: echo "::set-env name=CHROME_PATH::/home/runner/chrome-linux-tot/chrome" |
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 think set-env only applies to the next step.
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 the wording is stupid https://help.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-environment-variable
Creates or updates an environment variable for any actions running next in a job.
"Next", you say...
… The action that creates or updates the environment variable does not have access to the new value, but all subsequent actions in a job will have access.
so yeah. it's all subsequent.
also i hate how this doc says "actions" but should say "steps".
tested it earlier: https://github.com/GoogleChrome/lighthouse/runs/805258701?check_suite_focus=true
I read it two steps after i set it.
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.
ah good catch.
cc @github lol
fail-fast: false | ||
env: | ||
# The smokehouse tests run by job `smoke_0`. `smoke_1` will run the rest. | ||
SMOKE_GROUP_1: a11y oopif pwa pwa2 pwa3 dbw redirects errors offline |
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.
for future reference this can be split up further with one smoke test running $SMOKE_GROUP_1
, one running $SMOKE_GROUP_2
and a third running --invert-match $SMOKE_GROUP_1 $SMOKE_GROUP_2
(and so on with further divisions) but will need to make the matrix dynamically
env: | ||
# The smokehouse tests run by job `smoke_0`. `smoke_1` will run the rest. | ||
SMOKE_GROUP_1: a11y oopif pwa pwa2 pwa3 dbw redirects errors offline | ||
name: smoke_${{ strategy.job-index }}_${{ matrix.chrome-channel }} |
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.
these are ugly. Where's the craftsmanship? :P
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.
what were you thinking?
btw really glad you found this name
prop tho.. since we need these to be deterministically named for branch protection.
|
||
# A few steps are duplicated across all jobs. Can be done better when this feature lands: | ||
# https://github.community/t/reusing-sharing-inheriting-steps-between-jobs-declarations/16851 | ||
# https://github.com/actions/runner/issues/438 | ||
steps: | ||
- name: git clone | ||
uses: actions/checkout@v2 |
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.
faster to with: fetch-depth: 100
at this point instead of splitting it with the fetch
below?
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.
thx. i'll give that a shot.
@googlebot I consent. |
make sure to wait for #11025 |
Co-authored-by: connorjclark and brendankenny
Co-authored-by: connorjclark and brendankenny
This PR merges both @connorjclark's #10988 and @brendankenny's #10993. And then I tweaked the work balance of the non-smoke stuff.
smoke stuff
3 major non-smoke jobs:
basics
+unit
misc
back intobuild
build-all
in bothbasics
andunit
.The last two runs of this branch finished in 6:16 and 6:08, which is 2 and 6 min faster than the other two PRs.