test_runner: ignore unmapped lines for coverage

[geeksilva97]

Refs: #54753

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

Codecov Report

Attention: Patch coverage is 7.69231% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (e225f00) to head (c02ad91).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/coverage.js	7.69%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55162      +/-   ##
==========================================
+ Coverage   88.24%   88.40%   +0.16%     
==========================================
  Files         651      652       +1     
  Lines      183836   186578    +2742     
  Branches    35827    36043     +216     
==========================================
+ Hits       162218   164941    +2723     
- Misses      14916    14919       +3     
- Partials     6702     6718      +16     

Files with missing lines	Coverage Δ
lib/internal/test_runner/coverage.js	63.67% <7.69%> (-1.08%)	⬇️

... and 101 files with indirect coverage changes

[RedYetiDev]

Can you add a test?

Without a test, it's unclear what this change accomplished / fixed

[geeksilva97]

Can you add a test?

Without a test, it's unclear what this change accomplished / fixed

I'm not sure which tests to add. Do you have any suggestions? For now, I updated the existing one adjusting the coverage report to meet the expectation.

[RedYetiDev]

IMHO I'm not sure how I feel about this change. It breaks the existing tests (as they required modification), and IIUC those tests are valid before this change.

Can you provide more information about this change? Such as the motivation, and how the changed tests are more accurate?

Just looking at the files, they contain uncovered lines, which this PR does not account for.

---

So, -1 from me.

[geeksilva97]

IMHO I'm not sure how I feel about this change. It breaks the existing tests (as they required modification), and IIUC those tests are valid before this change.

Can you provide more information about this change? Such as the motivation, and how the changed tests are more accurate?

If we take the repro steps provided in the issue we can see the coverage report considering, to calculate the line coverage, lines that are present in TS files but not in the transpiled JS.

Evidence

import type {} from "node:assert";

console.log("Hi");

This TS will generate the following JS

console.log("Hi");
export {};
//# sourceMappingURL=a.mjs.map

There's no mapping pointing to the import type {} from "node:assert";. In current behavior, the coverage reports this line as uncovered.

If I understood correctly, when collecting coverage from source maps it should rely on source maps entirely. So, if there's no mapped line we should ignore it - as the suggestion left on the issue.

That's what this change does. It walks through mappings and ignores those lines.

Coverage report before this change

Hi
✔ test.mjs (49.142ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 53.724042
ℹ start of coverage report
ℹ ----------------------------------------------------------
ℹ file      | line % | branch % | funcs % | uncovered lines
ℹ ----------------------------------------------------------
ℹ src/a.mts |  66.67 |   100.00 |  100.00 | 1
ℹ test.mjs  | 100.00 |   100.00 |  100.00 |
ℹ ----------------------------------------------------------
ℹ all files |  75.00 |   100.00 |  100.00 |
ℹ ----------------------------------------------------------
ℹ end of coverage report

Coverage report after this change

✔ test.mjs (314.762709ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 319.633917
ℹ start of coverage report
ℹ -----------------------------------------------------------
ℹ file       | line % | branch % | funcs % | uncovered lines
ℹ -----------------------------------------------------------
ℹ a.mjs      | 100.00 |   100.00 |  100.00 |
ℹ test.mjs   | 100.00 |   100.00 |  100.00 |
ℹ -----------------------------------------------------------
ℹ all files  | 100.00 |   100.00 |  100.00 |
ℹ -----------------------------------------------------------
ℹ end of coverage report

[RedYetiDev]

This may fix that issue, however I'm concerned that it breaks existing tests.

[geeksilva97]

This may fix that issue, however I'm concerned that it breaks existing tests.

I see

If this PR reaches the expected behavior, should we update those tests then? If that makes sense, and you can point me to those tests, I can work on them.