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

[v16.x backport] node:test #43904

Closed
wants to merge 4 commits into from
Closed

[v16.x backport] node:test #43904

wants to merge 4 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jul 19, 2022

No description provided.

@aduh95 aduh95 added semver-minor PRs that contain new features and should be released in the next minor version. needs-citgm PRs that need a CITGM CI run. test_runner labels Jul 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules
  • @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. v16.x labels Jul 19, 2022
@aduh95 aduh95 requested a review from targos July 19, 2022 16:41
@ljharb
Copy link
Member

ljharb commented Jul 19, 2022

This seems very risky; node typically does not backport new core modules.

@F3n67u
Copy link
Member

F3n67u commented Jul 20, 2022

This seems very risky; node typically does not backport new core modules.

@ljharb Could you elaborate on why it is risky? Thanks.

@ljharb
Copy link
Member

ljharb commented Jul 20, 2022

@F3n67u new core modules are always risky. In the typical case (every single one but node:test) it's adding a new non-prefixed core module, that could break someone who's requiring a folder inside a node_modules or a NODE_PREFIX folder.

In this case, the node: prefix does mean that nobody in node 16 could be relying on it, so it's less risky, but userland tooling doesn't have a way to discover this module exists (#42785), and userland tools like resolve/is-core-module don't assume that anything with a node: prefix is a valid core module, so they'd need to be updated - both directly and transitively.

@targos
Copy link
Member

targos commented Jul 26, 2022

@nodejs/lts what do you think?

@GeoffreyBooth
Copy link
Member

This doesn’t strike me as all that risky; what could break in userland if there’s suddenly an additional prefixed core module? What relies on the list of builtins, and more precisely what would break if that list grew by one?

This feels like a semver-minor change; a new feature has been added. It’s not that different from a new API being added on any of the core modules. Sure, someone might have been depending somehow on the list of APIs on path being exactly 22 things and now there are 23, but that’s the definition of a semver-minor change.

If we don’t backport node:test, there will be lots of other PRs blocked from backporting, as many PRs since node:test landed have started to use that API in Node’s own tests. We could do some kind of special PR that makes node:test available internally but not publicly on 16, to allow these other PRs to be backported, but then we have a weird divergence to deal with in 16. Also node:test is generally useful and we should want our v16 users to enjoy it.

@aduh95
Copy link
Contributor Author

aduh95 commented Jul 26, 2022

What relies on the list of builtins, and more precisely what would break if that list grew by one?

To clarify here, it would break if a userland module was using the same name as the core built-in one. But that doesn't apply here because node:test is only available when using the node: prefix, and no user package can use a name that starts with node: since Node.js 16.0.0 – specifically for this use case.

Also node:test is generally useful and we should want our v16 users to enjoy it.

FWIW they can already, via npm i test :)

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Jul 27, 2022

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2972/

@ljharb
Copy link
Member

ljharb commented Jul 27, 2022

It could be backported without being exposed such that it has no risk and also unblocks additional backport PRs.

@MoLow
Copy link
Member

MoLow commented Jul 27, 2022 •
edited

if this lands, will we need to remove https://github.com/nodejs/node/labels/dont-land-on-v16.x from all/some of these?
https://github.com/nodejs/node/issues?q=label%3Atest_runner+label%3Adont-land-on-v16.x+

@targos
Copy link
Member

targos commented Jul 27, 2022

@MoLow yes, but with v16.x instead of v14.x

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Jul 31, 2022

I think CI was resumed enough times to confirm that the failing test is broken, not flaky.
It's probably unrelated to this PR so I'm starting a local bisect with the debug build.

@cjihrig @targos
test: add initial test module
e9fc0e6 
This commit adds a new 'test' module that exposes an API
for creating JavaScript tests. As the tests execute, TAP
output is written to standard output. This commit only supports
executing individual test files, and does not implement
command line functionality for a full test runner.

PR-URL: nodejs#42325
Refs: nodejs#40954
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@cjihrig @targos
module,repl: support 'node:'-only core modules
dc71d0c 
This commit makes it possible to add new core modules that can
only be require()'ed and imported when the 'node:' scheme is
used. The 'test' module is the first such module.

These 'node:'-only modules are not included in the list returned
by module.builtinModules.

PR-URL: nodejs#42325
Refs: nodejs#40954
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@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>
@cjihrig @targos
test_runner: add initial CLI runner
c2100cc 
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
Copy link
Member

targos commented Jul 31, 2022

I removed #41818 from the staging branch and rebased this PR.

@nodejs-github-bot
Copy link
Collaborator

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

@targos targos self-assigned this Jul 31, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 31, 2022 •
edited by targos

CI: https://ci.nodejs.org/job/node-test-pull-request/45752/ ✔️

targos pushed a commit that referenced this pull request Jul 31, 2022
@cjihrig @targos
test: add initial test module
9a53083 
This commit adds a new 'test' module that exposes an API
for creating JavaScript tests. As the tests execute, TAP
output is written to standard output. This commit only supports
executing individual test files, and does not implement
command line functionality for a full test runner.

PR-URL: #42325
Backport-PR-URL: #43904
Refs: #40954
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@cjihrig @targos
module,repl: support 'node:'-only core modules
3d851d6 
This commit makes it possible to add new core modules that can
only be require()'ed and imported when the 'node:' scheme is
used. The 'test' module is the first such module.

These 'node:'-only modules are not included in the list returned
by module.builtinModules.

PR-URL: #42325
Backport-PR-URL: #43904
Refs: #40954
Reviewed-By: Matteo Collina <matteo.collina@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 pushed a commit that referenced this pull request Jul 31, 2022
@cjihrig @targos
test_runner: add initial CLI runner
cd6f24b 
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
Copy link
Member

targos commented Jul 31, 2022

Landed in 92051cb...cd6f24b

@targos targos closed this Jul 31, 2022
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jul 31, 2022
Merged
@aduh95 aduh95 deleted the backport-node-test branch July 31, 2022 14:36
This was referenced Aug 1, 2022
Open
Open
Open
Open
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@cjihrig @guangwong
test: add initial test module
14d5f90 
This commit adds a new 'test' module that exposes an API
for creating JavaScript tests. As the tests execute, TAP
output is written to standard output. This commit only supports
executing individual test files, and does not implement
command line functionality for a full test runner.

PR-URL: nodejs/node#42325
Backport-PR-URL: nodejs/node#43904
Refs: nodejs/node#40954
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@cjihrig @guangwong
module,repl: support 'node:'-only core modules
88cf0d0 
This commit makes it possible to add new core modules that can
only be require()'ed and imported when the 'node:' scheme is
used. The 'test' module is the first such module.

These 'node:'-only modules are not included in the list returned
by module.builtinModules.

PR-URL: nodejs/node#42325
Backport-PR-URL: nodejs/node#43904
Refs: nodejs/node#40954
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@cjihrig @guangwong
test_runner: add initial CLI runner
f18b872 
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
Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@targos targos targos approved these changes

@juanarbol juanarbol juanarbol approved these changes

Assignees

@targos targos

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. needs-citgm PRs that need a CITGM CI run. semver-minor PRs that contain new features and should be released in the next minor version. test_runner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@aduh95 @nodejs-github-bot @ljharb @F3n67u @targos @GeoffreyBooth @MoLow @juanarbol @cjihrig