node:test outputs invalid TAP if there is a newline in the test name

[kmannislands]

Version

19.0.1

Platform

Darwin Kernel Version 21.3.0: Wed Jan 5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T8101 arm64

Subsystem

node:test

What steps will reproduce the bug?

import assert from 'node:assert';
import { test } from "node:test";

const testName = 'anything with newline \nnot ok';

test(testName, () => {
    assert.equal(true, true);
});

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

Requires a test name with a newline character. Suspect other characters should be escaped as well to prevent problems I stumbled across this when writing something like:

const testCases = [
  'TAP version 13\n',
  'TAP version 14\n',
];

for (const testStr of testCases) {
  it(`parses testString "${testStr}" correctly`, () => {
    // ...
  });
}

What is the expected behavior?

The node:test TAP producer should be robust to test names and produce valid TAP. I would recommend escaping \n (and probably other) characters when rendering test names to TAP.

The output should be:

# Subtest: anything with newline\nnot ok
ok 1 - anything with newline\nnot ok
  ---
  duration_ms: 1.084417
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 3.85175

What do you see instead?

The newline is not escaped. In this case I'm using not ok after the newline to drive home the problems as its particularly nasty. The output of the supplied example is:

TAP version 13
# Subtest: anything with newline 
not ok
ok 1 - anything with newline 
not ok
  ---
  duration_ms: 1.084417
  ...
1..1
# tests 1
# pass 1
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 3.85175

Depending on the contents after any \ns in test names, this could result in totally invalid TAP, false diagnostics or misleading TAP output like above.

Additional information

it seems that the escaping should also occur in diagnostic serialization code. In the example output above, the newline in the diagnostic was not escaped in the diagnostic either.

Interestingly, # seems to be escaped already so:

const testName = 'anything with newline # todo (not really)';

test(testName, () => {
    assert.equal(true, false);
});

produces good output of:

TAP version 13
# Subtest: anything with newline \# todo (not really)
not ok 1 - anything with newline \# todo (not really)
  ---
  duration_ms: 0.983167
  failureType: 'testCodeFailure'
  error: 'true == false'
  code: 'ERR_ASSERTION'
  stack: |-
    TestContext.<anonymous> (file:///Users/kieranmann/github/2ma-blog/src/parser-combinator/example.js:7:12)
    Test.runInAsyncScope (node:async_hooks:203:9)
    Test.run (node:internal/test_runner/test:511:25)
    Test.start (node:internal/test_runner/test:438:17)
    test (node:internal/test_runner/harness:131:18)
    file:///Users/kieranmann/github/2ma-blog/src/parser-combinator/example.js:6:1
    ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    async Promise.all (index 0)
    async ESMLoader.import (node:internal/modules/esm/loader:518:24)
    async loadESM (node:internal/process/esm_loader:102:5)
  ...
1..1
# tests 1
# pass 0
# fail 1
# cancelled 0
# skipped 0
# todo 0
# duration_ms 3.777209

@nodejs/test_runner

[sevkioruc]

Hey. I would like to work on it. Is it available?

[daltonna]

Is this still available to work on? It's my first issue but I think I should be able to finish it up pretty quickly.

[MoLow]

Yep :) go ahed!

[sevkioruc]

@MoLow can you assign this issue to me? :)

[MoLow]

@daltonna If you are looking for some good first issue Issues that are suitable for first-time contributors. , you can go over comments in #45326 that were left for follow-up and implement them

[sevkioruc]

Hey @MoLow, how can i test changes?

[MoLow]

you can run any test using ./node test/path/to/the/test.js are using tools/test.py test/path/to/the/test.js
see the contribution guide: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#the-process-of-making-changes

[sevkioruc]

Hey @MoLow,
I have just created a PR about this issue in here.
It works but i think it should be more cleaner resolve.
In addition to i should find a solution how it include some escape characters such as \r, \t, \b, \f, \v, \', \", too.

I had tried that create an array like that const escapedCharacters = ['\n', '\t', '\b' ...] and loop in here. I wanted to call StringPrototypeReplaceAll function with each escape characters. Unfortunately I couldn't like that. It may be cleaner solving.

What do you think about it? Do you have any suggestion for me?
Thanks.