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: add shards support #48639

Merged
merged 18 commits into from
Jul 5, 2023

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Jul 2, 2023

Fix #48619

TODOs:

  • add tests
    • run tests
    • CLI tests
  • update docs
  • allow running from cli as well

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 2, 2023
@rluvaton rluvaton marked this pull request as ready for review July 3, 2023 10:07
@rluvaton
Copy link
Member Author

rluvaton commented Jul 3, 2023

@MoLow this is ready for review, thanks

doc/api/cli.md Outdated Show resolved Hide resolved
@meyfa
Copy link
Contributor

meyfa commented Jul 3, 2023

How good is the compatibility with globbing? For example:

$ node --test --shards=1/3 "test/**/*.js"

Is globbing deterministic in a way that when sharded, each test file runs exactly once?

@rluvaton
Copy link
Member Author

rluvaton commented Jul 3, 2023

Yes, it works well with globbing as we sort the tests files that we are about to run

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
if (shard != null) {
validateObject(shard, 'options.shard');
validateNumber(shard.total, 'options.shard.total', 1);
validateNumber(shard.index, 'options.shard.index');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
validateNumber(shard.index, 'options.shard.index');
validateInteger(shard.index, 'options.shard.index', 1, shard.total);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used the max here as the error message is not really understandable because you don't understand what's the reason for the upper limit

Comment on lines +441 to +443
if (shard.index <= 0 || shard.total < shard.index) {
throw new ERR_OUT_OF_RANGE('options.shard.index', `>= 1 && <= ${shard.total} ("options.shard.total")`, shard.index);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed based on my previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see that comment answer 😄

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
Comment on lines +47 to +48
const index = NumberParseInt(indexStr, 10);
const total = NumberParseInt(totalStr, 10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw if it's NumberParseInt, the , 10 isn't required - only on the global one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the global one it's not required as far as I know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's required in the conceptual sense, because parseInt will try to guess the radix if you omit it.

validateNumber(shard.total, 'options.shard.total', 1);
validateNumber(shard.index, 'options.shard.index');
// Avoid re-evaluating the shard object in case it's a getter
shard = { index: shard.index, total: shard.total };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shard = { index: shard.index, total: shard.total };
shard = { __proto__: null, index: shard.index, total: shard.total };

Copy link
Member Author

@rluvaton rluvaton Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason there is no eslint rule for that or there is some limitation prevents from implementing it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anybody's taken the time to make one. There will be a few places where objects shouldn't have a null prototype - mostly when they're returned to userland - but those could have override comments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#48646 need to answer some questions before changing all files

rluvaton pushed a commit to rluvaton/node that referenced this pull request Jul 21, 2023
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Notable changes:

doc:
  * add atlowChemi to collaborators (atlowChemi) nodejs#48757
events:
  * (SEMVER-MINOR) allow safely adding listener to abortSignal (Chemi Atlow) nodejs#48596
fs:
  * add a fast-path for readFileSync utf-8 (Yagiz Nizipli) nodejs#48658
test_runner:
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs#48639

PR-URL: nodejs#48761
@ruyadorno
Copy link
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 11, 2023
@rluvaton
Copy link
Member Author

Missed that, will create one now

rluvaton added a commit to rluvaton/node that referenced this pull request Sep 22, 2023
PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@rluvaton rluvaton added backport-open-v18.x Indicate that the PR has an open backport. backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v20.x Indicate that the PR has an open backport labels Sep 22, 2023
@rluvaton
Copy link
Member Author

@ruyadorno backported to v18 created in #49762

rluvaton added a commit to rluvaton/node that referenced this pull request Sep 22, 2023
PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
rluvaton added a commit to rluvaton/node that referenced this pull request Sep 22, 2023
PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
rluvaton added a commit to rluvaton/node that referenced this pull request Sep 22, 2023
PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request Oct 28, 2023
PR-URL: #48639
Backport-PR-URL: #49762
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos added a commit that referenced this pull request Nov 27, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
  * (SEMVER-MINOR) upgrade npm to 10.0.0 (npm team) #49423
doc:
  * add new TSC members (Michael Dawson) #48841
  * move and rename loaders section (Geoffrey Booth) #49261
esm:
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
  * unflag import.meta.resolve (Guy Bedford) #49028
  * move hook execution to separate thread (Jacob Smith) #44710
  * leverage loaders when resolving subsequent loaders (Maël Nison) #43772
lib:
  * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391
  * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943
src:
  * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629
stream:
  * use bitmap in readable state (Benjamin Gruenbaum) #49745
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614
  * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975
  * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141
wasi:
  * (SEMVER-MINOR) updates required for latest uvwasi version (Michael Dawson) #49908

PR-URL: TODO
targos added a commit that referenced this pull request Nov 28, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
  * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531
doc:
  * move and rename loaders section (Geoffrey Booth) #49261
esm:
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
  * unflag import.meta.resolve (Guy Bedford) #49028
  * move hook execution to separate thread (Jacob Smith) #44710
  * leverage loaders when resolving subsequent loaders (Maël Nison) #43772
lib:
  * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391
  * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943
src:
  * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629
stream:
  * use bitmap in readable state (Benjamin Gruenbaum) #49745
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614
  * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975
  * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141

PR-URL: #50953
targos added a commit that referenced this pull request Nov 29, 2023
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908
  * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531
doc:
  * move and rename loaders section (Geoffrey Booth) #49261
esm:
  * use import attributes instead of import assertions (Antoine du Hamel) #50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869
  * unflag import.meta.resolve (Guy Bedford) #49028
  * move hook execution to separate thread (Jacob Smith) #44710
  * leverage loaders when resolving subsequent loaders (Maël Nison) #43772
lib:
  * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391
  * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943
src:
  * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629
stream:
  * use bitmap in readable state (Benjamin Gruenbaum) #49745
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614
  * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975
  * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141

PR-URL: #50953
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48639
Backport-PR-URL: nodejs/node#49762
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908
  * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531
doc:
  * move and rename loaders section (Geoffrey Booth) nodejs/node#49261
esm:
  * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869
  * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028
  * move hook execution to separate thread (Jacob Smith) nodejs/node#44710
  * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772
lib:
  * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391
  * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943
src:
  * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629
stream:
  * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614
  * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141

PR-URL: nodejs/node#50953
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48639
Backport-PR-URL: nodejs/node#49762
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Notable changes:

deps:
  * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908
  * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531
doc:
  * move and rename loaders section (Geoffrey Booth) nodejs/node#49261
esm:
  * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140
  * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869
  * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028
  * move hook execution to separate thread (Jacob Smith) nodejs/node#44710
  * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772
lib:
  * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391
  * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943
src:
  * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629
stream:
  * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614
  * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975
  * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639
  * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775
test_runner, cli:
  * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996
tls:
  * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190
vm:
  * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141

PR-URL: nodejs/node#50953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v18.x Indicate that the PR has an open backport. backported-to-v20.x PRs backported to the v20.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test runner under --shard <shard>