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

chore: Run tests in parallel jobs #4076

Merged
merged 2 commits into from
Nov 12, 2024
Merged

chore: Run tests in parallel jobs #4076

merged 2 commits into from
Nov 12, 2024

Conversation

KapJI
Copy link
Contributor

@KapJI KapJI commented Nov 8, 2024

Changes

  • Add -filter flag to TestScript which allows to filter scripts by filename using regex. This can be useful for tests sharding and running individual test locally.
  • go test -short runs all tests except TestScript.
  • Split tests into 2 jobs:
    • test-index: 0 runs ./... -short and TestScript tests ^[a-h]
    • test-index: 1 run TestScript remaining tests ^[i-z]
  • Split macos tests into 3 jobs:
    • test-index: 0 runs ./... -short and TestScript tests ^[a-d]
    • test-index: 1 run TestScript remaining tests ^[e-i]
    • test-index: 2 run TestScript remaining tests ^[j-z]

Results

  • Split all tests into 2 shards: Workflow completion is down from 9m30s to 6m20s, 33% faster.
  • Split macos tests into 3 shards: Workflow completion is down from 9m30s to 4m58s, 47% faster.

And this is where we started: 22m55s, 78% improvement!

@KapJI

This comment was marked as outdated.

@KapJI KapJI closed this Nov 8, 2024
@KapJI KapJI reopened this Nov 8, 2024
@KapJI KapJI force-pushed the cache-tests branch 4 times, most recently from 482256a to ab9b4b0 Compare November 8, 2024 21:35
@KapJI KapJI closed this Nov 8, 2024
@KapJI KapJI reopened this Nov 9, 2024
@KapJI KapJI force-pushed the cache-tests branch 2 times, most recently from ab5c974 to 355b7dd Compare November 9, 2024 21:23
@twpayne
Copy link
Owner

twpayne commented Nov 9, 2024

Hey, I really appreciate your work here! At the same time, is the runtime of the tests dominated by running the tests themselves or is the runtime dominated by all the cache loading/compiling that has to happen before? If the test time dominates then splitting the tests makes sense. If not...

@KapJI
Copy link
Contributor Author

KapJI commented Nov 9, 2024

It seems that listing (or rather building test binaries) takes around 1 minute. Remaining 5-6 minutes is test execution.

My goal here is to reduce test jobs run time to about 3 minutes, then they will be on par with other CI jobs. This should save another 3 minutes for workflow completion on PRs and releases.

@twpayne
Copy link
Owner

twpayne commented Nov 9, 2024

Thank you! The testscript tests are very I/O heavy. If there's a way to run them on a ramdisk, it might be a significant win.

@KapJI KapJI force-pushed the cache-tests branch 7 times, most recently from 5e09d30 to 7946fad Compare November 9, 2024 22:57
@KapJI
Copy link
Contributor Author

KapJI commented Nov 9, 2024

If there's a way to run them on a ramdisk, it might be a significant win.

I tried in 7946fad but there is absolutely no difference unfortunately. Maybe it's already a ramdisk.

@KapJI KapJI force-pushed the cache-tests branch 7 times, most recently from d94d8e6 to b959f39 Compare November 11, 2024 14:57
@KapJI KapJI marked this pull request as ready for review November 11, 2024 15:44
Copy link
Collaborator

@halostatue halostatue left a comment

Choose a reason for hiding this comment

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

This is very clever. The only question I have is whether the test scripts are "balanced" in the way you segmented them (0-9a-h and i-z). I suspect that it doesn't matter much because there's not that many, and it wouldn't be too hard to make more slices if the number of test scripts increases.

@KapJI
Copy link
Contributor Author

KapJI commented Nov 11, 2024

The only question I have is whether the test scripts are "balanced"

The runtime of jobs is pretty close as you can see. Mac jobs are the slowest now, it's pretty easy to split them into 3 parts if needed.

@KapJI KapJI force-pushed the cache-tests branch 3 times, most recently from 7797454 to 0ca0c80 Compare November 11, 2024 22:07
@KapJI
Copy link
Contributor Author

KapJI commented Nov 11, 2024

So this should save additional 4 minutes on every PR and release CI, sounds pretty good.

@KapJI KapJI changed the title chore: Run testscript tests in separate job chore: Run tests in parallel jobs Nov 11, 2024
@twpayne twpayne merged commit 9844fb3 into twpayne:master Nov 12, 2024
29 checks passed
@twpayne
Copy link
Owner

twpayne commented Nov 12, 2024

Brilliant, thank you!

@KapJI KapJI deleted the cache-tests branch November 12, 2024 17:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants