Skip to content

Parallelise build, lint, and test actions#128

Merged
Mrtenz merged 7 commits into
mainfrom
mrtenz/parallelise-actions
Nov 1, 2022
Merged

Parallelise build, lint, and test actions#128
Mrtenz merged 7 commits into
mainfrom
mrtenz/parallelise-actions

Conversation

@Mrtenz
Copy link
Copy Markdown
Member

@Mrtenz Mrtenz commented Oct 24, 2022

Inspired by MetaMask/core#941, this parallelises the build, lint, and test actions. This hypothetically speeds up CI.

@Mrtenz Mrtenz marked this pull request as ready for review October 24, 2022 13:04
@Mrtenz Mrtenz requested a review from a team as a code owner October 24, 2022 13:04
Comment thread .github/workflows/build-test.yml
Comment thread .github/workflows/build-test.yml
@Mrtenz Mrtenz force-pushed the mrtenz/parallelise-actions branch from 7ca8346 to f573b8a Compare October 25, 2022 16:00
Copy link
Copy Markdown
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

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

This LGTM, but I would appreciate if another engineer could take a look too

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Nov 1, 2022

I've been re-running this and the current main commit on CI to get a sense of how quickly it runs. There is a lot of variance, but I think this PR is slower (maybe 20% slower on average).

This repository is sortof a worse case though, because the actual build/lint/test steps are incredibly short here because of the lack of code, so the overhead of additional CI environments is greater. I would expect this to eventually be faster as the time to build/lint/test increases. There are some parts of the overhead that scale as well (the dependency caching gets slower with more dependencies, and the checkout gets slower as the repository grows), but most overhead is static.

Comment thread .github/workflows/build-test.yml
YARN_VERSION: ${{ steps.yarn-version.outputs.YARN_VERSION }}
strategy:
matrix:
node-version: [14.x, 16.x, 18.x]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than use a matrix for each job, maybe it would be better to create a reusable "build-lint-test" workflow and invoke it using a matrix?

Right now, all runs have to wait for all three prepare runs to complete. But if we move this into a reusable workflow, we can start running the build/test/lint steps for the first prepare job to complete without waiting for the others. And I don't think it would cost any additional overhead either, but I'm not sure.

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Unsure on what performance gains this will get, but at the very least I'd expect it to fail faster in certain cases on larger repositories.

@Mrtenz Mrtenz merged commit 3e9f188 into main Nov 1, 2022
@Mrtenz Mrtenz deleted the mrtenz/parallelise-actions branch November 1, 2022 18:40
legobeat pushed a commit to legobeat/metamask-module-template that referenced this pull request Nov 2, 2022
* Parallelise build, lint, and test actions

* Fix install

* Fix prepare step Node.js version

* Fix job name

* Use matrix for prepare job

* Fix step name

* Fix lint issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants