ci(test): split into multiple machines #4164

erezrokah · 2022-01-31T17:54:59Z

🎉 Thanks for submitting a pull request! 🎉

Summary

This is a PR to split our tests across multiple machines.
ava sorts the files by name, then split them to chunks across machines. See here
Due to ava's behavior I grouped our unit tests to run during the build job and our integration tests to run during the test job. Integration tests will get split across machines.
Since there isn't a good way to load balance the tests, I renamed the specs to better distribute them (yes I know this is ugly).
I also divided the dev command tests into 6 specs to help with load balancing (the dev spec had over 60 tests).

To stabilize the tests I changed some specs to use test.serial.

Also see avajs/ava#2947 for disabling worker threads

Todo

Update branch protection rules once PR is approved

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 · 2022-01-31T17:55:47Z

📊 Benchmark results

Comparing with 54040d8

Package size: 360 MB

⬇️ 0.00% decrease vs. 54040d8

^  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  359 MB  359 MB  360 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T

Legend

T-30 (54040d8): 360 MB
T-29 (1981515): 360 MB
T-28 (434cdb7): 360 MB
T-27 (c89d81a): 360 MB
T-26 (0550e60): 360 MB
T-25 (f1c4d10): 360 MB
T-24 (50b3483): 360 MB
T-23 (df33dd0): 360 MB
T-22 (a41323f): 360 MB
T-21 (b2b0faa): 360 MB
T-20 (7d65669): 360 MB
T-19 (1c8968d): 360 MB
T-18 (40b1cf9): 360 MB
T-17 (39eb031): 360 MB
T-16 (8720854): 360 MB
T-15 (7bb0322): 360 MB
T-14 (4e5eb1a): 360 MB
T-13 (3d7a4e1): 360 MB
T-12 (b247e69): 360 MB
T-11 (4d58ae5): 360 MB
T-10 (826ef86): 360 MB
T-9 (4c13e5d): 360 MB
T-8 (3c807bc): 360 MB
T-7 (68ef285): 360 MB
T-6 (b41c4c9): 360 MB
T-5 (918d1ad): 360 MB
T-4 (ab8276f): 360 MB
T-3 (2722b53): 360 MB
T-2 (a50c4d9): 359 MB
T-1 (d588dd2): 359 MB
T (current commit): 360 MB

package.json

erezrokah · 2022-01-31T17:57:25Z

.github/workflows/main.yml

        continue-on-error: true
        with:
          file: coverage/coverage-final.json
          flags: ${{ steps.test-coverage-flags.outputs.os }},${{ steps.test-coverage-flags.outputs.node }}
+        if: '${{ !steps.release-check.outputs.IS_RELEASE }}'
+  all:


This is here strictly to make branch protection rules easier, to avoid changing those based on matrix naming

erezrokah · 2022-02-03T13:10:56Z

src/utils/rules-proxy.js

