GeezerVison

View reviewed changes

Comment thread

lib/internal/test_runner/runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

rluvaton added 4 commits

July 3, 2023 11:40


          update validation

ed19709


          allow passing shards in CLI

5e39d75


          test_runner: add cli support

5c6ab88


          test_runner: added docs

d293da5

rluvaton marked this pull request as ready for review

July 3, 2023 10:07

rluvaton commented Jul 3, 2023

Copy link

Copy Markdown

Member Author

@MoLow this is ready for review, thanks

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton added 3 commits

July 3, 2023 13:11


          remove comment

51f7697


          format

0625b75


          fix example

2e0040b

meyfa reviewed

View reviewed changes

Comment thread

doc/api/cli.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

meyfa commented Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton commented Jul 3, 2023

Copy link

Copy Markdown

Member Author

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

meyfa and MoLow reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          test_runner: replace shards option with shard and --test-shard

602f767

targos reviewed

View reviewed changes

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.


          test_runner: used stricter shard validation

4ce4d76

cjihrig reviewed

View reviewed changes

Comment thread

lib/internal/test_runner/runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/test_runner/runner.js Outdated

		if (shard != null) {
		validateObject(shard, 'options.shard');
		validateNumber(shard.total, 'options.shard.total', 1);
		validateNumber(shard.index, 'options.shard.index');

cjihrig Jul 3, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rluvaton Jul 3, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/test_runner/runner.js

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);
		}

cjihrig Jul 3, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow reacted with thumbs up emoji

All reactions

👍 1 reaction

rluvaton Jul 3, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

see that comment answer 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/internal/test_runner/runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/test_runner/runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

MoLow reviewed

View reviewed changes

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

rluvaton added 2 commits

July 3, 2023 18:10


          update node man page

b7009d7


          test_runner: update based on CR

76cfcb6

ljharb reviewed

View reviewed changes

Comment thread

lib/internal/main/test_runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

lib/internal/main/test_runner.js

Comment on lines +47 to +48

		const index = NumberParseInt(indexStr, 10);
		const total = NumberParseInt(totalStr, 10);

ljharb Jul 3, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rluvaton Jul 3, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ljharb Jul 3, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton, benjamingr, goldbergyoni, and styfle reacted with thumbs up emoji

All reactions

👍 4 reactions

Comment thread

lib/internal/test_runner/runner.js Outdated

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

ljharb Jul 3, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rluvaton Jul 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MoLow reacted with thumbs up emoji

All reactions

👍 1 reaction

ljharb Jul 3, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rluvaton Jul 3, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll make one

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ljharb reacted with heart emoji

All reactions

❤️ 1 reaction

rluvaton Jul 3, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

MoLow approved these changes

View reviewed changes

Ceres6 pushed a commit to Ceres6/node that referenced this pull request


          test_runner: add shards support

1d03c88

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


          2023-07-18, Version 20.5.0 (Current)

4c6cd1d

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


          test_runner: add shards support

99e738a

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

[v18.x backport] test_runner: add shards support #49761


          2023-07-18, Version 20.5.0 (Current)

067c4ce

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 commented Sep 11, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ruyadorno added the backport-requested-v18.x label

Sep 11, 2023

rluvaton commented Sep 22, 2023

Copy link

Copy Markdown

Member Author

Missed that, will create one now

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton added a commit to rluvaton/node that referenced this pull request

Sep 22, 2023


          test_runner: add shards support

9be6419

PR-URL: nodejs#48639
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

This was referenced Sep 22, 2023

Closed

[v18.x backport] test_runner: add shards support #49762

Closed

rluvaton added backport-open-v18.x and removed backport-requested-v18.x labels

Sep 22, 2023

rluvaton commented Sep 22, 2023

Copy link

Copy Markdown

Member Author

@ruyadorno backported to v18 created in #49762

ruyadorno reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rluvaton added a commit to rluvaton/node that referenced this pull request

Sep 22, 2023


          test_runner: add shards support

4dcd96b

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


          test_runner: add shards support

b55eb2a

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


          2023-11-29, Version 18.19.0 'Hydrogen' (LTS)

933e5ea

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


          2023-11-29, Version 18.19.0 'Hydrogen' (LTS)

9e0c715

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 mentioned this pull request

Nov 28, 2023

v18.19.0 release proposal #50953

Merged

targos added a commit that referenced this pull request

Nov 29, 2023


          2023-11-29, Version 18.19.0 'Hydrogen' (LTS)

8787acb

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


          test_runner: add shards support

90b7c8d

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


          2023-11-29, Version 18.19.0 'Hydrogen' (LTS)

9d87f0b

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


          test_runner: add shards support

20e3c93

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