-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: fix support watch with run(), add globPatterns option #53866
test_runner: fix support watch with run(), add globPatterns option #53866
Conversation
Review requested:
|
test/parallel/test-runner-run.mjs
Outdated
// This is needed, because calling run() with no files will | ||
// load files matching from cwd, which includes all content of test/ | ||
tmpdir.refresh(); | ||
process.chdir(tmpdir.path); |
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.
Won't this break running the test suite in worker threads?
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.
Without this, I would need to fix #53867 as well in this PR.
Let's see if it actually breaks something in CI.
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 looks like it did break in the CI (https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/37045/). The issue is that the Node test suite is run within a worker thread in the CI. Any test that uses an API that is unsupported in worker threads needs to be skipped there.
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 think we still need to remove this.
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.
Ok. I'll need to add a cwd
parameter to run()
, otherwise it won't work.
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 would hold off on adding a cwd
parameter for now, but I do foresee us adding it in the future. I'm just not sure if it will break anything until the necessary refactoring happens. Another option is to spawn a child process with the correct cwd.
@cjihrig @MoLow can you tell me why this is trying to load
|
My guess is because the Python test runner uses it and the |
I guess it's #53867 at play. |
Yep. But also, the Python runner should probably not use a relative path like that. |
lib/internal/main/test_runner.js
Outdated
@@ -72,6 +72,7 @@ const options = { | |||
setup: setupTestReporters, | |||
timeout, | |||
shard, | |||
globPatterns: process.argv.slice(1), |
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.
use primordials?
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.
done
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 looks like this is not addressed.
@cjihrig @atlowChemi @MoLow PTAL, all reviews should have been handled. |
test/parallel/test-runner-run.mjs
Outdated
// This is needed, because calling run() with no files will | ||
// load files matching from cwd, which includes all content of test/ | ||
tmpdir.refresh(); | ||
process.chdir(tmpdir.path); |
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 think we still need to remove this.
lib/internal/main/test_runner.js
Outdated
@@ -72,6 +72,7 @@ const options = { | |||
setup: setupTestReporters, | |||
timeout, | |||
shard, | |||
globPatterns: process.argv.slice(1), |
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 looks like this is not addressed.
@@ -503,6 +505,16 @@ function run(options = kEmptyObject) { | |||
if (only != null) { | |||
validateBoolean(only, 'options.only'); | |||
} | |||
if (globPatterns != null) { | |||
validateArray(globPatterns, 'options.globPatterns'); |
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.
We should probably validate that the array is not empty. You can add the logic here. Otherwise, I will probably do it in a follow up.
const patterns = hasUserSuppliedPattern ? ArrayPrototypeSlice(process.argv, 1) : [kDefaultPattern]; | ||
const hasUserSuppliedPattern = patterns != null; | ||
if (!patterns || patterns.length === 0) { | ||
patterns = [kDefaultPattern]; |
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 would assign the default patterns in the run()
validation. You can do that in this PR, otherwise, I will probably do it in a follow up.
Can someone help me with this? I see no conflicts locally but CI does: https://ci.nodejs.org/job/node-test-commit/72259/console. Was by any chance a force push broke something? |
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Landed in 2208948 |
This commit updates the test runner's code coverage so that coverage options are explicitly passed in instead of pulled from command line options. PR-URL: #53931 Refs: #53924 Refs: #53867 Refs: #53866 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
Signed-off-by: Matteo Collina <[email protected]> PR-URL: #53866 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: TODO
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 * (SEMVER-MINOR) unflag detect-module (Geoffrey Booth) #53619 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
This commit updates the test runner's code coverage so that coverage options are explicitly passed in instead of pulled from command line options. PR-URL: #53931 Refs: #53924 Refs: #53867 Refs: #53866 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
Signed-off-by: Matteo Collina <[email protected]> PR-URL: #53866 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
Notable changes: deps: * (SEMVER-MINOR) V8: backport 7857eb34db42 (Stephen Belanger) #53997 http: * (SEMVER-MINOR) add diagnostics channel `http.client.request.error` (Kohei Ueno) #54054 inspector: * (SEMVER-MINOR) add initial support for network inspection (Kohei Ueno) #53593 lib,src: * drop --experimental-network-imports (Rafael Gonzaga) #53822 meta: * add jake to collaborators (jakecastelli) #54004 module: * (SEMVER-MINOR) add --experimental-strip-types (Marco Ippolito) #53725 stream: * (SEMVER-MINOR) expose DuplexPair API (Austin Wright) #34111 test_runner: * (SEMVER-MINOR) fix support watch with run(), add globPatterns option (Matteo Collina) #53866 * (SEMVER-MINOR) refactor snapshots to get file from context (Colin Ihrig) #53853 * (SEMVER-MINOR) add context.filePath (Colin Ihrig) #53853 PR-URL: #54123
This PR fixes one regression I found when using
run({ watch: true })
: a different test was run after a file rename.This was caused by calling
createTestFileList()
from thechanged
event handler innode/lib/internal/test_runner/runner.js
Line 423 in 362afa5
The problem is that
createTestFileList()
was loading the glob pattern fromargv
, which can be whatever. The result was but that was very hard to track.This PR includes:
run({ watch: true })
globPatterns
that will allow users to control whatrun()
will be globbing