Review requested:

• @nodejs/tsc

Some issues I've found with node:test that I think worth looking into before recommending using node:test.

1. The test does not immediately stop when a test case fails, instead it continue running other test cases and output a bunch of lines in the form of ✔ <test description> (time spent on test cases) that I have to ignore. In some files the lines I have to ignore can be multi page and it can be very frustrating. When the test is written in the "classic" style there's no fishing required - you only see failure information in stderr, and the passed test cases are silent unless you deliberately log things. I find that behavior a lot more efficient. Is there a way to make that the default in the test reporter we use to reduce the noise level?

2. 1 is amplified in the CI, where the special unicode characters used by the test reporters are not decoded properly on e.g. Windows and adds a lot of gibberish to the logs.

3. IIUC if the tests cases are async and you console.log() in them, the logs would be mixed up when there are multiple test cases logging. It encourage a test pattern where one has to comment stuff out of the test to get any meaningful logs, which makes working on test failures harder and is not always practical e.g. when trying to decypher logs in the CI especially when investigating old flakes.

4. I also wonder how much longer it takes to run the tests structured with just blocks v.s. running them using node:test and whether that would make the tests run even longer if we do a mass migration. I did some testing locally and it suggests that not using node:test would make the test run 30% faster.

   'use strict';
   
   require('../common');
   const assert = require('assert');
   const test = require('node:test');
   
   test('test', () => {
     assert.strictEqual(typeof process.argv[0], 'string');
   });

   'use strict';
   
   require('../common');
   const assert = require('assert');
   const test = require('node:test');
   assert.strictEqual(typeof process.argv[0], 'string');

   hyperfine "out/Release/node test/parallel/test-b.js" "out/Release/node test/parallel/test-a.js"
   Benchmark 1: out/Release/node test/parallel/test-b.js
     Time (mean ± σ):      49.8 ms ±  26.6 ms    [User: 40.8 ms, System: 4.5 ms]
     Range (min … max):    42.7 ms … 156.3 ms    18 runs
   
   Benchmark 2: out/Release/node test/parallel/test-a.js
     Time (mean ± σ):      38.4 ms ±   2.5 ms    [User: 35.0 ms, System: 2.8 ms]
     Range (min … max):    36.8 ms …  50.6 ms    71 runs
   
   Summary
     'out/Release/node test/parallel/test-a.js' ran
       1.30 ± 0.70 times faster than 'out/Release/node test/parallel/test-b.js'
   

The test does not immediately stop when a test case fails

I think people will have different opinions on this. When there are a number of tests in a single file and I am relying on the CI for some platform other than macOS I actually want to see everything that passes and fails before pushing up another commit. Another general solution is to not test multiple things in a single file.

In some files the lines I have to ignore can be multi page and it can be very frustrating

The failures should be at the very bottom?

Windows and adds a lot of gibberish to the logs

This drives me crazy as well. I'm not sure if this is something specific to Jenkins that can be fixed or what. I haven't noticed it in GitHub action for example.

From a quick check, it does appear that Deno and Bun both use their own test runners in at least some places. Of course, I didn't check every test and I don't know what policies they might have in place around that.

it suggests that not using node:test would make the test run 30% faster.

I certainly expect using a test runner to add overhead that isn't there when not using a test runner. A few things to note: this sounds like web frameworks claiming to be x% faster at serving an empty response, but this often goes away once any real logic is introduced. Also, the test runner bootstraps itself when the first test is run, so I would expect subsequent test() calls to be faster.

I think people will have different opinions on this. When there are a number of tests in a single file and I am relying on the CI for some platform other than macOS I actually want to see everything that passes and fails before pushing up another commit.

It is indeed opinionated. I prefer to stop at the first failure, fix it, rerun, repeat.

Another general solution is to not test multiple things in a single file.

Definitely.

From a quick check, it does appear that Deno and Bun both use their own test runners in at least some places.

I honestly don't care how other projects run their tests and I think there is nothing wrong with our tests. I actually think the way our tests are currently written and run is one of the best part of the project. No bullshit, only the strictly needed code and dependencies. I am very convinced that a refactor to use node:test is a mistake. What does this bring to the project? There are multiple comments here and in other threads with concerns and real downsides. What are are the upsides? No specific Node.js API? That's a lie, for example see #56027. More details in the tests? Comments work better.

Instead of harmful refactors, I think that our time is better spent on investigating and fixing dozen of tests marked flaky and issues like #54918.

@lpinca I've mentioned couple of upsides of using node:test using the last TSC meeting. I recommend watching it since it also includes several different opinions from other TSC members as well.

I think people will have different opinions on this. When there are a number of tests in a single file and I am relying on the CI for some platform other than macOS I actually want to see everything that passes and fails before pushing up another commit.

There is a difference between running it on CI and locally. For example we use different output settings in the python test runner locally and in the CI as well. I think it would make more sense to align with what we do with the Python test runner: low-noise output when run locally, more details in the CI.

The failures should be at the very bottom?

The logs are not, and are in the middle of a bunch of passing test descriptions that you need to ignore - and when you are debugging a test failures, you mostly care about the assertion failure and logs, not the test descriptions (especially when there's no requirement about writing good test descriptions and they might just be random words that people put together...).

Also, this is assuming that only a single test is run. When multiple tests are failing during a run by the Python test runner, you are still going to have to scroll and fish out failure form pages of noise from later tests, instead of just looking at only relevant error information from all the tests that are failing.

At the very least, is there e.g. an environment variable that allows us to skip the logs about successful tests? It can be opinionated but personally I find them rather counter productive especially when I put any logs in the tests to aid debugging.

A few things to note: this sounds like web frameworks claiming to be x% faster at serving an empty response, but this often goes away once any real logic is introduced. Also, the test runner bootstraps itself when the first test is run, so I would expect subsequent test() calls to be faster.

Isn't that going to be in conflict with the recommendation of:

Another general solution is to not test multiple things in a single file.

? The more singled out tests we have, the more overhead we will introduce; but if we squeeze the tests in one file, the reporter will make the test failures harder to fish out from the noise?

Also many core tests are just very light weight - they are core tests, after all, and many of them don't test complex operations but just trivial edge cases (validation errors, simple calls to deps, pure JS computations etc.). In many cases the biggest part of the overhead is the bootstrapping overhead and the actual tests actually take less time than the bootstrap itself. Of course there are also tests that are more complex and async e.g. the http tests, which I think might benefit from using node:test. But I think we should also have some guidelines about when to avoid using node:test on other smaller tests (e.g. many of the util tests).

I am very convinced that a refactor to use node:test is a mistake.

I'm not the person advocating for a massive refactor and never have been. I will say that I would not put that in the top 10 all time worst mistakes made by the project, or even top 3 in the past 6 months 😄

Isn't that going to be in conflict with the recommendation of

Yes. Some of the tests are currently written with many tests in the same file. We should aim to change that.

Also many core tests are just very light weight...

Of course there are also tests that are more complex and async...

This. It would be nice to have more subtlety around this topic.

I was mostly just commenting here because I think these threads are mixing valid feedback, personal opinions, and things presented as fact that are incorrect based on people's opinions. I'll stop now.

@anonrig I've just finished watching it. The following arguments in favor of node:test are raised

1. The current tests depends on things other runtimes don't have like process hooks.
2. There are duplicate tests and by enforcing a description node:test will help find them.
3. node:test improves readability.

1. Yes, Node.js tests use Node.js features. This was also discussed in Discussion/Tracking: Adding more structure to Node.js' tests #54796.
2. I struggle to understand how node:test can help with duplicate tests. Nothing will change for file names and as Joyee pointed out, there is no requirement for the the description, it can be just few letters. Comments are a better alternative.
3. It might for some tests, but definitely not here https://github.com/nodejs/node/pull/55716/files or here https://github.com/nodejs/node/pull/55748/files or here https://github.com/nodejs/node/pull/55751/files.

I'm getting bored of repeating myself but don't add complexity where it is not needed, especially in tests. We only hurt ourselves with those refactors.

This. It would be nice to have more subtlety around this topic.

I think maybe a good measure about this might be: only the people maintaining what the test is testing get to choose what format the test should be written in, and forbid test-only changes from PRs that don't simultaneously change the features that the test is testing (unless they obviously had many commits in said feature or they reach consensus about this if there are multiple people maintaining said feature).

The reasoning is that those who maintain the feature being tested would be the ones impacted the most by the test change and they should have better judgement about whether the test format changes can make their life easier or harder. That's what I have been doing so far as well - if I am touching an existing test, I respect whatever the test format it is in and just follow it. But I otherwise would not author node:test tests by myself at this point because of the UX issues mentioned in #56027 (comment) , especially the noise about successful tests. I don't mind tests for things that I don't maintain to be rewritten since I normally don't break them anyway, and don't normally need to spend minutes fishing out useful information out of the noise from dozens of tests being broken in a single test run because I make a mistake in the implementation. And I trust people who maintain things that I am not familiar with to write better tests using node:test if they believe this makes their life eaiser.

1. The test does not immediately stop when a test case fails

This is exactly what I want

1. Is there a way to make that the default in the test reporter we use to reduce the noise level?

I've seen other test runners with a reporter that outputs only on failure. I think that would be good here and addresses the issue Joyee raised (CI output is indeed quite long). Writing to stdout is also not free, so that should help with performance too.

2. 1 is amplified in the CI, where the special unicode characters used by the test reporters are not decoded properly on e.g. Windows and adds a lot of gibberish to the logs.

I think the dot reporter won't have that issue.

3. IIUC if the tests cases are async and you console.log() in them, the logs would be mixed up when there are multiple test cases logging.

Oof, yes, this is not ideal.

My personal experience with node:test in the core repo is that it outputs super long failures because a single failure is printed twice: #56316. I find this really hurts the experience.

It is indeed opinionated. I prefer to stop at the first failure, fix it, rerun, repeat.

I absolutely do not want this in the strongest of terms. IMO this an atrocious DX. I do not have a Windows PC, which is the least reliable platform. Whenever there are cross-platform issues, it's always Windows. If CI stops on the first of 50 failures and I have to discover and fix them one by one, with CI taking 4 hours each run? Absolutely. Not. I would never send another PR again.

These concerns seem very actionable though 🙂

I expect we could even fairly easily facilitate the kill-all-on-first-failure via a label (I think it shouldn't be the default though, and definitely not the only option).

For the output noise, I don't know enough about test reporters, but if we can't configure an existing one for a quiet mode, could we fork one to facilitate it?

@cjihrig are you against using the test runner itself in core?

Not at all. I have been happily using it.

@joyeecheung it seems the experimental-permission bug is fixed on main. do you have any objections to removing your block, or do you have any recommendations on how to move forward? sorry, if i'm asking you to repeat. i'm trying to understand if there is a path forward. if not, i'm happy to close this pr.

@anonrig I believe the objection is related to general output, not just for that specific test. @Ceres6 has been looking into that in #56739, #56468, and #56438, but the work is not complete yet.

@anonrig I believe the objection is related to general output, not just for that specific test. @Ceres6 has been looking into that in #56739, #56468, and #56438, but the work is not complete yet.

Glad to hear that there is a path forward. Let's wait for this awesome work.

Yes the block is not specific to that test, for more examples see #56751 - this means that by using node:test there will be more and more stack traces looking like this pop up in Jenkins until it gets fixed.

Yes the block is not specific to that test, for more examples see #56751 - this means that by using node:test there will be more and more stack traces looking like this pop up in Jenkins until it gets fixed.

@joyeecheung Would you mind making a checklist/todo list for unblocking your review. I'll be happy to avoid wasting any of your time and complete them one by one when the time comes (along with the PRs that are completing them)

Also, I think #56027 (comment) should be applied to prevent #56671 from happening again. If you really don't want to discourage modifying existing tests, then given the disruptions in output that can be caused by the new test reporter I think it warrants requirement of

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.

(But in general I think substantially modifying tests purely for stylistic preference is just not a good idea, even if we have consensus about the style, because with our workflow that would just make backports harder - is letting in some style changes really that important compared to not impeding backports of important bug fixes and features?)

Would you mind making a checklist/todo list for unblocking your review. I'll be happy to avoid wasting any of your time and complete them one by one when the time comes (along with the PRs that are completing them)

Basically

1. Deliberately break test-runner-mocking.js (for example just revert test: use --permission instead of --experimental-permission #56685) or test-without-async-context-frame.js (as long as they still exist and are still written using node:test, otherwise I suggest breaking some that use it under test/es-module, should be easy to find if you go with the biggest files there)
2. Run the Jenkins CI and see what it looks like in the Jenkins reports, in particular on Windows and macOS as those two were reported to be broken
3. Show that the report is formatted correctly and not look like what I posted above or what's pasted in Mark test-without-async-context-frame flaky on Windows #56751