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

test runner: Could not report code coverage (regression) #54240

Closed
yume-chan opened this issue Aug 7, 2024 · 10 comments · Fixed by #54444
Closed

test runner: Could not report code coverage (regression) #54240

yume-chan opened this issue Aug 7, 2024 · 10 comments · Fixed by #54444
Labels
coverage Issues and PRs related to native coverage support. test_runner Issues and PRs related to the test runner subsystem.

Comments

@yume-chan
Copy link
Contributor

Version

v22.6.0

Platform

Linux f5d6c39aef47 5.15.153.1-microsoft-standard-WSL2 #1 SMP Fri Mar 29 23:14:13 UTC 2024 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Because the cryptic error message, I can't narrow the repro down further.

npm i -g pnpm
git clone --depth 1 https://github.com/yume-chan/ya-webadb.git
cd ya-webadb/
pnpm i
pnpm build
cd libraries/struct
npx tsc -b tsconfig.test.json
node --enable-source-maps --experimental-test-coverage --test esm/struct.spec.js

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:

ℹ Warning: Could not report code coverage. TypeError: Cannot read properties of undefined (reading 'line')

Additional information

No response

@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Aug 7, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Aug 7, 2024

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

@RedYetiDev RedYetiDev added the wontfix Issues that will not be fixed. label Aug 7, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Aug 7, 2024

This doesn't seem like a good error for users to encounter. We should improve it.

@cjihrig cjihrig reopened this Aug 7, 2024
@RedYetiDev RedYetiDev removed the wontfix Issues that will not be fixed. label Aug 7, 2024
@RedYetiDev
Copy link
Member

Thanks for taking a look! Sorry for my misjudgment.

@RedYetiDev RedYetiDev added coverage Issues and PRs related to native coverage support. test_runner Issues and PRs related to the test runner subsystem. labels Aug 7, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Aug 7, 2024

@RedYetiDev
Copy link
Member

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());
npx tsc source.ts
node --enable-source-maps --experimental-test-coverage --test source.js

But I'll keep trying to find a minimal reproduction

@RedYetiDev
Copy link
Member

@yume-chan could you provide the compiled file + sourcemap?

@yume-chan
Copy link
Contributor Author

yume-chan commented Aug 8, 2024

I found a minimal repro. It just needs enough lines:

index.ts
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;
1;
function a() {
  console.log(1);
}
a();
index.js
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;
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 --enable-source-maps --experimental-test-coverage --test "index.js"                              
1
✔ D:\dev\sandbox\node-source-map\index.js (90.0243ms)
ℹ Warning: Could not report code coverage. TypeError: Cannot read properties of undefined (reading 'line')
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 95.1846

Node.js source map module calculates line lengths excluding the line break character:

// Cache the length of each line in the file that a source map was extracted
// from. This allows translation from byte offset V8 coverage reports,
// to line/column offset Source Map V3.
function lineLengths(content) {
const contentLength = content.length;
const output = [];
let lineLength = 0;
for (let i = 0; i < contentLength; i++, lineLength++) {
const codePoint = StringPrototypeCodePointAt(content, i);
// We purposefully keep \r as part of the line-length calculation, in
// cases where there is a \r\n separator, so that this can be taken into
// account in coverage calculations.
// codepoints for \n (new line), \u2028 (line separator) and \u2029 (paragraph separator)
if (codePoint === 10 || codePoint === 0x2028 || codePoint === 0x2029) {
ArrayPrototypePush(output, lineLength);
lineLength = -1; // To not count the matched codePoint such as \n character
}
}
ArrayPrototypePush(output, lineLength);
return output;
}

The line lengths are then converted to line offsets, but it's done incorrectly:

const executedLines = ArrayPrototypeMap(lineLengths, (length, i) => {
const coverageLine = new CoverageLine(i + 1, offset, null, length);
offset += length;
return coverageLine;
});

It didn't add the line break characters back, so after each line the offset is short by 1.

It then got matched with correct offsets:

const { lines } = mapRangeToLines(ranges[k], executedLines);

So after enough lines, mapRangeToLines returns an empty lines array, causing this line to throw error:

.findEntry(lines[0].line - 1, MathMax(0, startOffset - lines[0].startOffset));


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) {

@martinssonj
Copy link

Thanks for investigating this issue. I also experience the same problem.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 13, 2024

@yume-chan thanks for looking into this. Would you like to open a PR?

@yume-chan
Copy link
Contributor Author

@cjihrig I'm really busy recently. A Node.js contributor may fix this quicker than me. maybe @MoLow can you take a look as you authored this part?

ref #53315

cjihrig added a commit to cjihrig/node that referenced this issue Aug 19, 2024
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
cjihrig added a commit to cjihrig/node that referenced this issue Aug 19, 2024
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
RafaelGSS pushed a commit that referenced this issue Aug 25, 2024
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>
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. 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