test_runner: fix support watch with run(), add globPatterns option #53866

mcollina · 2024-07-16T13:19:28Z

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 the changed event handler in

Line 423 in 362afa5

const updatedTestFiles = createTestFileList();

.

The problem is that createTestFileList() was loading the glob pattern from argv, which can be whatever. The result was but that was very hard to track.

This PR includes:

tests for run({ watch: true })
a new option called globPatterns that will allow users to control what run() will be globbing
avoid tracking "rename" changes if individual files were specified

nodejs-github-bot · 2024-07-16T13:19:33Z

Review requested:

@nodejs/test_runner

cjihrig · 2024-07-16T13:23:49Z

test/parallel/test-runner-run.mjs

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


Won't this break running the test suite in worker threads?

Without this, I would need to fix #53867 as well in this PR.

Let's see if it actually breaks something in CI.

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.

I think we still need to remove this.

Ok. I'll need to add a cwd parameter to run(), otherwise it won't work.

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.

test/parallel/test-runner-run.mjs

lib/internal/test_runner/runner.js

test/fixtures/test-runner-watch.mjs

lib/internal/test_runner/runner.js

nodejs-github-bot · 2024-07-16T13:45:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60353/

mcollina · 2024-07-16T14:41:26Z

@cjihrig @MoLow can you tell me why this is trying to load tools/github_reporter/index.js?

=== release test-runner-run ===
Path: parallel/test-runner-run
Error: --- stderr ---
node:internal/test_runner/harness:46
      throw err;
      ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/runner/work/node/node/test/.tmp.2615/tools/github_reporter/index.js' imported from /home/runner/work/node/node/test/.tmp.2615/
    at finalizeResolution (node:internal/modules/esm/resolve:260:11)
    at moduleResolve (node:internal/modules/esm/resolve:921:10)
    at defaultResolve (node:internal/modules/esm/resolve:1123:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:557:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:526:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:249:38)
    at onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:484:36)
    at TracingChannel.tracePromise (node:diagnostics_channel:337:14)
    at ModuleLoader.import (node:internal/modules/esm/loader:483:21)
    at node:internal/test_runner/utils:158:45 {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file:///home/runner/work/node/node/test/.tmp.2615/tools/github_reporter/index.js'
}

cjihrig · 2024-07-16T14:46:40Z

My guess is because the Python test runner uses it and the process.chdir() call broke that logic.

mcollina · 2024-07-16T15:10:52Z

I guess it's #53867 at play.

cjihrig · 2024-07-16T15:14:04Z

Yep. But also, the Python runner should probably not use a relative path like that.

doc/api/test.md

atlowChemi · 2024-07-16T17:30:08Z

lib/internal/main/test_runner.js

