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: Shard JS unit tests #60045

Merged
merged 15 commits into from Apr 3, 2024
Merged

Tests: Shard JS unit tests #60045

merged 15 commits into from Apr 3, 2024

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Mar 20, 2024

What?

  • Split out JS date unit tests
  • Shard main JS unit tests into 4 shards.

See this comment for findings on different numbers of shards.

Anecdotally, the JavaScript unit tests run in ~2 minutes (parallelized on 4 workers * 2 node versions) instead of ~3:45 currently on trunk.

Follow up to #59904.

Why?

We can speed up the JS unit tests by sharding them — they're split up and different tests are run in parallel on different workers.

@sirreal sirreal force-pushed the try/shard-js-tests branch 2 times, most recently from 449fbcf to 094f8ae Compare March 20, 2024 15:58
@sirreal sirreal changed the title Tests: Share JS unit tests Tests: Shard JS unit tests Mar 20, 2024
@desrosj
Copy link
Contributor

desrosj commented Mar 21, 2024

A minute for each test run with shards is great. I'm curious how long the test step would be for 2 shards, though.

@sirreal
Copy link
Member Author

sirreal commented Mar 22, 2024

Yep! I'll pause this work for about a week, but before proceeding I plan to do some testing to find a sweet spot by testing different numbers of shards.

@sirreal sirreal force-pushed the try/shard-js-tests branch 2 times, most recently from f01829d to 01a190a Compare April 3, 2024 12:28
@sirreal
Copy link
Member Author

sirreal commented Apr 3, 2024

Testing runs:

Shards Clock time Accumulated time Longest shard
1 4:10 7:34 4:00
2 2:37 9:29 2:29
3 2:26 12:11 2:16
4 1:58 13:22 1:50
5 2:02 16:30 1:53
6 1:56 18:00 1:45

This is only 1 run for different shard numbers so is far from perfect but gives us an idea of where the sweet spot is.

Clock time is the time reported for the action. I think it includes time waiting for the workers to be ready.
Accumulated time is what's reported under "usage" - its the sum of the time of all the different shard runs.
Longest shard is the longest time reported for a single shard, this seems relevant because it represents a lower bound, the whole workflow can't complete any faster than its slowest individual run.

4 shards seems like a good number. It's the fastest run overall, and is not a large jump in accumulated time (occupying workers for more time that other workflows could use) from 3.

@sirreal sirreal added [Type] Code Quality Issues or PRs that relate to code quality Developer Experience Ideas about improving block and theme developer experience labels Apr 3, 2024
@sirreal sirreal marked this pull request as ready for review April 3, 2024 14:02
@sirreal sirreal requested a review from desrosj as a code owner April 3, 2024 14:02
@sirreal sirreal requested a review from gziolo April 3, 2024 14:02
Copy link

github-actions bot commented Apr 3, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: desrosj <desrosj@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

Let's give it a shot! I agree that 4 seems to be the current sweet spot. Added a few small change requests to address before merging. But no need to re-review.

.github/workflows/unit-test.yml Outdated Show resolved Hide resolved
.github/workflows/unit-test.yml Outdated Show resolved Hide resolved
.github/workflows/unit-test.yml Outdated Show resolved Hide resolved
.github/workflows/unit-test.yml Outdated Show resolved Hide resolved
@sirreal sirreal enabled auto-merge (squash) April 3, 2024 20:01
@sirreal sirreal merged commit c58ed9d into trunk Apr 3, 2024
64 checks passed
@sirreal sirreal deleted the try/shard-js-tests branch April 3, 2024 21:11
@github-actions github-actions bot added this to the Gutenberg 18.2 milestone Apr 3, 2024
cbravobernal pushed a commit to garridinsi/gutenberg that referenced this pull request Apr 9, 2024
Split out JS date unit tests
Shard main JS unit tests into 4 shards.

By sharding the tests, they can run in parallel on different runners. This allows the job to complete in less time. Testing suggests the time is reduced by around 50% from 4m to 2m.

Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: desrosj <desrosj@git.wordpress.org>
@talldan
Copy link
Contributor

talldan commented May 2, 2024

Small note, whenever changing job names like this (for what was formerly a required check for merging into trunk), the required status checks may also need to be updated in the repo settings to include the new job names (under the branches tab).

Access to that may vary so it might require asking someone else to do it.

@desrosj
Copy link
Contributor

desrosj commented May 2, 2024

Sorry about that, @talldan! I forget sometimes that they are associated with the human-readable names assigned. I wish that they were by workflow file instead. Noted for the future though. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants