Skip to content

concurrent tests are slow #47365

Closed
Closed

Description

@iambumblehead

Version

v19.8.1

Platform

Darwin Bumbles-MBP.home 21.6.0 Darwin Kernel Version 21.6.0: Mon Dec 19 20:44:01 PST 2022; root:xnu-8020.240.18~2/RELEASE_X86_64 x86_64

What steps will reproduce the bug?

Use this repo https://github.com/iambumblehead/demo-slow-node-test

from the package,

  • npm run test-ava takes about 10 seconds
    Screen Shot 2023-04-20 at 10 37 00 AM
  • npm run test-node-describe takes about 30 seconds
    Screen Shot 2023-04-20 at 10 39 05 AM
  • npm run test-node-test takes about 30 seconds
    Screen Shot 2023-04-20 at 10 41 18 AM

How often does it reproduce? Is there a required condition?

It is reproduced any time the test repo is used on this machine

What is the expected behavior? Why is that the expected behavior?

node:test should use multiple cores and should not be slow

What do you see instead?

Hello, I recently replaced node:test with ava in a project using ~250 tests. The node:test runner would complete in ~18 minutes and the ava runner completes in ~9 minutes. The node:test version of the project used describe/it tests with the following pattern,

describe('app', { concurrency: true }, async () => {
  beforeEach(() => {
    nock.disableNetConnect();
  });

  it('test a', async () => {
    assert.ok(true);
  });
  
  it('test b', async () => {
    assert.ok(true);
  });
  
  // ...
});

Additional information

Test concurrency might have some problems. Initially, I tried using { concurrency: true } with a single file that contained about a dozen tests and tests completed in a time that was observably faster. When { concurrency: true } was used with multiple files, the improvement seemed to go away and each group of tests would run slowly,

Test concurrency requires extra boilerplate. Eg, to run tests concurrently, one needs to use nested describe/it tests, nested tests wrapped in a promise or use a configuration file. related links here, and here. It would be nice if concurrent tests were easier to achieve with a special import import test from 'node:test/concurrent' or with a cli option --test-concurrency=true, link to a related comment here: do not expose a test-concurrency flag.

Thank you

Activity

  1. ForbiddenEra commented on Apr 20, 2023

    @ForbiddenEra

    using os.availableParallelism() - 1 for files and 1 for test()'s is a heuristic that tries to aim at the common use cases (or what we believe they are). there might be a better value but as you said, that would really depend on the specific tests and use-case, thus this is configurable

    Just want to mention that it may be a common case where some/all tests are intended to run sequentially.

    One might not want test2.js to run if test1.js fails or modify what tests test2.js runs.. or one might want test1.js to run basically sequentially and if it passes then test2.js could run in parallel.

    The current implementation isn't really ideal for this and I can't really use run() or --test in my situations, I have a run-tests.js file that launches the tests instead, generally importing a default exported async function from each test file and calling that to initiate it's tests.

    I can provide some more in-depth examples that relate to projects I'm using node:test for already if anyone is curious or wants more info.

  2. tniessen commented on Apr 20, 2023

    @tniessen
    Member

    Thanks for the detailed response @MoLow, that's very helpful. So every test file has its own process, and within a test file, there is no real concurrency in the classical sense: all tests run within the same application thread. That seems be confirmed by @iambumblehead's statement above: "node:test uses one cpu it appears."

    What surprises me is that the docs say that test({ concurrency: true }) is equivalent to test({ concurrency: os.availableParallelism() - 1 }). However, as far as I know, os.availableParallelism() is pretty much meaningless for scaling within a single thread.

  3. targos commented on Apr 20, 2023

    @targos
    Member

    @tniessen what would be a good default value in your opinion? Infinity?

  4. tniessen commented on Apr 20, 2023

    @tniessen
    Member

    npm run test-ava

    @iambumblehead Running this on a 16-core CPU, it takes less than 5 seconds wall clock time, but about 30 seconds of user CPU time. In other words, it utilizes about 50 % of all available CPU cores.

    npm run test-node-test

    Running this on the same CPU, it takes about 20 seconds wall clock time, and about the same amount of user CPU time. In other words, it utilizes just a single CPU core out of 16.

  5. MoLow commented on Apr 20, 2023

    @MoLow
    Member

    npm run test-node-test
    Running this on the same CPU, it takes about 20 seconds wall clock time, and about the same amount of user CPU time. In other words, it utilizes just a single CPU core out of 16.

    I have not yet had the time to look into the repro, but if node --test files does not use all available cores that sounds like an issue

  6. iambumblehead commented on Apr 20, 2023

    @iambumblehead
    Author

    btop cpu while running while true; do npm run test-ava; done

    Screen Shot 2023-04-20 at 10 28 07 AM

    btop cpu while running while true; do npm run test-node-describe; done

    Screen Shot 2023-04-20 at 10 29 48 AM

  7. MoLow commented on Apr 22, 2023

    @MoLow
    Member

    @iambumblehead test files were actually running serially, thanks for the good repro!
    after the fix, it seems like AVA and node --test perform pretty much the same

  8. iambumblehead commented on Apr 22, 2023

    @iambumblehead
    Author

    @MoLow awesome it will be great to try it out soon!

  9. tniessen commented on Apr 26, 2023

    @tniessen
    Member

    what would be a good default value in your opinion? Infinity?

    @targos It seems like the documentation is incorrect and the implementation does use Infinity in that case. Similar to the issue that was discovered in #47675, this also does not seem to be covered by any tests.

    PR for more doc fixes and test: #47734

  10. added a commit that references this issue on Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    performanceIssues and PRs related to the performance of Node.js.test_runnerIssues and PRs related to the test runner subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions