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

Run tests in parallel #3449

Merged
merged 2 commits into from Nov 10, 2022
Merged

Run tests in parallel #3449

merged 2 commits into from Nov 10, 2022

Conversation

RReverser
Copy link
Contributor

  • Run tests in parallel. On my ThinkPad laptop with 8 cores, this reduces tests execution time from 60-70 seconds to 30-35 seconds (essentially ~2x faster turnaround).
  • Move Mocha settings to a config file. This allows to easily invoke Mocha alone via npx mocha, or for e.g. VSCode Test Explorer to find & run tests with the correct settings automatically.

Reduces tests execution time from 62s to 36s.
package.json Outdated Show resolved Hide resolved
This allows to easily invoke Mocha alone via `npx mocha`, or for e.g. VSCode Test Explorer to find & run tests with the correct settings automatically.
@lovell
Copy link
Owner

lovell commented Nov 10, 2022

Thank you for this improvement Ingvar.

There are a few util tests (cache, concurrency, counters) that require sequential running and may sporadically fail or cause other tests to fail when run in parallel. We should ensure these run after other tests have completed, perhaps via some kind of "after all" hook.

@RReverser
Copy link
Contributor Author

There are a few util tests (cache, concurrency, counters) that require sequential running and may sporadically fail or cause other tests to fail when run in parallel.

I actually did run into that in my ahem other branch, but not with native build, even though I tried even running those asserts in a tight loop while Mocha was executing other test files.

Note that Mocha's parallel mode only parallelises tests at file-level, rather than per-test level, so I guess what happens is that each individual test file gets clean environment with its own copy of native module anyway, hence the counters are indeed zero in the individual test.

@RReverser
Copy link
Contributor Author

If I try to add e.g. console.log("sharp init") at the top level of constructor.js, I also see number of logs corresponding to my number of CPUs:

sharp init
sharp init
sharp init
sharp init
sharp init
sharp init
sharp init

So it does reuse the instances, but within each instance tests are running one by one and can't step on each others' toes and concurrently modify globals - that's why counters test passes successfully.

@lovell
Copy link
Owner

lovell commented Nov 10, 2022

Great, it looks like the overall situation has improved since I last tried this, e.g. commits c4bcd08 and d7d580a

Let's merge and should we experience flaky tests we can deal with specific failures, if any. Thanks again!

@lovell lovell merged commit 3a64a05 into lovell:main Nov 10, 2022
@RReverser RReverser deleted the parallel-tests branch November 10, 2022 22:44
@RReverser
Copy link
Contributor Author

No worries, and yeah, it's certainly a valid concern and something I tried to test extensively before making the PR. And I'm still not 100% sure - parallel stuff is always hard 😅 Happy to make another one if something does turn out flaky.

@RReverser RReverser mentioned this pull request Nov 11, 2022
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

3 participants