Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--experimental-test-coverage falsely reports missing coverage where TS source is import type #54753

Open
jaydenseric opened this issue Sep 4, 2024 · 6 comments · Fixed by #55339
Labels
coverage Issues and PRs related to native coverage support. source maps Issues and PRs related to source map support. test_runner Issues and PRs related to the test runner subsystem.

Comments

@jaydenseric
Copy link
Contributor

jaydenseric commented Sep 4, 2024

Version

v22.8.0

Platform

Darwin TRI-N93DLJDY6Y 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:30 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

With this TypeScript source code in src/a.mts (where the import is from doesn't matter):

import type {} from "node:assert";

console.log("Hi");

Compiling that to dist/a.mjs:

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

With source map dist/a.mjs.map:

{"version":3,"file":"a.mjs","sourceRoot":"","sources":["../src/a.mts"],"names":[],"mappings":"AAEA,OAAO,CAAC,GAAG,CAAC,IAAI,CAAC,CAAC"}

Then in test.mjs:

import "./dist/a.mjs";

Run:

node --experimental-test-coverage --test test.mjs

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior? Why is that the expected behavior?

100% code coverage reported for the module src/a.mts.

What do you see instead?

Notice the false missing line coverage reported in the terminal output:

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

Additional information

If you comment out the import type {} from "node:assert"; and rebuild, a second run for functionally the same runtime code now correctly reports no missing coverage:

Hi
✔ test.mjs (47.03ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 51.619166
ℹ start of coverage report
ℹ ----------------------------------------------------------
ℹ file      | line % | branch % | funcs % | uncovered lines
ℹ ----------------------------------------------------------
ℹ src/a.mts | 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

If you don't create source maps when compiling TypeScript modules containing import type, the Node.js test runner correctly reports no missing coverage. So the problem is around how Node.js is interpreting the source maps. Only runtime code should determine code coverage; not source code like TypeScript import type that is eliminated in the build.

Something else that is strange, is that the Node.js CLI flag --enable-source-maps doesn't seem to have an effect on how the Node.js test runner reports coverage; even without the flag it will always take into account the source maps information. Why is coverage exempt from respecting how --enable-source-maps works for other Node.js features?

@RedYetiDev RedYetiDev added coverage Issues and PRs related to native coverage support. source maps Issues and PRs related to source map support. test_runner Issues and PRs related to the test runner subsystem. strip-types Issues or PRs related to strip-types support labels Sep 4, 2024
@jaydenseric
Copy link
Contributor Author

I'm not sure, but maybe inspiration can be taken from istanbuljs/v8-to-istanbul#244 .

@RedYetiDev RedYetiDev removed the strip-types Issues or PRs related to strip-types support label Sep 5, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Sep 14, 2024

Why is coverage exempt from respecting how --enable-source-maps works for other Node.js features?

That seems like a bug to me. @MoLow do we need to check the flag here?

@MoLow
Copy link
Member

MoLow commented Sep 15, 2024

Seems like a bug to me as well. not sure where is the correct place to fix

@RedYetiDev RedYetiDev added the confirmed-bug Issues with confirmed bugs. label Sep 20, 2024
RedYetiDev added a commit to RedYetiDev/node that referenced this issue Sep 21, 2024
Fixes nodejs#54753

Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
Co-Authored-By: Jayden Seric <me@jaydenseric.com>
@RedYetiDev
Copy link
Member

(This issue isn't fixed by the linked PR #55037, I've fixed it~sorry for the confusion)

@RedYetiDev
Copy link
Member

Hey, I'm thinking this over, and technically the import type ... is uncovered, as it's not used anywhere at Runtime. If you look at:

{
  "version": 3,
  "sources": ["index.mts"],
  "sourcesContent": ["invalidJSSyntax;\n\nconsole.log(\"Hi\");"],
  "mappings": "AAEA,QAAQ,IAAI,IAAI",
  "names": []
}

invalidJSSyntax should return that it is uncovered, right? Well... import type is similar, no?

@RedYetiDev
Copy link
Member

RedYetiDev commented Oct 9, 2024

Reopening due to #55339. I've removed confirmed-bug Issues with confirmed bugs. as the "fix" for this issue, that is, not checking coverage on un-transpiled lines, doesn't seem to be viable.

IMHO this is expected behavior, as, while annoying, this line is technically not covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage Issues and PRs related to native coverage support. source maps Issues and PRs related to source map support. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants