Review requested:

• @nodejs/test_runner

Won’t this make it harder for tooling to parse out the lack of failures?

Can you please rebase to remove the additional commit?

Won’t this make it harder for tooling to parse out the lack of failures?

they can do that either by counting test:fail events or by checking the exit code

Also it would help if this PR had an explanation. If all tests pass, what failing tests are you hiding?

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

We would need better snapshot/comparison functionality (perhaps as part of node:assert?) in order to do that. I do desire to re-introduce assert.snapshot once I have time, but the lack of that ability should not block the introduction of other capabilities and features just because our testing tools are not cutting edge.

Also it would help if this PR had an explanation. If all tests pass, what failing tests are you hiding?

Sorry, I thought the code is self-explanatory. we added a list of failing tests at the end of the report, but if all tests pass there is still a "failing tests" title. that title is what the commit message is referring to. if you have a better suggestion for the commit message I am ok with changing it

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

Open an issue to track that probably and suggest it as a bigger policy change?

Can we please stop adding new tests to test/message? I feel like all test runner tests should be in one folder, so you can test that subsystem by itself quickly; and test/message should be deprecated, as it does comparisons in Python and it's harder to debug those types of tests.

We would need better snapshot/comparison functionality (perhaps as part of node:assert?) in order to do that. I do desire to re-introduce assert.snapshot once I have time, but the lack of that ability should not block the introduction of other capabilities and features just because our testing tools are not cutting edge.

We can do it today via assert.match. See #41352, where I moved all the ESM tests out of test/message into test/es-module. You could follow that same pattern.

As for larger policy change, sure we can discuss it, but in the meantime why make the problem worse? As far as I know only the test runner team is adding new tests into test/message lately, so if those folks find a pattern that works for them that stops adding to test/message, that would be future-proof.

Also it would help if this PR had an explanation. If all tests pass, what failing tests are you hiding?

Sorry, I thought the code is self-explanatory. we added a list of failing tests at the end of the report, but if all tests pass there is still a “failing tests” title. that title is what the commit message is referring to. if you have a better suggestion for the commit message I am ok with changing it

Maybe just add a before/after of the output that’s changing in the PR description? I looked in the PR diff expecting to see a test assertion changing, now that the “Test failed” label was removed, and was surprised not to find one.

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

We can do it today via assert.match. See #41352, where I moved all the ESM tests out of test/message into test/es-module. You could follow that same pattern.

this pattern is not feasible for the test runner tests, since the output of the test runner is an interface consumed by node users, we try to test all of it - which makes the amount of output we test significantly larger than other areas under tests/messsage.
testing all the possible combinations of output generated by the test runner using regex is not something maintainable AFAIK.
I am in favor of migrating away from tools/test.py but we need a better alternative than assert.match.

this pattern is not feasible for the test runner tests, since the output of the test runner is an interface consumed by node users, we try to test all of it - which makes the amount of output we test significantly larger than other areas under tests/messsage.

I think this is equivalent?

import { mustCall } from '../common/index.mjs';
import { match, strictEqual } from 'node:assert';
import { spawn } from 'node:child_process';
import { execPath } from 'node:process';

const source = `import { it } from 'node:test';
it('should pass', () => {});`;

const expectedOutput = `* should pass *(*ms)*
*ℹ tests 1*
*ℹ suites 0*
*ℹ pass 1*
*ℹ fail 0*
*ℹ cancelled 0*
*ℹ skipped 0*
*ℹ todo 0*
*ℹ duration_ms *
`;

const child = spawn(execPath, [
  '--no-warnings',
  '--test-reporter=spec',
  `--input-type=module`,
  '--eval',
  source,
]);

let stdout = '';
let stderr = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
  stdout += data;
});
child.stderr.setEncoding('utf8');
child.stderr.on('data', (data) => {
  stderr += data;
});
child.on('close', mustCall((code, _signal) => {
  strictEqual(code, 0);
  strictEqual(stderr, '');

  const expectedLines = expectedOutput.split('\n');
  stdout.split('\n').forEach((line, lineIndex) => {
    const expectedLine = expectedLines[lineIndex];
    match(line, new RegExp(expectedLine.replaceAll('*', '.*')));
  });
}));

There’s a lot more boilerplate here, but it could be simplified with some helpers added to test/common; and if you don’t like putting the source code in a string you could save it as a fixture. And of course it could be slightly faster if the expected output were written as a regex in the first place (or at least with .* instead of *) but I wanted to match the .out file in this PR.

As for larger policy change, sure we can discuss it, but in the meantime why make the problem worse? As far as I know only the test runner team is adding new tests into test/message lately, so if those folks find a pattern that works for them that stops adding to test/message, that would be future-proof.

There is an existing process/prior art which this PR uses. The burden of doing refactors should not be on individual PR authors and if you want to drive a bigger policy change I think the better place to discuss it is not on an individual PR.

You have been on the other side of people leaving comments on PRs asking for people to do extra (good, important) work not necessarily related the PR before. Giving you your (good) advice back - doing the technical bits and driving the effort goes a long way and is generally a lot better received than "don't do X" comments when there is prior art of X. You are of course welcome to interact in any positive way you see fit - I'm just trying to explain my perspective on what works well with these sort of communications.

Il try putting some time into developing an internal tool we can use for this, and hopefully, one day expose it

Thanks. I guess for now my question is, what about the approach in this PR (the message test) do you prefer over the alternative I proposed above? As far as I can tell the alternative tests exactly the same output, as completely as the .out test does, and is just as maintainable because the expected output string is exactly the same. You could put the source input and the expected output in separate files and import them if that makes things more comprehensible, but I put it all together for the purposes of an example.

Update: on my machine, the alternative proposal ran 3x faster than the message version:

hyperfine --warmup 3 --runs 10 \
  'tools/test.py --mode=release message/test_runner_spec_reporter_successful.js' \
  './node ./test/parallel/test-runner-spec-reporter-successful.mjs'

Benchmark 1: tools/test.py --mode=release message/test_runner_spec_reporter_successful.js
  Time (mean ± σ):     973.9 ms ±  18.4 ms    [User: 560.7 ms, System: 152.3 ms]
  Range (min … max):   951.3 ms … 994.4 ms    10 runs

Benchmark 2: ./node ./test/parallel/test-runner-spec-reporter-successful.mjs
  Time (mean ± σ):     278.8 ms ±   4.1 ms    [User: 229.0 ms, System: 48.5 ms]
  Range (min … max):   272.3 ms … 288.6 ms    10 runs

Summary
  './node ./test/parallel/test-runner-spec-reporter-successful.mjs' ran
    3.49 ± 0.08 times faster than 'tools/test.py --mode=release message/test_runner_spec_reporter_successful.js'

And of course it should run faster still if it wasn’t generating the regexes dynamically, but that’s a performance/readability tradeoff question.

As far as I can tell the alternative tests exactly the same output, as completely as the .out test does, and is just as maintainable because the expected output string is exactly the same. You could put the source input and the expected output in separate files and import them if that makes things more comprehensible, but I put it all together for the purposes of an example.

just off the top of my head, the process of producing the .out file is currently straightforward (./node test.js > test.out), but in your suggestion, it is not.
I am not saying it is impossible or hard, it is just not at the level for use in this PR, and even if it was I would prefer it to be in its own PR

just off the top of my head, the process of producing the .out file is currently straightforward (./node test.js > test.out)

Except for the fact that you need to go through the generated .out file and replace a bunch of things with asterisks?

And of course you could still use the .out file, generated however you’re doing it currently, and replace my const expectedOutput = line above with const expectedOutput = fs.readFileSync('./test_runner_spec_reporter_successful.out').

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

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

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

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

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

- Loading data for nodejs/node/pull/47370
✔  Done loading data for nodejs/node/pull/47370
----------------------------------- PR info ------------------------------------
Title      test_runner: hide failing tests title when all tests pass (#47370)
Author     Moshe Atlow  (@MoLow)
Branch     MoLow:spec-show-failing-only-when-failing -> nodejs:main
Labels     author ready, needs-ci, dont-land-on-v14.x, test_runner
Commits    1
 - test_runner: hide failing tests title when all tests pass
Committers 1
 - Moshe Atlow 
PR-URL: https://github.com/nodejs/node/pull/47370
Reviewed-By: Colin Ihrig 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/47370
Reviewed-By: Colin Ihrig 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test_runner: hide failing tests title when all tests pass
   ℹ  This PR was created on Sun, 02 Apr 2023 07:14:07 GMT
   ✔  Approvals: 3
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/47370#pullrequestreview-1368114083
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/47370#pullrequestreview-1368115172
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/47370#pullrequestreview-1368130734
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-04-04T04:44:06Z: https://ci.nodejs.org/job/node-test-pull-request/50898/
- Querying data for job/node-test-pull-request/50898/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Landed in af8ed02