@@ -72,6 +72,7 @@ const options = {
  setup: setupTestReporters,
  timeout,
  shard,
+  globPatterns: process.argv.slice(1),


use primordials?

It looks like this is not addressed.

test/parallel/test-runner-run-watch.mjs

mcollina · 2024-07-18T15:46:14Z

@cjihrig @atlowChemi @MoLow PTAL, all reviews should have been handled.

nodejs-github-bot · 2024-07-18T15:48:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60423/

cjihrig · 2024-07-18T15:51:30Z

test/parallel/test-runner-run.mjs

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


I think we still need to remove this.

cjihrig · 2024-07-18T15:54:03Z

lib/internal/main/test_runner.js

@@ -72,6 +72,7 @@ const options = {
  setup: setupTestReporters,
  timeout,
  shard,
+  globPatterns: process.argv.slice(1),


It looks like this is not addressed.

cjihrig · 2024-07-18T16:02:14Z

lib/internal/test_runner/runner.js

@@ -503,6 +505,16 @@ function run(options = kEmptyObject) {
  if (only != null) {
    validateBoolean(only, 'options.only');
  }
+  if (globPatterns != null) {
+    validateArray(globPatterns, 'options.globPatterns');


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.

cjihrig · 2024-07-18T16:03:07Z

lib/internal/test_runner/runner.js

-  const patterns = hasUserSuppliedPattern ? ArrayPrototypeSlice(process.argv, 1) : [kDefaultPattern];
+  const hasUserSuppliedPattern = patterns != null;
+  if (!patterns || patterns.length === 0) {
+    patterns = [kDefaultPattern];


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.

nodejs-github-bot · 2024-07-18T16:53:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60426/

nodejs-github-bot · 2024-07-18T17:11:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60428/

mcollina · 2024-07-18T17:19:01Z

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 <hello@matteocollina.com>

nodejs-github-bot · 2024-07-23T16:27:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60571/

Signed-off-by: Matteo Collina <hello@matteocollina.com>

nodejs-github-bot · 2024-07-24T08:41:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60580/

Signed-off-by: Matteo Collina <hello@matteocollina.com>

nodejs-github-bot · 2024-07-24T10:54:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60582/

nodejs-github-bot · 2024-07-24T13:11:22Z

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 <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com>

Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #53866 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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 <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com>

Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #53866 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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

mcollina requested review from cjihrig and MoLow July 16, 2024 13:19

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 16, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2024

cjihrig reviewed Jul 16, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2024

MoLow reviewed Jul 16, 2024

View reviewed changes

doc/api/test.md Show resolved Hide resolved

atlowChemi reviewed Jul 16, 2024

View reviewed changes

doc/api/test.md Outdated Show resolved Hide resolved

atlowChemi reviewed Jul 16, 2024

View reviewed changes

mcollina added request-ci Add this label to start a Jenkins CI on a PR. semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 18, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024

cjihrig reviewed Jul 18, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2024

test_runner: fix support watch with run(), add globPatterns option

70074ab

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2024

fixup

bd7d8e2

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

fixup

689bd6d

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

MoLow approved these changes Jul 24, 2024

View reviewed changes

mcollina added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 24, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 24, 2024

nodejs-github-bot merged commit 2208948 into nodejs:main Jul 24, 2024
59 checks passed

mcollina deleted the support-watch-in-run branch July 24, 2024 13:26

cjihrig mentioned this pull request Jul 25, 2024

test_runner: remove redundant bootstrap boolean #54013

Merged

RafaelGSS mentioned this pull request Jul 30, 2024

v22.6.0 proposal #54123

Merged

mcollina mentioned this pull request Aug 8, 2024

test_runner: ensure test watcher picks up new test files #54225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: fix support watch with run(), add globPatterns option #53866

test_runner: fix support watch with run(), add globPatterns option #53866

mcollina commented Jul 16, 2024

nodejs-github-bot commented Jul 16, 2024

cjihrig Jul 16, 2024

mcollina Jul 17, 2024

cjihrig Jul 18, 2024

cjihrig Jul 18, 2024

mcollina Jul 18, 2024

cjihrig Jul 18, 2024

nodejs-github-bot commented Jul 16, 2024

mcollina commented Jul 16, 2024

cjihrig commented Jul 16, 2024

mcollina commented Jul 16, 2024

cjihrig commented Jul 16, 2024

atlowChemi Jul 16, 2024

mcollina Jul 18, 2024

cjihrig Jul 18, 2024

mcollina commented Jul 18, 2024

nodejs-github-bot commented Jul 18, 2024

cjihrig Jul 18, 2024

cjihrig Jul 18, 2024

cjihrig Jul 18, 2024

cjihrig Jul 18, 2024

nodejs-github-bot commented Jul 18, 2024

nodejs-github-bot commented Jul 18, 2024

mcollina commented Jul 18, 2024 •

edited

Loading

nodejs-github-bot commented Jul 23, 2024

nodejs-github-bot commented Jul 24, 2024

nodejs-github-bot commented Jul 24, 2024

nodejs-github-bot commented Jul 24, 2024

test_runner: fix support watch with run(), add globPatterns option #53866

test_runner: fix support watch with run(), add globPatterns option #53866

Conversation

mcollina commented Jul 16, 2024

nodejs-github-bot commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 16, 2024

mcollina commented Jul 16, 2024

cjihrig commented Jul 16, 2024

mcollina commented Jul 16, 2024

cjihrig commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jul 18, 2024

nodejs-github-bot commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 18, 2024

nodejs-github-bot commented Jul 18, 2024

mcollina commented Jul 18, 2024 • edited Loading

nodejs-github-bot commented Jul 23, 2024

nodejs-github-bot commented Jul 24, 2024

nodejs-github-bot commented Jul 24, 2024

nodejs-github-bot commented Jul 24, 2024

mcollina commented Jul 18, 2024 •

edited

Loading