Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
test: add initial test module #42325
Conversation
nodejs-github-bot
added
lib / src
cjihrig
added
the
semver-major
|
Why semver-major? |
It adds a new core module ( |
|
@cjihrig is this ready for a full review or you're looking for general feedback? |
|
@targos it's ready for a full review (I still need to address Antoine's comments from yesterday). It's semver major because it adds a new core module. It's experimental so that the API can evolve and features can be added/changed. The v18 cutoff date will be coming up soon, so I'd like to be mindful of that as well. The name |
|
First off, thank you for working on this! It's looking great so far. Just did a cursory pass through the code and just found a few issues. I want to check it out and play with it a bit before signing off tho. |
|
would it be possible to get CJS and ESM examples for every code block in the docs? Would prefer if we don't ship anything new without both at this point. |
I can do that for the code blocks that would not be identical between CJS and ESM. |
|
As @martinheidegger said in #42325 (comment), exposing this only as |
cjihrig
force-pushed
the
test-pr
branch
2 times, most recently
from
20f1c31
to
ac2b747
Compare
cjihrig
added
commit-queue-rebase
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/42325 ✔ Done loading data for nodejs/node/pull/42325 ----------------------------------- PR info ------------------------------------ Title test: add initial test module (#42325) Author Colin Ihrig (@cjihrig) Branch cjihrig:test-pr -> nodejs:master Labels semver-major, lib / src, needs-ci, commit-queue-rebase Commits 2 - test: add initial test module - module,repl: support 'node:'-only core modules Committers 1 - cjihrig PR-URL: https://github.com/nodejs/node/pull/42325 Refs: https://github.com/nodejs/node/issues/40954 Reviewed-By: Matteo Collina Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/42325 Refs: https://github.com/nodejs/node/issues/40954 Reviewed-By: Matteo Collina Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 13 Mar 2022 19:54:52 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/42325#pullrequestreview-916012177 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42325#pullrequestreview-916296613 ✔ Last GitHub CI successful ✖ No Jenkins CI runs detected -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/2018508154 |
cjihrig
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in da399a6...3c4ee52 |
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 Refs: #40954 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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 Refs: #40954 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
| if (mod?.canBeRequiredByUsers) return mod.exports; | ||
| if (mod?.canBeRequiredByUsers) { | ||
| if (!NativeModule.canBeRequiredWithoutScheme(filename)) { | ||
| throw new ERR_UNKNOWN_BUILTIN_MODULE(filename); |
There was a problem hiding this comment.
It seems to me that instead of throwing an error, it might be better to skip to the user module of the name test?
if (mod?.canBeRequiredByUsers && NativeModule.canBeRequiredWithoutScheme(filename)) {
return mod.exports;
}This would allow for require('test') to still be a user module without introducing a new error and maybe solve the problem of incompatibility?
I am also wondering if it wouldn't be faster to add a canBeRequiredWithoutScheme property in the module constructor
this.canBeRequiredByUsers = !StringPrototypeStartsWith(id, 'internal/');
this.canBeRequiredByUsersWithoutScheme = this.canBeRequiredByUsers && NativeModule.canBeRequiredWithoutScheme(id)then it could be used like:
if (mod?.canBeRequiredByUsersWithoutScheme) return mod.exportsThere was a problem hiding this comment.
It seems to me that instead of throwing an error, it might be better to skip to the user module of the name test?
You're right. I'll send a fix for that.
|
Unfortunate this never cc’d @nodejs/modules. Why is this only available under the node: prefix? |
| const testResources = new SafeMap(); | ||
| const hook = createHook({ | ||
| init(asyncId, type, triggerAsyncId, resource) { | ||
| if (resource instanceof Test) { |
There was a problem hiding this comment.
Is Test’s Symbol.hasInstance locked down? If not, this check isn’t robust.
This comment has been minimized.
This comment has been minimized.
This commit adds a new
testmodule 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 (that will come in a future PR).Refs: #40954