Skip to content

fix/coverage-ignore-brda-61586#62264

Closed
PradSharma554 wants to merge 3 commits intonodejs:mainfrom
PradSharma554:fix/coverage-ignore-brda-61586
Closed

fix/coverage-ignore-brda-61586#62264
PradSharma554 wants to merge 3 commits intonodejs:mainfrom
PradSharma554:fix/coverage-ignore-brda-61586

Conversation

@PradSharma554
Copy link

@PradSharma554 PradSharma554 commented Mar 15, 2026


Summary

Fixes #61586

/* node:coverage ignore next */ comments were not properly excluding branches from LCOV coverage output. When a branch was ignored, the LCOV reporter still emitted BRDA entries with count=0, causing BRF (branches
found) to exceed BRH (branches hit) even though the uncovered code was supposed to be ignored.

Root Cause

V8's block coverage ranges for branches can include structural elements like closing braces (}) that belong to the parent scope. The existing check range.ignoredLines === range.lines.length fails in this case
because the closing brace line isn't marked as ignored — it's covered by the parent function range.

For example:

  function getValue(condition) {
    if (condition) {
      return 'truthy';
    }                                    // ← line covered by parent range, not ignored
    /* node:coverage ignore next */
    return 'falsy';                      // ← ignored
  }

V8's false branch range spans from return 'falsy' to }. Since } isn't ignored, ignoredLines !== lines.length, so the branch wasn't excluded.

Fix

  1. Mark the ignore comment line itself as ignored (coverageLine.ignore = true), so it no longer counts as an uncovered line.
  2. Add a secondary check in summary(): for uncovered branches (count === 0) that contain ignored lines (ignoredLines > 0), skip the branch if all non-ignored lines are already covered (count > 0) by other ranges.
    This handles the case where V8's branch range includes structural lines covered by the parent range, but the actual uncovered code is all on ignored lines.

Test Coverage

  • getValue() — branch where false path is fully ignored → branch excluded from LCOV
  • getMixed() — branch with both ignored and non-ignored uncovered code → branch correctly kept in LCOV

@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 Mar 15, 2026
@avivkeller
Copy link
Member

@PradSharma554 can you please give your PR a proper description explaining what/why/how?

@codecov
Copy link

codecov bot commented Mar 15, 2026

Codecov Report

❌ Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (ae228c1) to head (1c9bd19).
⚠️ Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/coverage.js 86.36% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62264      +/-   ##
==========================================
+ Coverage   89.65%   89.68%   +0.02%     
==========================================
  Files         676      676              
  Lines      206546   206575      +29     
  Branches    39558    39560       +2     
==========================================
+ Hits       185179   185260      +81     
+ Misses      13485    13451      -34     
+ Partials     7882     7864      -18     
Files with missing lines Coverage Δ
lib/internal/test_runner/coverage.js 64.76% <86.36%> (+0.57%) ⬆️

... and 49 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.

@PradSharma554
Copy link
Author

@PradSharma554 can you please give your PR a proper description explaining what/why/how?

Okay
Lemme make some minor changes related to the PR first

@PradSharma554 PradSharma554 reopened this Mar 16, 2026
@PradSharma554
Copy link
Author

@avivkeller Can you review it now?

@JakobJingleheimer JakobJingleheimer added the duplicate Issues and PRs that are duplicates of other issues or PRs. label Mar 16, 2026
@JakobJingleheimer
Copy link
Member

Duplicate of #61598

@JakobJingleheimer JakobJingleheimer marked this as a duplicate of #61598 Mar 16, 2026
@PradSharma554
Copy link
Author

Duplicate of #61598

then shall i not work on this?

@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Mar 16, 2026

The cited PR was the first one and the furthest along. So, I would guess probably not?

Also, there are like 5 other PRs to fix this issue—probably best to check that before starting 🙂 (they're all linked to/from the hug report)

@PradSharma554
Copy link
Author

The cited PR was the first one and the furthest along. So, I would guess probably not?

Also, there are like 5 other PRs to fix this issue—probably best to check that before starting 🙂 (they're all linked to/from the hug report)

Okay! I'll pick up something else

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

Labels

duplicate Issues and PRs that are duplicates of other issues or PRs. 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.

test_runner: node:coverage ignore comments exclude DA but leave BRDA in lcov output

4 participants