-
Notifications
You must be signed in to change notification settings - Fork 5
(perf) - Optimize CI tests time #4802
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4802 +/- ##
=======================================
Coverage 99.55% 99.55%
=======================================
Files 1215 1215
Lines 26542 26542
Branches 8606 8606
=======================================
Hits 26425 26425
Misses 117 117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /** | ||
| * Count test files in a package directory | ||
| */ | ||
| function countTestFiles(packageDir) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used, can be removed.
| /** | ||
| * Get package directory path | ||
| */ | ||
| function getPackageDir(packageName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not used, can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AimeurAmin Leaving comments, I think I need some more time to dig a bit deeper but in the meantime I think we can make some easy improvements.
Let me know if something doesn't make sense! :) Thanks for taking the effort of improving the time it takes to run the tests, it drives me mad as well!
| unit: | ||
| needs: setup-turbo | ||
| runs-on: ubuntu-latest | ||
| # Limit concurrent test jobs to avoid hitting GitHub Actions limits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other two jobs in this file check and extensions and integration-tests are using the following container properties. Should we add it here as well?
container:
image: ghcr.io/yldio/asap-hub/node-python-sq:86a189edc900d4e1afdcf3935c697292f69d409b
credentials:
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
|
|
||
| // Packages that should be sharded (with their shard count) | ||
| const SHARDED_PACKAGES = { | ||
| '@asap-hub/crn-frontend': 8, // Split into 8 shards (128 test files, ~16 per shard) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the number of test files will age badly (128 test files) 😸 just mention the split, and encourage future visitors to update the number based on the number of tests in the corresponding projects.
| if [ -n "${{ matrix.shard }}" ]; then | ||
| SHARD_ARGS="--shard=${{ matrix.shard }}" | ||
| fi | ||
| yarn turbo test:coverage --filter="${{ matrix.package }}" --cache-dir=.turbo -- --maxWorkers=${{ steps.cpu-cores.outputs.count }} --ci $SHARD_ARGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing --maxWorkers=${{ steps.cpu-cores.outputs.count }} to yarn turbo test:coverage will take precedence over the value specified in jest.config.js
So, we can remove this argument entirely here and leave the config in jest using the CI flag.
In summary:
remove the maxWorkers parameter here, which means we won't need the step that calculates the cpu-cores as well, since we'll be using 100% of them in CI all the time.
| testEnvironment: 'jsdom', | ||
|
|
||
| // Performance optimizations | ||
| maxWorkers: process.env.CI ? '100%' : getMaxWorkers(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we use something like
maxWorkers: process.env.CI ? '100%' : '75%';And remove the getMaxWorkers function?
No description provided.