Tap parser fails if a test logs a number

[connor4312]

Version

v19.3.0

Platform

Linux Helios 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

test_runner

What steps will reproduce the bug?

Have a test file containing

const { it } = require("node:test");

it("hello world", async () => {
  console.log(1234);
})

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

100% reproduction

What is the expected behavior?

The test passes when running node --test

What do you see instead?

TAP version 13
# Subtest: /home/connor/Github/nodejs-testing/example/basic/b.test.js
not ok 1 - /home/connor/Github/nodejs-testing/example/basic/b.test.js
  ---
  duration_ms: 41.664198
  failureType: 'uncaughtException'
  error: "Cannot read properties of undefined (reading 'value')"
  code: 'ERR_TEST_FAILURE'
  stack: |-
    #Plan (node:internal/test_runner/tap_parser:653:20)
    #TAPDocument (node:internal/test_runner/tap_parser:552:28)
    #parseChunk (node:internal/test_runner/tap_parser:274:35)
    #parseTokens (node:internal/test_runner/tap_parser:249:23)
    TapParser.parse (node:internal/test_runner/tap_parser:105:24)
    TapParser._transform (node:internal/test_runner/tap_parser:187:10)
    Transform._write (node:internal/streams/transform:175:8)
    writeOrBuffer (node:internal/streams/writable:392:12)
    _write (node:internal/streams/writable:333:10)
    Writable.write (node:internal/streams/writable:337:10)
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 46.015529

Additional information

This happens for any numberic value, so console.log('1234') will produce the same result

[cjihrig]

cc @manekinekko

[pulkit-30]

While testing my PR, I observed something unexpected,
when i run code

test('testing', () => {
	console.log(123);
})

test('testing', () => {
	console.log("123");
})

it throws error, saying error: "Cannot read properties of undefined (reading 'value')", that points here

but when I am running code

test('testing', () => {
        console.log(123, "..");
	console.log(123, "..", 123);
})

then it pass, but doesn't print the expected output

and with code

test('testing', () => {
	console.log("anytext 123");
	console.log("anytest", 123);
})

it passed as expected.

what I found, this happening because of logic written inside #Plan() function.

your views @cjihrig @manekinekko

[cjihrig]

@pulkit-30 writing to stdout has the potential to interfere with TAP parsing because TAP also uses stdout. I believe that's what is happening here. I don't think we can really avoid that aspect, but the parser should be able to handle these cases without crashing.

[connor4312]

It would be very nice if output during tests could be redirected as e.g. TAP comments. I'm off and on working on building a node:test extension for VS Code and ended up implementing that by hand in a wrapper script

[cjihrig]

It would be very nice if output during tests could be redirected as e.g. TAP comments.

I'm not sure what version(s) you're testing on, but that is the behavior since #43525. This issue is just one of a handful of bugs in the parser.

[connor4312]

Ah, got it. That only happens when running node --test test.js, not when running simply node test.js.

[manekinekko]

@pulkit-30 writing to stdout has the potential to interfere with TAP parsing because TAP also uses stdout. I believe that's what is happening here. I don't think we can really avoid that aspect, but the parser should be able to handle these cases without crashing.

The parser throws on invalid syntax by design. We can however catch those exceptions (in the runner) and decide whether we silence them or how to handle invalid syntax. @cjihrig @MoLow any suggestions?

[cjihrig]

The parser throws on invalid syntax by design.

I don't think that is the correct behavior, based on this text from the spec:

Any line that is not a valid version, plan, test point, YAML diagnostic, pragma, a blank line, or a bail out is invalid TAP.
A Harness may silently ignore invalid TAP lines, pass them through to its own stderr or stdout, or report them in some other fashion. However, Harnesses should not treat invalid TAP lines as a test failure by default.

Right now, it looks like we are treating it as a failure. We're also surfacing an error message (Cannot read properties of undefined (reading 'value')) that seems much more like a bug in Node than an intentional error message.

We're also losing valid data from that file. Given the following file, I would expect test 1 and test 3 at least to pass.

const test = require('node:test');

test('test 1');

test('test 2', () => {
  console.log('1234');
});

test('test 3');

In my opinion we should ignore invalid TAP, but continue parsing.

EDIT: By ignore invalid TAP, I really mean display it as a diagnostic and ignore it from a parsing perspective.