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.

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: run smoke tests in parallel jobs #10993

Closed
wants to merge 2 commits into from
Closed

tests: run smoke tests in parallel jobs #10993

wants to merge 2 commits into from

Conversation

brendankenny
Copy link
Member

πŸ”  πŸ”’ 🏌️ time :)

Counter proposal to #10988

@brendankenny brendankenny requested a review from a team as a code owner June 18, 2020 22:08
@brendankenny brendankenny requested review from connorjclark and removed request for a team June 18, 2020 22:08
@@ -38,19 +34,6 @@ jobs:
python -m pip install --upgrade pip
pip install protobuf==3.7.1

# Cache yarn deps. thx https://github.com/actions/cache/blob/master/examples.md#node---yarn
Copy link
Member Author

Choose a reason for hiding this comment

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

looking at the runs for https://github.com/GoogleChrome/lighthouse/pull/10988/checks?check_run_id=783384854 and running a bunch of them myself, it seems like caching only saves like 5 seconds out of ~30. Doesn't seem worth it.


- run: yarn test-lantern
- run: yarn test-legacy-javascript
- run: yarn i18n:checks
- run: yarn dogfood-lhci

- name: Upload dist/
Copy link
Member Author

Choose a reason for hiding this comment

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

kept from #10988

# e.g. if smoke 0 failes, continue with smoke 1 anyway
fail-fast: false
env:
# The smokehouse tests run by job smoke 0. smoke-1 will run the rest.
Copy link
Member

Choose a reason for hiding this comment

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

whatcha thinking with ToTChrome in the mix too?

Copy link
Member Author

@brendankenny brendankenny Jun 18, 2020

Choose a reason for hiding this comment

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

whatcha thinking with ToTChrome in the mix too?

matrix:
  smokeTestInvert: [false, true]
  chrome: [Stable, ToT]
name: smoke ${{ strategy.job-index }} (Chrome ${{ matrix.chrome }})

and then conditionally set CHROME_PATH? conditionals are awkward but that doesn't seem too bad

Copy link
Collaborator

@connorjclark connorjclark Jun 18, 2020

Choose a reason for hiding this comment

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

I broke up the ToT and parallel job PRs for a reason. Things are getting complex :)

runs-on: ubuntu-latest
strategy:
matrix:
invert: [false, true]
Copy link
Member

Choose a reason for hiding this comment

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

out here i'd name this smokeTestInvert or something

@@ -34,18 +34,19 @@ const runnerPaths = {
* Determine batches of smoketests to run, based on the `requestedIds`.
* @param {Array<Smokehouse.TestDfn>} allTestDefns
* @param {Array<string>} requestedIds
* @param {{invert: boolean}} options
Copy link
Member

Choose a reason for hiding this comment

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

grep's -v option is called --invert-match so... maybe that instead.

runs-on: ubuntu-latest
strategy:
matrix:
invert: [false, true]
Copy link
Member Author

Choose a reason for hiding this comment

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

also amusing would be

matrix:
  smoketest: [a11y, errors, oopif, pwa, pwa2, pwa3, dbw, redirects, seo, offline, byte, perf, lantern, metrics, legacy-javascript, source-maps]
# ...
- name: Run smoke test
  run yarn smoke -j=1 --retries=2 ${{ matrix.smoketest }}

and launch all the worker jobs :)

But I'm pretty sure we compete with the rest of the org for workers?

- name: yarn test-bundle
run: xvfb-run --auto-servernum yarn test-bundle
- name: yarn test-docs
run: xvfb-run --auto-servernum yarn test-docs

- run: yarn test-lantern
- run: yarn test-legacy-javascript
Copy link
Collaborator

@connorjclark connorjclark Jun 18, 2020

Choose a reason for hiding this comment

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

Some of these tests can take a long time. That's why I split up the "special" tests (legacy js, lantern, etc.) from the core unit test and the build job–with this PR all of these are still in the same job.

@connorjclark
Copy link
Collaborator

How long does it take?

@paulirish
Copy link
Member

How long does it take?

looking at https://github.com/GoogleChrome/lighthouse/actions?query=branch%3Aparsmoke (i think the time value is the end-to-end time?)

looks like it was 9min and 10-20s.. and after splitting out basics.. it's a minute faster at 8m11s.

@brendankenny
Copy link
Member Author

looks like it was 9min and 10-20s.. and after splitting out basics.. it's a minute faster at 8m11s.

It's end to end, but the parallel jobs don't always start at once. The slowest job was 7m 55s and we can improve that :)

@paulirish
Copy link
Member

It's end to end,

aye. just confirmed that, too.

const data = await fetch('https://api.github.com/repos/GoogleChrome/lighthouse/actions/workflows/1093967/runs?conclusion=success&status=completed&per_page=100').then(r => r.json());
data
  .workflow_runs
  .filter(e => e.conclusion === "success")
  .map(e => ((new Date(e.updated_at) - new Date(e.created_at)) / 1000 / 60).toLocaleString(undefined, {style: 'unit', unit: 'minute', unitDisplay: 'long'}))

image

runs-on: ubuntu-latest
strategy:
matrix:
smokeTestInvert: [false, true]
Copy link
Collaborator

Choose a reason for hiding this comment

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

yaml keys are normally skewer-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants