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 initial CLI runner #42658

Merged
merged 1 commit into from Apr 15, 2022
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 8, 2022

This commit introduces an initial version of a CLI-based test runner.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 8, 2022
doc/api/test.md Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md 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
lib/internal/test_runner/tap_stream.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Apr 8, 2022

//cc @nodejs/test_runner

@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 8, 2022
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

How does --test affect --require, or loaders? What happens to the REPL with node --test and no other arguments?

If NODE_OPTIONS='--test' is set, and the user doesn't control how node is invoked (via a shebang, for example), does this mean the user can never "undo" test mode?

This really feels to me like it should be an entirely distinct binary, rather than just a "mode" of the main node binary.

doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md 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/test_runner/utils.js Outdated Show resolved Hide resolved
lib/internal/test_runner/tap_stream.js Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Show resolved Hide resolved
@richardlau
Copy link
Member

If NODE_OPTIONS='--test' is set, and the user doesn't control how node is invoked (via a shebang, for example), does this mean the user can never "undo" test mode?

As this PR currently is --test hasn't been added to the allow list for NODE_OPTIONS so setting it there would throw an error.

@ljharb
Copy link
Member

ljharb commented Apr 8, 2022

@richardlau thanks, if that's explicitly going to never be allowed there then that does mitigate that one concern, but all the others remain.

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 8, 2022

How does --test affect --require, or loaders? What happens to the REPL with node --test and no other arguments?

node --test doesn't launch the REPL. I haven't tried a loader, but --require seems to work fine.

If NODE_OPTIONS='--test' is set, and the user doesn't control how node is invoked (via a shebang, for example), does this mean the user can never "undo" test mode?

--test in NODE_OPTIONS explicitly errors out.

doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
@cjihrig cjihrig force-pushed the test-cli branch 2 times, most recently from f3263c3 to 805361e Compare April 9, 2022 18:32
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
lib/internal/test_runner/utils.js Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
}

