-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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 throws "cannot read properties of undefined (reading: line)" #52775
Comments
Thanks for the bug report! Can you possibly provide a minimal reproducible example? One with only built-in modules, preferably. Thank you! |
This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open. |
@cjihrig, doesn't stalled mean that the PR/issue is waiting for information? |
I wouldn't categorize this as stalled, and I certainly don't want the bot to close the issue in 30 days. An exact reproduction would be nice, but we can make progress on this without it. There are only ~6 places in a single file where this could be happening, so we need some defensive coding. Also, if #51751 moves forward, I expect the error would have the missing information. |
Ahh, my bad. Sorry! I'll try and see if I can make a smaller example. |
@khaosdoctor this issue cannot be reproduced without the contents of |
Hey guys, I'm super sorry about the delay in responding. I'll try to make a minimal reproduction here to check whether it happens on a specific case. As far as I could test for now, simpler tests tend to work, while on longer tests (with more complex structures) this error appears, I should be back shortly with some tests I made. Thanks for waiting up! |
So I made a few tests here, I used this file for reference: import { describe, it } from 'node:test'
describe('ClassRepository', () => {
it('should create a new json file under .data', () => {
new ClassRepository()
})
}) Apparently the issue is the transpilation, the original file is being run by tsx via I tested both with Weirdly enough, if I run a simple test like: import { describe, it } from 'node:test'
function foo () { return 1+1 }
describe('ClassRepository', () => {
it('should create a new json file under .data', () => {
assert.strictEqual(foo(), 2)
})
}) The test coverage works with both commands. Here's the contents of the // ClassRepository
import { Class } from '../domain/Class.js'
import { Database } from './Db.js'
export class ClassRepository extends Database {
constructor() {
super(Class)
}
} // Db.ts
import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'fs'
import { dirname, resolve } from 'path'
import type { Serializable, SerializableStatic } from '../domain/types.js'
import { fileURLToPath } from 'url'
export abstract class Database {
protected readonly dbPath: string
protected dbData: Map<string, Serializable> = new Map()
readonly dbEntity: SerializableStatic
constructor(entity: SerializableStatic) {
const prefix = process.env.NODE_ENV === 'test' ? '-test' : ''
this.dbPath = resolve(dirname(fileURLToPath(import.meta.url)), `.data/${entity.name.toLowerCase()}${prefix}.json`)
this.dbEntity = entity
this.#initialize()
}
#initialize() {
if (!existsSync(dirname(this.dbPath))) {
mkdirSync(dirname(this.dbPath), { recursive: true })
}
if (existsSync(this.dbPath)) {
const data: [string, Record<string, unknown>][] = JSON.parse(readFileSync(this.dbPath, 'utf-8'))
for (const [key, value] of data) {
this.dbData.set(key, this.dbEntity.fromObject(value))
}
return
}
this.#updateFile()
}
#updateFile() {
const data = [...this.dbData.entries()].map(([key, value]) => [key, value.toObject()])
writeFileSync(this.dbPath, JSON.stringify(data))
return this
}
findById(id: string) {
return this.dbData.get(id)
}
listBy(property: string, value: any) {
const allData = this.list()
return allData.filter((data) => {
let comparable = (data as any)[property] as unknown // FIXME: Como melhorar?
let comparison = value as unknown
if (typeof comparable === 'object')
[comparable, comparison] = [JSON.stringify(comparable), JSON.stringify(comparison)]
// Ai podemos comparar os dois dados
return comparable === comparison
})
}
list(): Serializable[] {
return [...this.dbData.values()]
}
remove(id: string) {
this.dbData.delete(id)
return this.#updateFile()
}
save(entity: Serializable) {
this.dbData.set(entity.id, entity)
return this.#updateFile()
}
} Could this be due to ESM? |
Thanks for more information! If you transpile with |
Nope, if I transpile the file it goes away, but in some other cases, the coverage also works with the bare TS files being imported with |
It might be an issue with @nodejs/typescript (IDK if this is the right team to ping) |
I'm not sure, because it works in other cases, even running with tsx. I'm not sure it could be something related to ESM since I've seen some problems with it in the past |
@khaosdoctor your repro still does not include all files. for example |
I'm sorry! I have it here actually. This is the repo: https://github.com/Formacao-Typescript/projeto-3 If you go to the node-test-runner branch the code will be all there |
@khaosdoctor Hi! I have opened a PR to avoid throwing in this case, but the coverage will still be useless unless tsx will fix this by supporting source maps. for example, the original source for import { Teacher } from '../domain/Teacher.js'
import { Database } from './Db.js'
export class TeacherRepository extends Database {
constructor() {
super(Teacher)
}
} but the actual code emitted by tsx is this: var __defProp=Object.defineProperty;var __name=(target,value)=>__defProp(target,"name",{value,configurable:true});import{Teacher}from"../domain/Teacher.js";import{Database}from"./Db.js";class TeacherRepository extends Database{static{__name(this,"TeacherRepository")}constructor(){super(Teacher)}}export{TeacherRepository}; so the reported coverage will have absolutely nothing to do with the original source. |
PR-URL: #53000 Fixes: #52775 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: #53000 Fixes: #52775 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
I see... Thanks a lot for the validation! I'll try to find some documentation about it in the TSX page, if not, I'll open an issue there and take it to them! Thanks for the PR! |
tsx has had source map support since the beginning. There are no docs on it because it's not configurable. Source map support in Node is enabled via The tests verify it's working in Node v18~22, and also verify V8 coverage. |
@khaosdoctor @privatenumber I'v hade antoher try with the provided example: https://github.com/Formacao-Typescript/projeto-3 it seems that upgrading tsx from 3->4 does emit source maps and it works. |
Here are tests demonstrating that v3 does emit source maps (you can verify this for every file format and every version): There was one point when tsx didnt emit sourcemaps with the latest versions of Node, and it was due to this change in Node: But you can just upgrade tsx to fix this. So given the latest tsx emits source maps & coverage support, does this indicate there's an issue in Node's handling of experimental test coverage? |
I just did one more test, the simplest one of them, created a export function sum (...n) {
if (!n.every((num) => typeof num === 'number')) throw new Error('Not a number')
return n.reduce((acc, cur) => acc + cur)
} Then in the import { describe, it } from 'node:test'
import assert from 'node:assert'
import { sum } from '../sum.mjs'
describe('sum', () => {
it('should sum two numbers', () => {
assert.deepStrictEqual(sum(1,2), 3)
})
it('should error out if one is not a number', () => {
assert.throws(() => sum(1, 'b'), Error)
})
}) And replaced the command to be |
@khaosdoctor thanks for this reproduction. I'v opened a PR to fix this: #53315 |
PR-URL: nodejs#53000 Fixes: nodejs#52775 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#53000 Fixes: nodejs#52775 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: #53000 Fixes: #52775 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: #53000 Fixes: #52775 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
Version
22.0.0
Platform
Darwin Turing-2.local 23.4.0 Darwin Kernel Version 23.4.0: Fri Mar 15 00:10:42 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T6000 arm64
Subsystem
Test Runner
What steps will reproduce the bug?
While running this command:
NODE_ENV='test' tsx --test --experimental-test-coverage './src/**/*.test.ts' (same thing happens with
NODE_ENV='test' node --import='tsx/esm' --test --experimental-test-coverage './src/**/*.test.ts'`) in this file:Shows me the result of the test runner, but the coverage is not shown:
How often does it reproduce? Is there a required condition?
Seems to be intermittent, I have ran files without this issue in the past but for some file it does seem to happen, however I do not know which conditions are required for the bug to be reproduced as it seemed random.
What is the expected behavior? Why is that the expected behavior?
The expected behavior is that the code coverage is shown.
What do you see instead?
Warning: Could not report code coverage. TypeError: Cannot read properties of undefined (reading 'line')
Additional information
I had this issue before in a few different repositories, but they didn't have any configurations in common, as of now the reproduction of this error eludes me.
I've checked #51552 and #51392 but it doesn't seem to be related to source maps in my case.
The text was updated successfully, but these errors were encountered: