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

Speed up tests locally by upto 45% #1818

Merged
merged 2 commits into from
Sep 24, 2023

Conversation

anshgoyalevil
Copy link
Contributor

Which problem is this PR solving?

  • This PR aims to add an additional flag to the jest script which would make running tests locally even faster than ever.

Description of the changes

  • It uses --maxWorkers=50% flag to do so.

How was this change tested?

  • It was tested with several flags as below;
-----------------
yarn test

Time:        96.903 s
-----------------
yarn test --runInBand

Time:        131.721 s, estimated 606 s
-----------------
yarn test --maxWorkers=50%

Time:        53.295 s
-----------------

Operating System Used to Benchmark:
Ubuntu 22

Checklist

@anshgoyalevil anshgoyalevil marked this pull request as ready for review September 24, 2023 15:14
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

Patch has no changes to coverable lines.

📢 Thoughts on this report? Let us know!.

@@ -104,7 +104,7 @@
"coverage": "yarn run test-ci --coverage",
"start:ga-debug": "REACT_APP_GA_DEBUG=1 REACT_APP_VSN_STATE=$(../../scripts/get-tracking-version.js) vite",
"start": "vite",
"test": "jest",
"test": "jest --maxWorkers=50%",
Copy link
Member

Choose a reason for hiding this comment

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

does % refer to the number of available CPU cores?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's the CPU Cores present in the current run environment. Most of the articles on internet recommends this setting.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense for local testing, so as not to bring the whole system down during the test. But if this %-based addressing scheme is available, why does the line below have --maxWorkers=4? We don't really know how many CPU cores are available in a CI runner, but it would make sense to utilize all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every CI environment has their own workers limit, and the setting which we are currently using for test-ci is adapted to GitHub Actions only.

Moreover, reason for choosing a concrete value (4) instead of % over here is because in CI, we know we won't be performing multi-tasking, so all threads can be used for tests. (Minus the thread for main workflow task)

In future, if we have any plans to move to another CI environment like CircleCI, I think we would again have to figure out this max number of workers. (I guess, it's 8 for CircleCI).

Copy link
Member

Choose a reason for hiding this comment

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

But why can't we say 100% for GitHub? When the tests are running, no other activity is higher priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoretically that seems right. Don't know why everyone suggests using a number value instead of percentage in CI for jest.

I would have to try that thing.

@yurishkuro yurishkuro merged commit e51ec20 into jaegertracing:main Sep 24, 2023
9 checks passed
@anshgoyalevil anshgoyalevil deleted the boost-tests-locally branch September 25, 2023 13:22
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.

None yet

2 participants