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 throws "cannot read properties of undefined (reading: line)" #52775

Closed
khaosdoctor opened this issue May 1, 2024 · 22 comments · Fixed by #53000 or #53315
Closed
Labels
test_runner Issues and PRs related to the test runner subsystem.

Comments

@khaosdoctor
Copy link
Member

khaosdoctor commented May 1, 2024

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:

import assert from 'node:assert'
import { readFileSync, readdirSync, unlinkSync } from 'node:fs'
import { dirname, resolve } from 'node:path'
import { before, describe, it } from 'node:test'
import { fileURLToPath } from 'node:url'
import { Teacher } from '../domain/Teacher.js'
import { TeacherRepository } from './TeacherRepository.js'

describe('TeacherRepository', () => {
  const dbPath = resolve(dirname(fileURLToPath(import.meta.url)), '.data')
  const teacher = new Teacher({
    firstName: 'Lucas',
    surname: 'Santos',
    phone: '123456789',
    email: 'foo@gmail.com',
    document: '123456789',
    hiringDate: new Date('2020-10-20').toISOString(),
    major: 'Computer Science',
    salary: 5000
  })

  before(() => {
    // limpa o arquivo de teste antes de começar
    // isso garante que sempre vamos ter um único registro
    unlinkSync(resolve(dbPath, 'teacher-test.json'))
  })

  it('should create a new teachers.json file under .data', () => {
    void new TeacherRepository()
    const dirs = readdirSync(dbPath)
    assert.ok(dirs.includes('teacher-test.json'))
  })

  it('should save a new Teacher in the database', () => {
    const db = new TeacherRepository()

    const instance = db.save(teacher)
    assert.ok(instance instanceof TeacherRepository)
    const teachersFile = JSON.parse(readFileSync(`${dbPath}/teacher-test.json`, 'utf-8'))
    assert.deepStrictEqual(Teacher.fromObject(teachersFile[0][1]), teacher)
  })

  it('should list all teachers in the database', () => {
    const db = new TeacherRepository()
    const teachers = db.list() as Teacher[]
    assert.ok(teachers.length === 1)
    assert.ok(teachers[0] instanceof Teacher)
  })

  it('should find a teacher by id', () => {
    const db = new TeacherRepository()
    const found = db.findById(teacher.id)
    assert.ok(found instanceof Teacher)
    assert.deepStrictEqual(found, teacher)
  })

  it('should update a teacher', () => {
    const db = new TeacherRepository()
    teacher.firstName = 'Not Lucas'
    const updated = db.save(teacher).findById(teacher.id)
    assert.ok(updated instanceof Teacher)
    assert.deepStrictEqual(updated, teacher)
  })

  it('should list teachers by a specific property', () => {
    const db = new TeacherRepository()
    const teachers = db.listBy('firstName', 'Not Lucas') as Teacher[]
    assert.ok(teachers.length === 1)
    assert.ok(teachers[0] instanceof Teacher)
    assert.deepStrictEqual(teachers[0], teacher)
  })
})

Shows me the result of the test runner, but the coverage is not shown:

▶ TeacherRepository
  ✔ should create a new teachers.json file under .data (1.419416ms)
  ✔ should save a new Teacher in the database (4.545542ms)
  ✔ should list all teachers in the database (0.264125ms)
  ✔ should find a teacher by id (0.181667ms)
  ✔ should update a teacher (0.311292ms)
  ✔ should list teachers by a specific property (0.801084ms)
▶ TeacherRepository (8.870708ms)
ℹ Warning: Could not report code coverage. TypeError: Cannot read properties of undefined (reading 'line')
ℹ tests 6
ℹ suites 1
ℹ pass 6
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 221.09275

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.

@VoltrexKeyva VoltrexKeyva added the test_runner Issues and PRs related to the test runner subsystem. label May 1, 2024
@avivkeller
Copy link
Member

Thanks for the bug report! Can you possibly provide a minimal reproducible example? One with only built-in modules, preferably.

Thank you!

@avivkeller avivkeller added the stalled Issues and PRs that are stalled. label May 6, 2024
Copy link
Contributor

github-actions bot commented May 6, 2024

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 cjihrig removed the stalled Issues and PRs that are stalled. label May 6, 2024
@avivkeller
Copy link
Member

avivkeller commented May 6, 2024

@cjihrig, doesn't stalled mean that the PR/issue is waiting for information?

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2024

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.

@avivkeller
Copy link
Member

avivkeller commented May 6, 2024

