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(ci-tests): split tests across multiple machines #4212

Merged
merged 18 commits into from
Mar 7, 2022

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Mar 4, 2022

🎉 Thanks for submitting a pull request! 🎉

Summary

Redo of #4139 with better load balancing

Todo

  • Add a section in our contributing guide explaining this setup

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@github-actions github-actions bot added the type: chore work needed to keep the product and development running smoothly label Mar 4, 2022
@erezrokah erezrokah changed the title Chore/load balance tests chore(ci-tests): split tests across multiple machines Mar 4, 2022
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tools/tests_duration.mjs Outdated Show resolved Hide resolved
@ehmicky
Copy link
Contributor

ehmicky commented Mar 4, 2022

The speed benefit is really good: 18 minutes to 9 minutes! 🎉

@erezrokah erezrokah mentioned this pull request Mar 4, 2022
5 tasks
@erezrokah
Copy link
Contributor Author

Looks like some tests are failing randomly, so I'll need to figure it out.

Also, once the PR is ready we'll need to update branch protection rules to match the new status checks names

@erezrokah erezrokah force-pushed the chore/load_balance_tests branch 2 times, most recently from 52777c6 to 2f5ff2a Compare March 4, 2022 17:15
@@ -20,7 +20,8 @@
"test:dev": "run-s test:dev:*",
"test:ci": "run-s test:ci:*",
"test:dev:ava": "ava",
"test:ci:ava": "c8 -r lcovonly -r text -r json ava",
"test:measure": "node tools/tests_duration.mjs",
"test:ci:ava": "c8 -r lcovonly -r text -r json ava --concurrency 1",
Copy link
Contributor Author

@erezrokah erezrokah Mar 4, 2022

Choose a reason for hiding this comment

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

I used concurrency as I was getting out of memory errors in CI.

AVA itself runs using serial in their CI, so we should consider doing the same, see https://github.com/avajs/ava/blob/ada1a4f8876ad7b2476440609691e301a02f3951/test/snapshot-workflow/README.md#serial-execution

  • concurrency - number of parallel processes to spawn (a process is spawned per test file)
  • serial - number of parallel tests within the same file

@erezrokah erezrokah marked this pull request as ready for review March 7, 2022 09:48
@erezrokah erezrokah requested a review from ehmicky March 7, 2022 10:11
@erezrokah
Copy link
Contributor Author

This PR is ready. We'll need to update the branch protection rules to be able to merge it.

@erezrokah erezrokah requested a review from ehmicky March 7, 2022 14:42
machine: '0'
- os: ubuntu-latest
node-version: '*'
install-command: npm install --no-package-lock
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering whether YAML aliases could be used to avoid the repetitions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@ehmicky ehmicky Mar 7, 2022

Choose a reason for hiding this comment

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

Too bad :(
Let's keep the repetition then 👍

@erezrokah erezrokah merged commit 931faf4 into main Mar 7, 2022
@erezrokah erezrokah deleted the chore/load_balance_tests branch March 7, 2022 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants