Test count may not be as useful as it could be

[willm]

Version

v18.3.0

Platform

Darwin willmunn-2 20.6.0 Darwin Kernel Version 20.6.0: Tue Feb 22 21:10:41 PST 2022; root:xnu-7195.141.26~1/RELEASE_X86_64 x86_64

Subsystem

test_runner

What steps will reproduce the bug?

I wanted to try the new test runner, being a long term tape user, this was pretty exciting as the apis are very similar. It seems the test summary at the end of the tap output is recording test counts as the number of test files. I had a go at a simple fizz buzz implementation:

import test from 'node:test';
import assert from 'assert';

import test from 'node:test';
import assert from 'assert';

const fizzbuzz = (num) => {
  const special = [
    {condition: num % 3 === 0, output: 'fizz'},
    {condition: num % 5 === 0, output: 'buzz'},
  ];
  const specialAnswer = special.reduce((output, x) => x.condition ? output + x.output : output, '');
  return specialAnswer ? specialAnswer : num;
};

test('1 should return 1', t => {
  assert.equal(fizzbuzz(1), 1);
});

test('3 should return fizz', t => {
  assert.equal(fizzbuzz(3), 'fizz');
});

test('5 should return buzz', t => {
  assert.equal(fizzbuzz(5), 'buzz');
});

test('15 should return fizzbuzz', t => {
  assert.equal(fizzbuzz(5), 'buzz');
});

test('16 should return 16', t => {
  assert.equal(fizzbuzz(16), 16);
});

node --test returns the following output:

TAP version 13
ok 1 - /Users/will.munn/code/experiments/node18/tests/test.js
  ---
  duration_ms: 0.07915145
  ...
1..1
# tests 1
# pass 1
# fail 0
# skipped 0
# todo 0
# duration_ms 0.126770722

note that the the output suggests that only 1 test has been run. After adding another test file, I noticed that the output is actually counting the number of files, not the number of test() blocks or the amount of assertions.

If I add a test.skip to one of the tests, it will still report that there are 0 skipped tests.

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

I personally think the best solution would be that The TAP output reports the number of test() blocks rather that the number of test files. So for my example:

TAP version 13
ok 1 - /Users/will.munn/code/experiments/node18/tests/test.js
  ---
  duration_ms: 0.07915145
  ...
1..5
# tests 5
# pass 5
# fail 0
# skipped 0
# todo 0
# duration_ms 

What do you see instead?

TAP version 13
ok 1 - /Users/will.munn/code/experiments/node18/tests/test.js
  ---
  duration_ms: 0.07915145
  ...
1..1
# tests 1
# pass 1
# fail 0
# skipped 0
# todo 0
# duration_ms 0.126770722

Additional information

Note that my suggestion is actually different to what the tape module does, this reports counts based on the number of assertions. I personally feel that number of test blocks makes the most sense.

[tniessen]

cc @nodejs/test_runner @cjihrig

[cjihrig]

This could be improved by parsing the standard output of the child processes that run each test file. There is a TODO in the code regarding implementing a TAP parser. IMO that is the best approach, but also requires the most work. If someone really wanted to, they could implement more light weight parsing that, for example, only parses the ending summary lines of each child process.

[manekinekko]

This could be improved by parsing the standard output of the child processes that run each test file. There is a TODO in the code regarding implementing a TAP parser.

@cjihrig happy to work on this. Could you point me to the TODO comment?

[aduh95]

@manekinekko

node/lib/internal/main/test_runner.js

Lines 108 to 109 in adaf602

	// TODO(cjihrig): Implement a TAP parser to read the child's stdout
	// instead of just displaying it all if the child fails.

[manekinekko]

Thank you @aduh95. I'm gonna work on this 👍

[MoLow]

I took a little look at this issue, and I was wondering if this can be approached by adding support for additional reporters other than the current TapStream,
this will allow adding a plain JSON reporter that is already very easy to serialize/deserialize
possibly another reporter can be using worker_threads postMessage to share data between the parent test and its children

this approach can also enable passing custom reporters that are implemented in userland as an option to the root test runner
(assuming we define a common interface for a reporter)
WDYT?

[manekinekko]

I agree with @MoLow!

this approach can also enable passing custom reporters that are implemented in userland as an option to the root test runner
(assuming we define a common interface for a reporter)

I am in favor of this as well.

[MoLow]

@cjihrig I'd love to hear your feedback regarding the reporter's approach.
@manekinekko would you like to implement some kind of parentPort.postMessage reporter?

[cjihrig]

I took a little look at this issue, and I was wondering if this can be approached by adding support for additional reporters other than the current TapStream

@cjihrig I'd love to hear your feedback regarding the reporter's approach.

In my head I always pictured the test runner outputting TAP via a stream and other reporters being implemented as transform streams (this part kind of depends on having the TAP parser in place).

[manekinekko]

would you like to implement some kind of parentPort.postMessage reporter?

@MoLow I am happy to give it a shot. Could you provide a high-level design so I have a bigger picture of the different pieces?

In my head I always pictured the test runner outputting TAP via a stream and other reporters being implemented as transform streams (this part kind of depends on having the TAP parser in place).

This would totally be doable once the TAP parser is done. The current AST has all the info to run it through a codegen and spit out virtually any format.

[MoLow]

In my head I always pictured the test runner outputting TAP via a stream and other reporters being implemented as transform streams (this part kind of depends on having the TAP parser in place).

I totally understand why implementing reporters as a transform stream makes sense,

my two arguments don't necessarily contradict that:

• JSON is much more natural in javascript environments than TAP and can already be de/serealized out of the box, so I question TAP being the default output for a test?
  I assume that it can be very hard to write a TAP parser that beats JSON.parse, both in terms of performance and of usability.
  IMHO it makes more sense that the base stream outputs JSON objects and then use a transform stream to output TAP (in case one wants tap output and not something else)
• the implementation of --test might leverage the ability to share memory between workers - which might be a little more efficient than serializing -> parsing -> serializing again
  I am not sure about this one as multiple child processes can leverage multiple CPUs, and this wont allow that

[ljharb]

Test runners in the ecosystem don't output JSON by default - ever, afaik. A number of them output TAP by default, though. TAP is the best choice.