Skip to content

doc: add restrictions around node:test usage#56027

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

Closed
anonrig wants to merge 4 commits into from
Closed

doc: add restrictions around node:test usage #56027

Changes from all commits
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
21 changes: 21 additions & 0 deletions doc/contributing/writing-tests.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -141,6 +141,27 @@ request. Interesting things to notice:

## General recommendations

### Usage of `node:test`

It is optional to use `node:test` in tests outside of testing the `node:test`

@joyeecheungjoyeecheungNov 27, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems important to document things like #52177 or otherwise we would see more flakes coming up once people start to spawn hundreds of child processes in parallel and overloading the machine using spawnPromisified + node:test....

@cjihrigcjihrigNov 27, 2024

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.

Agreed. Using the concurrency option is fine though unless you are specifically planning to spawn child processes. But that applies to things like Promise.all() as well.

@anonriganonrigNov 27, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@cjihrigcjihrigJan 21, 2025

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.

I have thought about this a bit, and I don't believe we should include this list.

  • First, it is inaccurate and subject to change. For example, child_process is only used by the run() API, which is not something we would be using in our tests anyway.
  • Second, by following this logic, we would need to update our current test suite to not use test/common in many places. For example, test/common/index.js, which is included in almost every test file imports a number of core modules.
  • Third, some modules are almost impossible to avoid and our current testing setup does not try to do that. For example, we use fs to read test files to look for test flags, we use child_process to respawn the test file with the correct flags, we use an 'exit' event to check for leaked globals and mustCalls().
  • Fourth, if we are going to maintain a list of problematic things to test, then that list should probably be based on real problems that have been encountered (such as spawning a bunch of child processes concurrently, which coincidentally the test runner can actually help with, and we have a workaround for running tests locally vs. in CI already).

@anonriganonrigJan 22, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fourth, if we are going to maintain a list of problematic things to test, then that list should probably be based on real problems that have been encountered

I agree about this statement. Let's wait for tomorrow's @nodejs/tsc meeting to add this.

@joyeecheungjoyeecheungJan 22, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
It is optional to use `node:test`in tests outside of testing the `node:test`
It is discouraged to change existing tests that do not use `node:test`to use it, if there is no
other better motivation. For newly added tests, is optional to use `node:test` in tests outside of testing
the `node:test`

I think we should mention that this is optional for newly added tests, to avoid encouraging people update existing tests solely for stylistic preferences like what happened in #56671. If people have to update it to use node:test based on stylistic preferences, IMO we should require them to break what it is testing deliberately in the PR and and run the CI both before and after the update, and see what the output looks like, to see if the use of node:test makes the output look worse than before, so that other people don't get hit by something like https://ci.nodejs.org/job/node-test-commit-osx/63165/nodes=osx11-x64/testReport/junit/(root)/parallel/test_runner_module_mocking/ afterwards.

module, as long as the functionality being tested is not a dependency of the
`node:test` module. This ensures that a bug in the test runner doesn't impact
the outcome of the underlying dependencies' test results.

These dependencies are:

- `node:events`
- `node:async_hooks`

@cjihrigcjihrigNov 27, 2024

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.

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).

@anonriganonrigNov 27, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@cjihrigcjihrigNov 27, 2024•
edited
Loading

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.

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 😄

@anonriganonrigNov 27, 2024

Copy link
Copy Markdown
MemberAuthor

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?

Comment thread
anonrig marked this conversation as resolved.
- `node:child_process`
- `node:fs`
- `node:streams` with the exception of WebStreams
- `node:vm`

@joyeecheungjoyeecheungNov 27, 2024

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

@anonriganonrigNov 27, 2024

Copy link
Copy Markdown
MemberAuthor

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

@cjihrigcjihrigNov 27, 2024

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.

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.

@joyeecheungjoyeecheungNov 27, 2024

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

@anonriganonrigNov 27, 2024

Copy link
Copy Markdown
MemberAuthor

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