Skip to content

Conversation

vassudanagunta
Copy link
Contributor

@vassudanagunta vassudanagunta commented Sep 1, 2025

While trying to understand the underlying logic of the built-in test reporters, necessary because documentation of the semantics of TestsStream is minimal, I was thoroughly confused by this code in formatTestReport:

const error = showErrorDetails ? formatError(data.details?.error, indent) : '';
const err = hasChildren ?
  (!error || data.details?.error?.failureType === 'subtestsFailed' ? '' : `\n${error}`) :
  error;

...and not just because of the obfuscation. It made sense that if data.details?.error?.failureType === 'subtestsFailed', the error should not be reported against the parent test, and even that this condition is only tested if hasChildren is true. But all callers that do pass in hasChildren (which otherwise defaults to false) also pass in  showErrorDetails: false, so that part of the convoluted nested ternary never even fires.

Ok, I thought, maybe formatTestReport represents an API the current built-in reporters do not use fully. But that seems a little odd because it is internal. So I reduced it to its essence, as currently used. I did it in four steps because I'm old and need to break things down more than you young'uns might need. Each step is a commit, with explanation in the commit message, with each commit passing all tests.

The upshot is the above gets reduced to

const err = showErrorDetails && data.details?.error ? 
  formatError(data.details.error, indent) : '';

along with an easier to understand formatTestReport function signature and removal of more dead code in the spec reporter.

reporter/utils.js formatTestReport:
1. hasChildren and showErrorDetails are never both true in
   existing calls.
2. Thus if hasChildren is true, showErrorDetails is false.
3. So `|| data.details?.error?.failureType === 'subtestsFailed'`
   is irrelevant.
4. And `\n${error}` never occurs.

Even though all tests pass after this commit, what if future reporter
code might make calls where both hasChildren and showErrorDetails
are true? I will address this in the last commit of this PR.
Trust me for now.
1. formatTestReport: inline the `const error`
2. move the check for `data.details?.error` out of formatError
   to its sole caller, formatTestReport, which makes more sense.
3. collapse the nested ternary expressions in formatTestReport to one.
reporter/utils.js formatTestReport:
1. consider again that hasChildren and showErrorDetails are never
   both true in existing calls to formatTestReport.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Sep 1, 2025
Copy link

codecov bot commented Sep 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.92%. Comparing base (45a8b55) to head (44ccad8).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59700      +/-   ##
==========================================
- Coverage   89.93%   89.92%   -0.02%     
==========================================
  Files         667      667              
  Lines      196792   196767      -25     
  Branches    38426    38405      -21     
==========================================
- Hits       176983   176939      -44     
- Misses      12233    12242       +9     
- Partials     7576     7586      +10     
Files with missing lines Coverage Δ
lib/internal/test_runner/reporter/spec.js 96.49% <100.00%> (-0.15%) ⬇️
lib/internal/test_runner/reporter/utils.js 94.89% <100.00%> (+1.76%) ⬆️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

1. formatTestReport: hasChildren arg is unused
2. consider that the existing calls to formatTestReport break down into
   two types:
    - print a line in the test result tree without error details (spec)
    - print error details (spec and dot)

   i.e. whether or not to showErrorDetails.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants