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

test runner: concurrency boolean flag #43837

Closed
MoLow opened this issue Jul 14, 2022 · 16 comments · Fixed by #43887
Closed

test runner: concurrency boolean flag #43837

MoLow opened this issue Jul 14, 2022 · 16 comments · Fixed by #43887
Assignees
Labels
good first issue Issues that are suitable for first-time contributors. test_runner

Comments

@MoLow
Copy link
Member

MoLow commented Jul 14, 2022

following up the discussion in #43757
if we want to allow the concurrency option to be a boolean, we need to come up with good defaults

there are two levels of concurrency:

  1. when running multiple test files, using --test each file is spawned in its own process so we use os.cpus().length as the default
  2. inside a test file - we use 1 as the default wich will be equivalent to false

Assuming we want to allow passing concurrency: true in a test file - what number would that convert to?

@MoLow
Copy link
Member Author

MoLow commented Jul 14, 2022

cc @nodejs/test_runner @lpinca @JakobJingleheimer

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label Jul 14, 2022
@ljharb
Copy link
Member

ljharb commented Jul 14, 2022

We should never use os.cpus().length as a default; we should use that minus 1.

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Jul 15, 2022

Actually, we need to spawn a process for each case (it()s)? ex

describe('example', { concurrency: true } () => {
  it('should add A and B', () => {
    assert.strictEqual(arithmetic(1,2, 'add'), 3);
  });

  it('should subtract A from B', () => {
    assert.strictEqual(arithmetic(1,3, 'subtract'), 2);
    assert.strictEqual(arithmetic(3,1, 'subtract'), -2);
  });
});

Those 2 it()s should be run in parallel, but they don't need isolated processes. In that case, do we care about os.cpus() at all?

@juliangruber
Copy link
Member

Concurrent it tests will run in the same process, you only leverage more cpu cores by splitting your tests across multiple files

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Jul 15, 2022

Soooo, why is concurrency an integer at all?

@aduh95
Copy link
Contributor

aduh95 commented Jul 15, 2022

Concurrent it tests will run in the same process, you only leverage more cpu cores by splitting your tests across multiple files

True, except if those it tests spawn new thread/processes on their own.

@juliangruber
Copy link
Member

juliangruber commented Jul 15, 2022

Integer concurrency on the file level lets you control how many processes will be spawned, integer concurrency on the test level lets you limit how much work is done in parallel inside a single process

@JakobJingleheimer
Copy link
Contributor

integer concurrency on the test level lets you limit how much work is done in parallel inside a single process

I can certainly understand wanting to limit the number of process spawned, but why would anyone care to limit the number of cases within that process to a specific number?

@juliangruber
Copy link
Member

Say you have a large file with 2000 it() tests. Some perform compute-heavy sync work, others use resources like the file system, which doesn't provide unlimited file descriptors. It can be necessary or beneficial then to limit how many tests run at the same time.

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Jul 15, 2022

Ahh, okay. Seems a bit of an edge-case; I wouldn't expect an edge-case config option to be the default, but good to have supported.

@JakobJingleheimer
Copy link
Contributor

Assuming we want to allow passing concurrency: true in a test file - what number would that convert to?

Given that it's not related to cpus, does it need to convert to a number?

@MoLow
Copy link
Member Author

MoLow commented Jul 16, 2022

Given that it's not related to cpus, does it need to convert to a number?

I guess unlimited is ok for test level, since a specific limit can be provided with a number.

Please 👍 if this makes sense:

File level concurrency

  • default: true
  • true: CPUs - 1
  • false: 1

Test level concurrency

  • default: false
  • true: unlimited
  • false: 1

@98lenvi
Copy link
Contributor

98lenvi commented Jul 18, 2022

@MoLow I'm interested in taking up this issue. Can this issue be assigned to me please?

@MoLow
Copy link
Member Author

MoLow commented Jul 18, 2022

@98lenvi please do!

98lenvi added a commit to 98lenvi/node that referenced this issue Jul 18, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 18, 2022
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 18, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 18, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 19, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 19, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 20, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
98lenvi added a commit to 98lenvi/node that referenced this issue Jul 23, 2022
adds the test_concurrency flag,
if set to true, runs all the tests in parallel

Fixes: nodejs#43837
aduh95 pushed a commit that referenced this issue Jul 24, 2022
PR-URL: #43887
Fixes: #43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
aduh95 pushed a commit to aduh95/node-core-test that referenced this issue Jul 24, 2022
PR-URL: nodejs/node#43887
Fixes: nodejs/node#43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
(cherry picked from commit dab492f0444b0a6ae8a41dd1d9605e036c363655)
aduh95 pushed a commit to aduh95/node-core-test that referenced this issue Jul 24, 2022
PR-URL: nodejs/node#43887
Fixes: nodejs/node#43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
(cherry picked from commit dab492f0444b0a6ae8a41dd1d9605e036c363655)
danielleadams pushed a commit that referenced this issue Jul 26, 2022
PR-URL: #43887
Fixes: #43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
targos pushed a commit that referenced this issue Aug 1, 2022
PR-URL: #43887
Fixes: #43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
targos pushed a commit that referenced this issue Aug 1, 2022
PR-URL: #43887
Fixes: #43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
PR-URL: nodejs#43887
Fixes: nodejs#43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
PR-URL: nodejs/node#43887
Fixes: nodejs/node#43837
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. test_runner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@juliangruber @ljharb @JakobJingleheimer @MoLow @aduh95 @98lenvi and others