Ahh, my bad. Sorry! I'll try and see if I can make a smaller example.

@MoLow
Copy link
Member

MoLow commented May 7, 2024

@khaosdoctor this issue cannot be reproduced without the contents of Teacher and TeacherRepository. Is there any way you can make a minimal reproduction within a gist or similar?

@khaosdoctor
Copy link
Member Author

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!

@khaosdoctor
Copy link
Member Author

khaosdoctor commented May 7, 2024

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 --import tsx flag, the command is node --import tsx --test --experimental-test-coverage './src/**/*.test.*', if it's run like this, it won't work and will output that error, if I precompile the test and run it again, but using the dist folder instead, it works.

I tested both with node and tsx commands, the results are the same, if I run tsx --test --experimental-test-coverage './src/**/*.test.*' the error persists, but if I change the path to dist it works.

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 file and its underlying class:

// 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?

@avivkeller
Copy link
Member

Thanks for more information! If you transpile with tsc, and then run the output file, will this issue also occur?

@khaosdoctor
Copy link
Member Author

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 --import as well, like the simple example I mentioned.

@avivkeller
Copy link
Member

It might be an issue with tsx, but I'm far from an expert.

@nodejs/typescript (IDK if this is the right team to ping)

@khaosdoctor
Copy link
Member Author

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

@MoLow
Copy link
Member

MoLow commented May 9, 2024

@khaosdoctor your repro still does not include all files. for example domain/Class and domain/types are missing. I suggest you create a fresh GitHub repo we can close and run to reproduce the issue.

@khaosdoctor
Copy link
Member Author

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

@MoLow
Copy link
Member

MoLow commented May 15, 2024

@khaosdoctor Hi!
the underlying issue here is that tsx transpiles your code and the test coverage is evaluated using the transpiled code instead of the source code.
the standard solution for this is using source maps - but tsx doesn't seem to emit any (I have tried some things such as adding --enable-source-maps but that didn't seem to change much - didn't find any tsx documentation about source maps)

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 TeacherRepository.ts is

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.

nodejs-github-bot pushed a commit that referenced this issue May 17, 2024
PR-URL: #53000
Fixes: #52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
targos pushed a commit that referenced this issue May 21, 2024
PR-URL: #53000
Fixes: #52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@khaosdoctor
Copy link
Member Author

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!

@privatenumber
Copy link
Contributor

privatenumber commented May 22, 2024

@MoLow

the standard solution for this is using source maps - but tsx doesn't seem to emit any (I have tried some things such as adding --enable-source-maps but that didn't seem to change much - didn't find any tsx documentation about source maps)

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 process.setSourceMapsEnabled(true) and source maps are emitted inline.

The tests verify it's working in Node v18~22, and also verify V8 coverage.

@MoLow
Copy link
Member

MoLow commented May 24, 2024

@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.
I have no idea why tsx 3 doesn't emit source maps

@privatenumber
Copy link
Contributor

privatenumber commented May 24, 2024

Here are tests demonstrating that v3 does emit source maps (you can verify this for every file format and every version):
https://github.com/privatenumber/tsx/blob/v3.14.0/tests/specs/typescript/ts.ts#L21

There was one point when tsx didnt emit sourcemaps with the latest versions of Node, and it was due to this change in Node:
privatenumber/tsx@1a10da7

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?

@khaosdoctor
Copy link
Member Author

Tried with the new version of TSX and still having the same error here:
image

Using Node 22.2 and TSX 4.11, these are my devdeps:

{
    "@types/express": "^4.17.20",
    "@types/helmet": "^4.0.0",
    "@types/node": "^20.12.12",
    "tsx": "^4.11.0",
    "typescript": "^5.2.2"
}

Tried both with npm ci, removing node modules, removing everything and reinstalling it all again.

@khaosdoctor
Copy link
Member Author

khaosdoctor commented May 29, 2024

I just did one more test, the simplest one of them, created a sum.mts file with the contents:

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 sum.test.mts I did this:

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 node --test --import=tsx --experimental-test-coverage ./tests/**/*.test.*. Still had the same line issue.

@MoLow
Copy link
Member

MoLow commented Jun 3, 2024

@khaosdoctor thanks for this reproduction. I'v opened a PR to fix this: #53315

@MoLow MoLow reopened this Jun 3, 2024
sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53000
Fixes: nodejs#52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53000
Fixes: nodejs#52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53000
Fixes: #52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53000
Fixes: #52775
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
6 participants