Skip to content

Only print failing assertion once with spec reporter #56316

Closed
Closed
Only print failing assertion once with spec reporter#56316
Labels
feature requestIssues requesting new Node.js features.test_runnerIssues and PRs related to the test runner subsystem.

Description

@legendecas
legendecas
opened on Dec 19, 2024

What is the problem this feature will solve?

The default test runner reporter spec prints an error twice. For example, with the following simple test:

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

test('foo', () => {
  throw new Error('foo');
});

The command node --test test.js, or node test.js prints:

✖ foo (0.308375ms)
  Error: foo
      at TestContext.<anonymous> (/Users/cwu631/Developer/nodejs/node/test.js:4:9)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:931:25)
      at Test.start (node:internal/test_runner/test:829:17)
      at startSubtestAfterBootstrap (node:internal/test_runner/harness:297:17)

ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 50.714375

✖ failing tests:

test at t.js:3:1
✖ foo (0.308375ms)
  Error: foo
      at TestContext.<anonymous> (/Users/cwu631/Developer/nodejs/node/test.js:4:9)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:931:25)
      at Test.start (node:internal/test_runner/test:829:17)
      at startSubtestAfterBootstrap (node:internal/test_runner/harness:297:17

A single test caused two identical errors in the output.

When there are 2 or more test failures, the output is really verbose to be inspected by a human -- are these the same failures?

This hurts the experience with node:test in node core development. A change can fail many cases in the test, and every test failure is printed twice, causing super long outputs.

What is the feature you are proposing to solve the problem?

Only print a single test identical failure detail exactly once. This reduces the length of the output on failure and allow people to focus, rather than been distracted by the verbose long output.

✖ foo (0.308375ms)
  Error: foo
      at TestContext.<anonymous> (/Users/cwu631/Developer/nodejs/node/test.js:4:9)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:931:25)
      at Test.start (node:internal/test_runner/test:829:17)
      at startSubtestAfterBootstrap (node:internal/test_runner/harness:297:17)

ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 50.714375

✖ failing tests:

test at t.js:3:1
✖ foo (0.308375ms)

What alternatives have you considered?

Don't change the status quo.

Activity

  1. legendecas
    added
    feature requestIssues requesting new Node.js features.
    on Dec 19, 2024
  2. github-project-automation
    added this to Node.js feature requestson Dec 19, 2024
  3. github-project-automation
    moved this to Awaiting Triage in Node.js feature requestson Dec 19, 2024
  4. legendecas
    added
    test_runnerIssues and PRs related to the test runner subsystem.
    on Dec 19, 2024

  5. pmarchini commented on Dec 19, 2024

    @pmarchini
    pmarchini
    on Dec 19, 2024
    Member

    Hey @legendecas,
    I completely agree with you.
    I was thinking the same a few days ago while reviewing some tests output.

    I’ll take a look if nobody else gets to it before me

  6. cjihrig commented on Dec 19, 2024

    @cjihrig
    cjihrig
    on Dec 19, 2024
    Contributor

    Definitely +1 to only printing the failure once. Which one will you keep?

    For context, the list at the end was added because people complained about scrolling back to see failures. I would see which approach other runners take and do what seems to be most popular.

    This hurts the experience with node:test in node core development.

    FWIW, I think we should make this particular change, but I don't think we should tailor the spec reporter output specific to Node core. I think a better way forward is #52189. We can use a custom reporter instead of the spec one and make it look exactly how we want for core.

  7. legendecas commented on Dec 19, 2024

    @legendecas
    legendecas
    on Dec 19, 2024
    MemberAuthor

    This issue is generic about the spec reporter. I complain about it is because I use it in node core development most often. But I don't think this is specific to node core. Agree that we can create a dedicated reporter for node core and this is still a worth change.

    Which one will you keep?

    Maybe we can keep the last one, it could look like this:

    (assuming lots of passing tests...)
...
✖ foo (0.308375ms)
...
(assuming lots of passing tests...)

ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 1
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 50.714375

✖ failing tests:

test at t.js:3:1
✖ foo (0.308375ms)
  Error: foo
      at TestContext.<anonymous> (/Users/cwu631/Developer/nodejs/node/test.js:4:9)
      at Test.runInAsyncScope (node:async_hooks:211:14)
      at Test.run (node:internal/test_runner/test:931:25)
      at Test.start (node:internal/test_runner/test:829:17)
      at startSubtestAfterBootstrap (node:internal/test_runner/harness:297:17

  8. Llorx commented on Dec 21, 2024

    @Llorx
    Llorx
    on Dec 21, 2024
    Contributor

    I'm also with the last one. I will pickup this issue this incoming week as I'm on PTO.

  9. legendecas
    closed this as completedon Jan 24, 2025
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

    feature requestIssues requesting new Node.js features.test_runnerIssues and PRs related to the test runner subsystem.

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions