Repository navigation
Test count may not be as useful as it could be #43344
Description
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.
Activity
tniessen commentedon Jun 8, 2022
cc @nodejs/test_runner @cjihrig
cjihrig commentedon Jun 10, 2022
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 commentedon Jun 10, 2022
@cjihrig happy to work on this. Could you point me to the TODO comment?
aduh95 commentedon Jun 10, 2022
@manekinekko
node/lib/internal/main/test_runner.js
Lines 108 to 109 in adaf602
manekinekko commentedon Jun 10, 2022
Thank you @aduh95. I'm gonna work on this 👍
MoLow commentedon Jul 5, 2022
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_threadspostMessage to share data between the parent test and its childrenthis 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 commentedon Jul 12, 2022
I agree with @MoLow!
I am in favor of this as well.
MoLow commentedon Jul 12, 2022
@cjihrig I'd love to hear your feedback regarding the reporter's approach.
@manekinekko would you like to implement some kind of
parentPort.postMessagereporter?cjihrig commentedon Jul 12, 2022
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 commentedon Jul 12, 2022
@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?
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 commentedon Jul 12, 2022
I totally understand why implementing reporters as a transform stream makes sense,
my two arguments don't necessarily contradict that:
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)
--testmight leverage the ability to share memory between workers - which might be a little more efficient than serializing -> parsing -> serializing againI am not sure about this one as multiple child processes can leverage multiple CPUs, and this wont allow that
ljharb commentedon Jul 12, 2022
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.
cjihrig commentedon Jul 12, 2022
I also think that we need to think in terms of streaming output, which
JSON.parse()would not handle. Streaming JSON parsing is possible, but I still think TAP is the right choice for the default output format.MoLow commentedon Jul 12, 2022
ok, I've got your point :)
5 remaining items
test_runner: add initial TAP parser
test_runner: add initial TAP parser
feat: add initial TAP parser
feat: add initial TAP parser
feat: add initial TAP parser