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: split github workflow into multiple jobs #10988

Closed
wants to merge 24 commits into from
Closed

Conversation

connorjclark
Copy link
Collaborator

Splits into four jobs (with no dependencies):

  • build
  • unit
  • smoke
  • misc

Currently takes ~19 minutes to run our workflow. Now it takes ~10 minutes.

Also uploading dist/ as a build artifact. Even though currently there are no dependent jobs requiring the usage of artifacts, making dist/ downloadable may prove useful.

- run: yarn diff:sample-json
- run: yarn type-check
- run: yarn lint
- run: yarn test-proto # Run before unit-core because the roundtrip json is needed for proto tests.

# Run tests that require headfull Chrome.
- run: sudo apt-get install xvfb
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

build stops being a great name for this job past this point. All the xvfb stuff could be moved to a new job. a couple of them require dist so it'd need to be dependent on build and download the artifact.

smoke will take so much longer than this job so it'd ndb, I prefer to keep it like this and not split the job.

require('../../../../dist/lighthouse-dt-bundle.js');
// Add empty string to circumvent TS, otherwise a clean `dist/` folder
// would result in `yarn type-check` failing.
require('../../../../dist/lighthouse-dt-bundle.js' + '');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Running into Amdahl's law :)

I'm nervous splitting up the build and misc steps. I don't see obvious problems, but as the number of, uh, custom tests has grown, there are also a bunch of implicit dependencies (e.g. tests passing from a straight checkout but failing once yarn update:sample-json is run). yarn smoke is the limiting reagent, so we could split things up differently and still save a bunch of time (and work on untangling some of the tests a bit in the meantime).

@paulirish
Copy link
Member

looks great!

i would also like to bikeshed how to parallelize this. :) how about

  1. move all xvfb stuff and tests from build -> unit
  2. move all of misc to the start of unit
  3. i suppose you're expecting to have a separate job for the ToT chrome smokes?

@brendankenny dunno what you mean about the implicit dependencies... i'm not seeing a problem so far...

@brendankenny
Copy link
Member

brendankenny commented Jun 18, 2020

dunno what you mean about the implicit dependencies

⬇️

move all xvfb stuff and tests from build -> unit

would expose several of them, for instance :)

@brendankenny
Copy link
Member

regardless of any more juggling, it isn't going to go any faster than the smoke tests unless they're broken up. There's also some weird low hanging fruit, like why is uploading to buildtracker so slow?

@paulirish
Copy link
Member

why is uploading to buildtracker so slow?

looked into it a bit ago. it's primarily the cost of running brotli against all our bundles

@paulirish
Copy link
Member

dunno what you mean about the implicit dependencies
⬇️
move all xvfb stuff and tests from build -> unit

would expose several of them, for instance :)

ah just built file dependencies! okay great. #10994 has what i was thinkin there.

@vercel vercel bot temporarily deployed to Preview June 19, 2020 00:23 Inactive
@vercel vercel bot temporarily deployed to Preview June 19, 2020 00:25 Inactive
@connorjclark
Copy link
Collaborator Author

Merged the additional ToT smoke test and made a separate job for it.

https://github.com/GoogleChrome/lighthouse/actions/runs/140296254 logged 12m for everything

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