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

doc: add restrictions around node:test usage #56027

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+21 −0
Open

doc: add restrictions around node:test usage #56027

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62ec497
doc: add restrictions around node:test usage
anonrig Nov 27, 2024
cd7c003
Update doc/contributing/writing-tests.md
anonrig Nov 27, 2024
75dd6c1
Update doc/contributing/writing-tests.md
anonrig Dec 18, 2024
203f8a9
Update doc/contributing/writing-tests.md
anonrig Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions doc/contributing/writing-tests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,26 @@ request. Interesting things to notice:

## General recommendations

### Usage of `node:test`

It is recommended to use `node:test` in tests outside of testing the `node:test`
anonrig marked this conversation as resolved.
Show resolved Hide resolved
module, if particular functionality that's tested is not a dependency of
`node:test` module. This ensures that a bug in the test runner doesn't impact
the outcome of the underlying dependencies' test results.
anonrig marked this conversation as resolved.
Show resolved Hide resolved

These dependencies are:

- `node:async_hooks`
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, async_hooks is probably the only thing I would include here (and I may update the test runner to migrate off of that in the future).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me, but before removing the rest, what's your reasoning for keeping only async_hooks here?

Copy link
Contributor

@cjihrig cjihrig Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

async_hooks actually changes how things work.

child_process and fs are so heavily depended on by other things that if they stop working we will definitely notice. The test runner also doesn't do anything "fancy" with them. You can also use the test runner without spawning child processes. But, child processes are only used by the test runner CLI, which Node core doesn't use at all anyway.

The only place the test runner uses a stream is for emitting events. If you were going to include that, you may as well include event emitter as well since it is part of streams.

The vm module is only used (directly) for evaluating snapshot files.

Also worth noting that the test runner is already used to test the test runner itself 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you recommend changes to the text, please?

anonrig marked this conversation as resolved.
Show resolved Hide resolved
- `node:child_process`
- `node:fs`
- ReadableStream in `node:streams`
anonrig marked this conversation as resolved.
Show resolved Hide resolved
- `node:vm`
Copy link
Member

Choose a reason for hiding this comment

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

I think the files listed in test/parallel/test-bootstrap-modules.js can be a good measure here.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? I don't follow

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth adding anything related to the bootstrapping process to the list of things not to test with the test runner since I'm not sure you can be 100% certain the test runner itself is bootstrapped properly at that point.

Copy link
Member

Choose a reason for hiding this comment

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

(Most of) the files listed there are essential parts of the Node.js functionality that are used more ubiquitously, hence more likely to be depended on by node:test itself (e.g. when we talk about node:async_hooks, that's actually built on top of other modules, not just itself, test/parallel/test-bootstrap-modules.js list a set of files that are generally used everywhere)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you recommend changes to the text, please?


#### Caveats

* `node:url` is excluded due to the extensive testing on web-platform tests.
* `node:path` and `node:os` is excluded due to being used by both test runners.

### Timers

Avoid timers unless the test is specifically testing timers. There are multiple
Expand Down
Loading