-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
test runner: Could not report code coverage (regression) #54240
Comments
This is an issue that is caused by the tsc compilation. If I recall correctly, a fix is not planned, but I might be wrong. CC @nodejs/test_runner to be sure |
This doesn't seem like a good error for users to encounter. We should improve it. |
Thanks for taking a look! Sorry for my misjudgment. |
FWIW I couldn't reproduce using the following code: interface Person {
firstName: string;
lastName: string;
age: number;
greet(): string;
}
class Student implements Person {
firstName: string;
lastName: string;
age: number;
grade: string;
constructor(firstName: string, lastName: string, age: number, grade: string) {
this.firstName = firstName;
this.lastName = lastName;
this.age = age;
this.grade = grade;
}
greet(): string {
return `Hello, my name is ${this.firstName} ${this.lastName} and I am ${this.age} years old.`;
}
getGrade(): string {
return `I am in grade ${this.grade}.`;
}
}
let student1 = new Student('John', 'Doe', 20, 'Junior');
console.log(student1.greet());
console.log(student1.getGrade());
But I'll keep trying to find a minimal reproduction |
@yume-chan could you provide the compiled file + sourcemap? |
I found a minimal repro. It just needs enough lines: index.ts1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
function a() {
console.log(1);
}
a(); index.js1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
1;
function a() {
console.log(1);
}
a();
//# sourceMappingURL=index.js.map index.js.map {"version":3,"file":"index.js","sourceRoot":"","sources":["index.ts"],"names":[],"mappings":"AAAA,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,CAAC,CAAC;AACF,SAAS,CAAC;IACR,OAAO,CAAC,GAAG,CAAC,CAAC,CAAC,CAAC;AACjB,CAAC;AACD,CAAC,EAAE,CAAC"} Any source map visualizer will tell you the generated source map is definitely correct: link Output:
Node.js source map module calculates line lengths excluding the line break character: node/lib/internal/source_map/source_map_cache.js Lines 228 to 249 in 90dea9e
The line lengths are then converted to line offsets, but it's done incorrectly: node/lib/internal/test_runner/coverage.js Lines 342 to 346 in 90dea9e
It didn't add the line break characters back, so after each line the It then got matched with correct offsets: node/lib/internal/test_runner/coverage.js Line 363 in 90dea9e
So after enough lines, node/lib/internal/test_runner/coverage.js Line 366 in 90dea9e
Fix: --- a/lib/internal/test_runner/coverage.js
+++ b/lib/internal/test_runner/coverage.js
@@ -340,8 +340,8 @@ class TestCoverage {
const { data, lineLengths } = sourceMapCache[url];
let offset = 0;
const executedLines = ArrayPrototypeMap(lineLengths, (length, i) => {
- const coverageLine = new CoverageLine(i + 1, offset, null, length);
- offset += length;
+ const coverageLine = new CoverageLine(i + 1, offset, null, length + 1);
+ offset += length + 1;
return coverageLine;
});
if (data.sourcesContent != null) { |
Thanks for investigating this issue. I also experience the same problem. |
@yume-chan thanks for looking into this. Would you like to open a PR? |
This commit updates the source mapping logic in the test runner to account for newline characters that are not included in line length calculations. Co-authored-by: yume-chan Fixes: nodejs#54240
This commit updates the source mapping logic in the test runner to account for newline characters that are not included in line length calculations. Co-authored-by: Simon Chan <1330321+yume-chan@users.noreply.github.com> Fixes: nodejs#54240
This commit updates the source mapping logic in the test runner to account for newline characters that are not included in line length calculations. Co-authored-by: Simon Chan <1330321+yume-chan@users.noreply.github.com> Fixes: #54240 PR-URL: #54444 Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Version
v22.6.0
Platform
Subsystem
No response
What steps will reproduce the bug?
Because the cryptic error message, I can't narrow the repro down further.
How often does it reproduce? Is there a required condition?
100%
What is the expected behavior? Why is that the expected behavior?
Reports code coverage. It worked in v20.15.0
What do you see instead?
Begin with v20.16.0, it outputs:
Additional information
No response
The text was updated successfully, but these errors were encountered: