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

Add support for running all tests serially #49487

Closed
mcollina opened this issue Sep 4, 2023 · 11 comments · Fixed by #49996
Closed

Add support for running all tests serially #49487

mcollina opened this issue Sep 4, 2023 · 11 comments · Fixed by #49996
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@mcollina
Copy link
Member

mcollina commented Sep 4, 2023

What is the problem this feature will solve?

Currently it's very hard to use node:test to test against an external database or an external service with state as multiple files are automatically run in parallel.

What is the feature you are proposing to solve the problem?

Adding a node --test --test-concurrenty=1 will do the trick

What alternatives have you considered?

Calling run manually.

@mcollina mcollina added the feature request Issues that request new features to be added to Node.js. label Sep 4, 2023
@MoLow MoLow added test_runner Issues and PRs related to the test runner subsystem. good first issue Issues that are suitable for first-time contributors. labels Sep 4, 2023
@MoLow
Copy link
Member

MoLow commented Sep 4, 2023

we have had some pushback on adding more cli flags for the test runner but besides that implementation would be trivial.

@mcollina
Copy link
Member Author

mcollina commented Sep 4, 2023

I think most applications would need this as it's a very common case. Environment variables are also good to configure this.

Alternatively, we are forcing users to call run manually, implementing a runner themselves. If this is the case, then we should add some docs in this regard.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 4, 2023

I'm in favor of supporting this use case. I think the biggest questions to answer are:

  • What exactly is the flag? We have a concurrency option for tests. It supports numbers and booleans. I don't think we should support both in the CLI.
  • Does this flag impact tests within files at all? Currently, the CLI uses parallelism, but individual files do not by default. If I set this new flag, will it only impact the CLI? People will inevitably want to change the behavior of both.

@MrJithil
Copy link
Member

MrJithil commented Sep 25, 2023

lib/internal/test_runner/test.js: 263

switch (typeof concurrency) {
      case 'number':
        validateUint32(concurrency, 'options.concurrency', 1);
        this.concurrency = concurrency;
        break;

      case 'boolean':
        if (concurrency) {
          this.concurrency = parent === null ?
            MathMax(availableParallelism() - 1, 1) : Infinity;
        } else {
          this.concurrency = 1;
        }
        break;

      default:
        if (concurrency != null)
          throw new ERR_INVALID_ARG_TYPE('options.concurrency', ['boolean', 'number'], concurrency);
    }

So, if we pass the concurrency = true , will it use the maximum possible threads?

The proposal is to suppress this and use the integer value passed to concurrency option?

I can work on it, please guide.

@mcollina
Copy link
Member Author

I think if nothing is specified, it keeps the current behavior. Otherwise, a number is specified, setting concurrency to X.

@MrJithil
Copy link
Member

I think if nothing is specified, it keeps the current behavior. Otherwise, a number is specified, setting concurrency to X.

Sure. Thanks

@MrJithil
Copy link
Member

MrJithil commented Sep 27, 2023

case 'number':
        validateUint32(concurrency, 'options.concurrency', 1);
        this.concurrency = concurrency;
        break;

this is the way its working. So, if we are passing the concurrency option as a number, it will use it.

So, what changes we are proposing here?

@mcollina
Copy link
Member Author

Add a cli flag to set the concurrency of node --test.

@adrian-burlacu-software
Copy link

adrian-burlacu-software commented Sep 28, 2023

Ugly solution I have so far in src/tests/utils/runner without any command line flags:

import { tap } from 'node:test/reporters';
import process from 'node:process';
import { run } from 'node:test';
import fs from 'fs';
import path from 'path';

let Files: string[]  = [];
function ThroughDirectory(Directory: string) {
  fs.readdirSync(Directory).forEach(File => {
        const Absolute = path.join(Directory, File);
        if (fs.statSync(Absolute).isDirectory()) return ThroughDirectory(Absolute);
        else if (Absolute.endsWith('.test.js')) return Files.push(Absolute);
    });
}
ThroughDirectory('./dist/test');

// console.log('Files: ' + JSON.stringify(Files));

run({ concurrency: 1, files: Files })
  .compose(tap)
  .pipe(process.stdout);

In package.json:
"scripts": { "test": "node --env-file=./test/utils/.test.env --test-reporter=spec ./dist/test/utils/runner.js" },

Would be good if I could output in spec format as a stream and get the files names/respect the testNamePatterns without the files argument somehow.

cjihrig added a commit to cjihrig/node that referenced this issue Oct 1, 2023
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

Fixes: nodejs#49487
@cjihrig
Copy link
Contributor

cjihrig commented Oct 1, 2023

Proposed fix in #49996.

I decided to address the concurrency of the CLI only. If we want to provide a mechanism for controlling the concurrency within a test file there are at least a few ways to extend what I've implemented:

  • Add yet another flag for that.
  • Support passing the new flag twice with the order having some special meaning.
  • Extend the new flag's syntax to something like --test-concurrency=1,2, where the first value controls the CLI and the second value controls the concurrency within files.
  • Something totally different?

The concurrency within individual files is not a priority for me at this time though.

cjihrig added a commit to cjihrig/node that referenced this issue Oct 1, 2023
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

Fixes: nodejs#49487
@MoLow
Copy link
Member

MoLow commented Oct 1, 2023

The concurrency within individual files is not a priority for me at this time though.

  • 1, a top-level describe is usually sufficient for this

cjihrig added a commit to cjihrig/node that referenced this issue Oct 8, 2023
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

Fixes: nodejs#49487
@cjihrig cjihrig closed this as completed in 9f9c582 Oct 8, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

PR-URL: nodejs#49996
Fixes: nodejs#49487
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
targos pushed a commit that referenced this issue Nov 11, 2023
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

PR-URL: #49996
Fixes: #49487
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
targos pushed a commit that referenced this issue Nov 27, 2023
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

PR-URL: #49996
Fixes: #49487
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

PR-URL: nodejs#49996
Fixes: nodejs#49487
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

PR-URL: nodejs/node#49996
Fixes: nodejs/node#49487
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this issue Apr 25, 2024
This commit adds a new --test-concurrency CLI flag that controls
the parallelism of the test runner CLI.

PR-URL: nodejs/node#49996
Fixes: nodejs/node#49487
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants