Skip to content

test_runner: support 'only' tests - #42514

Merged
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
cjihrig:only-flag
Apr 4, 2022
Merged

test_runner: support 'only' tests#42514
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
cjihrig:only-flag

Conversation

@cjihrig

@cjihrig cjihrig commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

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

aduh95 commented Mar 30, 2022

Copy link
Copy Markdown
Contributor

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

@cjihrig

cjihrig commented Mar 30, 2022

Copy link
Copy Markdown
Contributor Author

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

aduh95 commented Mar 31, 2022

Copy link
Copy Markdown
Contributor

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

cjihrig commented Mar 31, 2022

Copy link
Copy Markdown
Contributor Author

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

nodejs-github-bot commented Apr 3, 2022

Copy link
Copy Markdown
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

nodejs-github-bot commented Apr 4, 2022

Copy link
Copy Markdown
Collaborator

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

aduh95
aduh95 approved these changes Apr 4, 2022
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented Apr 4, 2022

Copy link
Copy Markdown
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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@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 <jasnell@gmail.com>
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

Reviewers

@jasnell jasnell jasnell approved these changes
@aduh95 aduh95 aduh95 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