Skip to content

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

Description

@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

Activity

  1. added
    good first issueIssues that are suitable for first-time contributors.
    test_runnerIssues and PRs related to the test runner subsystem.
    on Nov 10, 2022
  2. scuro-dev commented on Nov 10, 2022

    @scuro-dev

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

  3. daltonna commented on Nov 10, 2022

    @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.

  4. MoLow commented on Nov 10, 2022

    @MoLow
    Member

    Yep :) go ahed!

  5. scuro-dev commented on Nov 10, 2022

    @scuro-dev

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

  6. MoLow commented on Nov 10, 2022

    @MoLow
    Member

    @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

  7. scuro-dev commented on Nov 12, 2022

    @scuro-dev

    Hey @MoLow, how can i test changes?

  8. MoLow commented on Nov 12, 2022

    @MoLow
    Member

    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

  9. scuro-dev commented on Nov 20, 2022

    @scuro-dev

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    good first issueIssues that are suitable for first-time contributors.test_runnerIssues and PRs related to the test runner subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions