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.

Sign up for GitHub

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

Jump to bottom

test_runner: support 'only' tests #42514

Merged
merged 1 commit into from
Apr 4, 2022
Merged

test_runner: support 'only' tests #42514

merged 1 commit into from
Apr 4, 2022

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 29, 2022

This commit introduces a CLI flag and test runner functionality to support running a subset of tests that are indicated by an 'only' option passed to the test.

@cjihrig
test_runner: support 'only' tests
e651e18 
This commit introduces a CLI flag and test runner functionality
to support running a subset of tests that are indicated by an
'only' option passed to the test.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 29, 2022
@aduh95 aduh95 added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v12.x labels Mar 29, 2022
@aduh95
Copy link
Contributor

aduh95 commented Mar 30, 2022

What's the use case for this? A boolean flag doesn't seem very efficient to select what tests to run 🤔

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 30, 2022

What's the use case for this?

If you mean the 'only' functionality, it's present in just about every test framework that I can think of.

A boolean flag doesn't seem very efficient to select what tests to run

A few things to note here:

  • To implement 'only' functionality, you either need a flag (like what is done here, and also what tap does) or you need to process every test that could run and then revisit the ones marked with 'only'. The test runner in core (like tap) tries to execute the tests as quickly as possible, so the flag makes more sense IMO. I also think visiting every test before running any tests seems less efficient.
  • A nice side effect of using the flag is that you need to explicitly opt into 'only' mode. This means if you accidentally check in code with the 'only' flag set (which I've seen many times), you won't leave your test suite in a bad state. For example, tape has a --no-only flag that is noted as "particularly useful in a CI environment where an only test is not supposed to go unnoticed." Mocha has a similar --forbid-only flag.
  • Having multiple flags to specify which tests to run isn't terribly uncommon. For example, lab has at least two flags for specifying this. Mocha has at least two or three as well.

@aduh95
Copy link
Contributor

aduh95 commented Mar 31, 2022

Ive never heard of that feature before this PR ^^ just tried it as I was debugging some Jest test, and sure I can see how it’s useful :) imo the DX would be improved if the default was the less performant option, with a flag to disable it for CI? Wdyt?

@cjihrig
Copy link
Contributor Author

cjihrig commented Mar 31, 2022

imo the DX would be improved if the default was the less performant option, with a flag to disable it for CI?

I'd prefer to keep the common case faster to be honest.

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Apr 4, 2022
Open
9 tasks
@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM. I still wonder if it would make sense to have a --test-dev-mode flag instead in case we want to add more DX features that would have impact on performance, so we don't end up with a pletora of flags to add when debugging tests.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2022
@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2022
@nodejs-github-bot nodejs-github-bot merged commit 54819f0 into nodejs:master Apr 4, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 54819f0

@cjihrig cjihrig deleted the only-flag branch April 4, 2022 14:16
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2022
Merged
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@cjihrig @xtx1130
test_runner: support 'only' tests
2800d47 
This commit introduces a CLI flag and test runner functionality
to support running a subset of tests that are indicated by an
'only' option passed to the test.

PR-URL: nodejs#42514
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this pull request Jul 19, 2022
@cjihrig @aduh95
test_runner: support 'only' tests
4c2743f 
This commit introduces a CLI flag and test runner functionality
to support running a subset of tests that are indicated by an
'only' option passed to the test.

PR-URL: nodejs#42514
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit to aduh95/node that referenced this pull request Jul 31, 2022
@cjihrig @targos
test_runner: support 'only' tests
a77bdaf 
This commit introduces a CLI flag and test runner functionality
to support running a subset of tests that are indicated by an
'only' option passed to the test.

PR-URL: nodejs#42514
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@cjihrig @targos
test_runner: support 'only' tests
a4b277c 
This commit introduces a CLI flag and test runner functionality
to support running a subset of tests that are indicated by an
'only' option passed to the test.

PR-URL: #42514
Backport-PR-URL: #43904
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos added a commit that referenced this pull request Aug 2, 2022
@targos
2022-08-16, Version 16.17.0 'Gallium' (LTS)
8f36b1c 
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
@targos targos mentioned this pull request Aug 2, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@cjihrig @guangwong
test_runner: support 'only' tests
4fdebab 
This commit introduces a CLI flag and test runner functionality
to support running a subset of tests that are indicated by an
'only' option passed to the test.

PR-URL: nodejs/node#42514
Backport-PR-URL: nodejs/node#43904
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjihrig @aduh95 @nodejs-github-bot @jasnell @targos