@@ -97,4 +104,5 @@ module.exports = {
  onChanges,
  getLanguage,
  createRewriter,
+  getWatchers,


So we can close the watchers in the tests

erezrokah · 2022-02-03T13:15:15Z

tests/integration/230.rules-proxy.test.js

@@ -44,8 +46,8 @@ test.after(async (t) => {
    t.context.server.on('close', resolve)
    t.context.server.close()
  })
-  // TODO: check why this line breaks the rewriter on windows
-  // await t.context.builder.cleanupAsync()
+  await Promise.all(getWatchers().map((watcher) => watcher.close()))


We need to close the watchers as a part of the cleanup

erezrokah · 2022-02-03T13:16:51Z

tests/integration/utils/cli-path.js

@@ -0,0 +1,5 @@
+const path = require('path')


This file was renamed

ehmicky · 2022-02-03T15:21:52Z

.github/workflows/main.yml

    runs-on: ${{ matrix.os }}
    timeout-minutes: 30
    strategy:
      matrix:
        os: [ubuntu-latest, macOS-latest, windows-latest]
        node-version: [12.x, '*']
+        machine: ['0', '1', '2', '3', '4', '5', '6']


That's a good idea!

I am wondering about how this will scale. As we add more tests, we might be likely to distribute too many or not enough tests to a specific number, because that distribution is manual instead of being automated.

Not sure if this is worth exploring for this initial PR, but I am wondering whether we could hash each test filename (or file path relative to the repository root directory), then use a modulo on them to know to which machine to assign?
For example, if we had 16 machines, we could make a SHA-1 of each test filename, then use the last hexadecimal number to decide on which machine to use. We could make the base arbitrary (not only hexadecimal / base 16) so that increasing/decreasing the number of machines just works.
While this would be a more thorough initial implementation work, this would decrease the amount of maintenance: we would not need to figure out which digit to assign to each test file, instead the only thing we'd need to remember would be to keep test files small. Also, we would be able to optimize the best amount of machines easily by just trying different ones and see how long the CI tests take.

What do you think?

Note: that's just an idea. The initial PR looks ready to ship otherwise as it already helps a lot with the performance as is.
Also, we should probably merge this asap to avoid merge conflicts.

This is a good idea, and I think we should definitely find a way to automate the distribution.
I like the idea of small specs (maybe we can enforce via a lint rule), making the need to load balance less important.

ehmicky · 2022-02-03T15:25:39Z

This is awesome @erezrokah!

github-actions bot added the type: chore work needed to keep the product and development running smoothly label Jan 31, 2022

erezrokah commented Jan 31, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

erezrokah commented Jan 31, 2022

View reviewed changes

erezrokah changed the title ~~Test/split into more machines~~ Test: split into more machines Jan 31, 2022

erezrokah changed the title ~~Test: split into more machines~~ ci(test): split into multiple machines Jan 31, 2022

erezrokah force-pushed the test/split_into_more_machines branch 3 times, most recently from f6e9c84 to c5cce82 Compare February 1, 2022 10:46

erezrokah mentioned this pull request Feb 1, 2022

test: disable flaky tests #4159

Closed

5 tasks

erezrokah force-pushed the test/split_into_more_machines branch 20 times, most recently from d2379ef to 55fc4ce Compare February 3, 2022 12:52

erezrokah force-pushed the test/split_into_more_machines branch from 55fc4ce to e3e90fa Compare February 3, 2022 13:00

erezrokah marked this pull request as ready for review February 3, 2022 13:10

erezrokah commented Feb 3, 2022

View reviewed changes

tests/integration/utils/cli-path.js

@@ -0,0 +1,5 @@

const path = require('path')

Copy link

Contributor Author

erezrokah Feb 3, 2022

Choose a reason for hiding this comment

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

This file was renamed

ci(test): split tests into multiple machines

94f78ee

erezrokah force-pushed the test/split_into_more_machines branch from e3e90fa to 94f78ee Compare February 3, 2022 13:22

erezrokah requested review from lukasholzer and ehmicky February 3, 2022 13:35

test: add --no-worker-threads

93990b5

ehmicky reviewed Feb 3, 2022

View reviewed changes

ehmicky approved these changes Feb 3, 2022

View reviewed changes

erezrokah added the automerge Add to Kodiak auto merge queue label Feb 3, 2022

kodiakhq bot merged commit c8281ec into main Feb 3, 2022

kodiakhq bot deleted the test/split_into_more_machines branch February 3, 2022 15:36

anmonteiro mentioned this pull request Feb 3, 2022

chore: inject the Authlify Token in netlify dev #4167

Merged

5 tasks

danez mentioned this pull request Nov 30, 2022

chore: introduce vitests for tests #5269

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci(test): split into multiple machines #4164

ci(test): split into multiple machines #4164

erezrokah commented Jan 31, 2022 •

edited

Loading

github-actions bot commented Jan 31, 2022 •

edited

Loading

erezrokah Jan 31, 2022

erezrokah Feb 3, 2022

erezrokah Feb 3, 2022

erezrokah Feb 3, 2022

ehmicky Feb 3, 2022 •

edited

Loading

erezrokah Feb 3, 2022

ehmicky commented Feb 3, 2022

ci(test): split into multiple machines #4164

ci(test): split into multiple machines #4164

Conversation

erezrokah commented Jan 31, 2022 • edited Loading

Summary

Todo

github-actions bot commented Jan 31, 2022 • edited Loading

📊 Benchmark results

Package size: 360 MB

erezrokah Jan 31, 2022

Choose a reason for hiding this comment

erezrokah Feb 3, 2022

Choose a reason for hiding this comment

erezrokah Feb 3, 2022

Choose a reason for hiding this comment

erezrokah Feb 3, 2022

Choose a reason for hiding this comment

ehmicky Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

erezrokah Feb 3, 2022

Choose a reason for hiding this comment

ehmicky commented Feb 3, 2022

erezrokah commented Jan 31, 2022 •

edited

Loading

github-actions bot commented Jan 31, 2022 •

edited

Loading

ehmicky Feb 3, 2022 •

edited

Loading