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

Open
MoLow opened this issue Jul 14, 2022 · 11 comments
Open

test runner: concurrency boolean flag #43837

MoLow opened this issue Jul 14, 2022 · 11 comments
Labels
good first issue test_runner

Comments

@MoLow
Copy link
Member

@MoLow 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 MoLow commented Jul 14, 2022

cc @nodejs/test_runner @lpinca @JakobJingleheimer

@MoLow MoLow added the good first issue label Jul 14, 2022
@ljharb
Copy link
Member

@ljharb 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 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

@juliangruber juliangruber 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

@JakobJingleheimer
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer commented Jul 15, 2022

Soooo, why is concurrency an integer at all?

@aduh95
Copy link
Contributor

@aduh95 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 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

@JakobJingleheimer JakobJingleheimer commented Jul 15, 2022

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

@juliangruber juliangruber commented Jul 15, 2022

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 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

@JakobJingleheimer JakobJingleheimer commented Jul 16, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue test_runner
Projects
None yet
Development

No branches or pull requests

5 participants