if (debug_options_.inspector_enabled) {
errors->push_back("the inspector cannot be used with --test");
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: why is this? I can definitely see wanting to run the inspector for debugging while running tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A couple reasons:

  1. First, it's trivial to run an individual file with the inspector flags. At this point, the CLI runner is just spawning Node child processes with no special flags.
  2. Debugging through the CLI test runner is an awkward experience similar to the cluster module. Each test file will need a different debugger port. We would need to manage that logic as well as relay the correct information to users (I want to debug test X, I need to debug file Y, and connect to debug port Z). It also doesn't make sense to set something like --inspect-brk on a bunch of child processes.
  3. I looked at a couple test frameworks, and they didn't seem to have a great story around propagating inspector flags to test files.
  4. If someone is passionate about this, it can be added in the future. It just didn't seem worth it to me for an initial version given the previous points. I think it would make sense in the future to have configuration options for execArgv and argv of the child processes.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use chrome's debugger blackboxing stuff to "hide" the test runner code when inspecting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly - I'm not sure though.

@nodejs-github-bot nodejs-github-bot merged commit adaf602 into nodejs:master Apr 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in adaf602

@cjihrig cjihrig deleted the test-cli branch April 15, 2022 17:50
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
This commit introduces an initial version of a CLI-based
test runner.

PR-URL: nodejs#42658
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2022
This commit introduces an initial version of a CLI-based
test runner.

PR-URL: #42658
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos added a commit that referenced this pull request May 2, 2022
Notable changes:

doc:
  * add @kuriyosh to collaborators (Yoshiki Kurihara) #42824
lib,src:
  * (SEMVER-MINOR) implement WebAssembly Web API (Tobias Nießen) #42701
test_runner:
  * (SEMVER-MINOR) add initial CLI runner (Colin Ihrig) #42658
worker:
  * (SEMVER-MINOR) add hasRef() to MessagePort (Darshan Sen) #42849

PR-URL: #42943
@targos targos mentioned this pull request May 2, 2022
targos added a commit that referenced this pull request May 3, 2022
Notable changes:

doc:
  * add @kuriyosh to collaborators (Yoshiki Kurihara) #42824
lib,src:
  * (SEMVER-MINOR) implement WebAssembly Web API (Tobias Nießen) #42701
test_runner:
  * (SEMVER-MINOR) add initial CLI runner (Colin Ihrig) #42658
worker:
  * (SEMVER-MINOR) add hasRef() to MessagePort (Darshan Sen) #42849

PR-URL: #42943
@sosoba
Copy link
Contributor

sosoba commented May 4, 2022

Who will extend the @types/node package with a new node:test module?

@Trott
Copy link
Member

Trott commented May 4, 2022

Who will extend the @types/node package with a new node:test module?

As always, the maintainers of https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node.

aduh95 pushed a commit to aduh95/node that referenced this pull request Jul 19, 2022
This commit introduces an initial version of a CLI-based
test runner.

PR-URL: nodejs#42658
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to aduh95/node that referenced this pull request Jul 31, 2022
This commit introduces an initial version of a CLI-based
test runner.

PR-URL: nodejs#42658
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
This commit introduces an initial version of a CLI-based
test runner.

PR-URL: #42658
Backport-PR-URL: #43904
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos added a commit that referenced this pull request Aug 2, 2022
Notable changes:

crypto:
  * (SEMVER-MINOR) remove Node.js-specific webcrypto extensions (Filip Skokan) #43310
  * (SEMVER-MINOR) add CFRG curves to Web Crypto API (Filip Skokan) #42507
  * (SEMVER-MINOR) make authTagLength optional for CC20P1305 (Tobias Nießen) #42427
  * (SEMVER-MINOR) align webcrypto RSA key import/export with other implementations (Filip Skokan) #42816
deps:
  * update undici to 5.4.0 (Node.js GitHub Bot) #43262
  * update undici to 5.3.0 (Node.js GitHub Bot) #43197
dns:
  * (SEMVER-MINOR) export error code constants from `dns/promises` (Feng Yu) #43176
doc:
  * add F3n67u to collaborators (Feng Yu) #43953
  * deprecate coercion to integer in process.exit (Daeyeon Jeong) #43738
  * (SEMVER-MINOR) deprecate diagnostics_channel object subscribe method (Stephen Belanger) #42714
  * add LiviaMedeiros to collaborators (LiviaMedeiros) #43039
  * add @kuriyosh to collaborators (Yoshiki Kurihara) #42824
  * add RafaelGSS to collaborators (RafaelGSS) #42718
  * add @meixg to collaborators (Xuguang Mei) #42576
errors:
  * (SEMVER-MINOR) add support for cause in aborterror (James M Snell) #41008
esm:
  * (SEMVER-MINOR) add chaining to loaders (Jacob Smith) #42623
events:
  * (SEMVER-MINOR) expose CustomEvent on global with CLI flag (Daeyeon Jeong) #43885
  * (SEMVER-MINOR) add `CustomEvent` (Daeyeon Jeong) #43514
  * (SEMVER-MINOR) propagate abortsignal reason in new AbortError ctor in events (James M Snell) #41008
fs:
  * (SEMVER-MINOR) propagate abortsignal reason in new AbortSignal constructors (James M Snell) #41008
  * (SEMVER-MINOR) make params in writing methods optional (LiviaMedeiros) #42601
  * (SEMVER-MINOR) add `read(buffer[, options])` versions (LiviaMedeiros) #42768
http:
  * (SEMVER-MINOR) add drop request event for http server (theanarkh) #43806
  * (SEMVER-MINOR) add diagnostics channel for http client (theanarkh) #43580
  * (SEMVER-MINOR) add perf_hooks detail for http request and client (theanarkh) #43361
  * (SEMVER-MINOR) add uniqueHeaders option to request and createServer (Paolo Insogna) #41397
http2:
  * (SEMVER-MINOR) propagate abortsignal reason in new AbortError constructor (James M Snell) #41008
  * (SEMVER-MINOR) compat support for array headers (OneNail) #42901
lib:
  * (SEMVER-MINOR) propagate abortsignal reason in new AbortError constructor in blob (James M Snell) #41008
  * (SEMVER-MINOR) add abortSignal.throwIfAborted() (James M Snell) #40951
  * (SEMVER-MINOR) improved diagnostics_channel subscribe/unsubscribe (Stephen Belanger) #42714
module:
  * (SEMVER-MINOR) add isBuiltIn method (hemanth.hm) #43396
module,repl:
  * (SEMVER-MINOR) support 'node:'-only core modules (Colin Ihrig) #42325
net:
  * (SEMVER-MINOR) add drop event for net server (theanarkh) #43582
  * (SEMVER-MINOR) add ability to reset a tcp socket (pupilTong) #43112
node-api:
  * (SEMVER-MINOR) emit uncaught-exception on unhandled tsfn callbacks (Chengzhong Wu) #36510
perf_hooks:
  * (SEMVER-MINOR) add PerformanceResourceTiming (RafaelGSS) #42725
report:
  * (SEMVER-MINOR) add more heap infos in process report (theanarkh) #43116
src:
  * (SEMVER-MINOR) add --openssl-legacy-provider option (Daniel Bevenius) #40478
  * (SEMVER-MINOR) define fs.constants.S_IWUSR & S_IRUSR for Win (Liviu Ionescu) #42757
src,doc,test:
  * (SEMVER-MAJOR) add --openssl-shared-config option (Daniel Bevenius) #43124
stream:
  * (SEMVER-MINOR) use cause options in AbortError constructors (James M Snell) #41008
  * (SEMVER-MINOR) add iterator helper find (Nitzan Uziely) #41849
  * (SEMVER-MINOR) add writableAborted (Robert Nagy) #40802
test:
  * (SEMVER-MINOR) add initial test module (Colin Ihrig) #42325
test_runner:
  * (SEMVER-MINOR) expose `describe` and `it` (Moshe Atlow) #43420
  * (SEMVER-MINOR) add initial CLI runner (Colin Ihrig) #42658
  * (SEMVER-MINOR) support 'only' tests (Colin Ihrig) #42514
timers:
  * (SEMVER-MINOR) propagate signal.reason in awaitable timers (James M Snell) #41008
util:
  * (SEMVER-MINOR) add tokens to parseArgs (John Gee) #43459
  * (SEMVER-MINOR) add parseArgs module (Benjamin Coe) #42675
v8:
  * (SEMVER-MINOR) add v8.startupSnapshot utils (Joyee Cheung) #43329
  * (SEMVER-MINOR) export more fields in getHeapStatistics (theanarkh) #42784
worker:
  * (SEMVER-MINOR) add hasRef() to MessagePort (Darshan Sen) #42849

PR-URL: TODO
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
This commit introduces an initial version of a CLI-based
test runner.

PR-URL: nodejs/node#42658
Backport-PR-URL: nodejs/node#43904
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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. lib / src Issues and PRs related to general changes in the lib or src directory. 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet