[heft] Upgrading to latest version of Heft from v50.7 test coverage includes files with /* istanbul ignore file */

Question

[heft] Upgrading to latest version of Heft from v50.7 test coverage includes files with /* istanbul ignore file */

nathanberry97 opened this issue 5 months ago · 6 comments

Summary

We are trying to update our repo with the latest version of @rushstack/heft and @rushstack/heft-node-rig but are running into issues in our code coverage
When running code build it is including files which declare /* istanbul ignore file */ resulting in our code coverage dropping below our required coverage
But when using the following versions for Heft it excludes file which include /* istanbul ignore file */ :

Package	Version which works
`@rushstack/heft`	`v50.7`
`@rushstack/heft-node-rig`	`v1.13.1`

The above table is the latest version of Heft we can currently use which works with /* istanbul ignore file */

Repro steps

Note If you want to use the latest packages for example repo please update the libraries/something/package.json with the following:

latest packages for Example repos package.json

{
"name": "something",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
  "build": "heft test --clean"
},
"keywords": [],
"author": "",
"license": "ISC",
"devDependencies": {
  "@rushstack/eslint-config": "^3.5.1",
  "@rushstack/heft-jest-plugin": "^0.10.6",
  "@rushstack/heft-node-rig": "^2.3.14",
  "@rushstack/heft": "^0.63.4",
  "@types/heft-jest": "^1.0.6",
  "@types/node": "^18.18.0",
  "eslint": "^8.56.0",
  "typescript": "^5.3.3"
}
}

To reproduce the error please clone the example repo and run the following commands:

rush install
rush build

Now open the libraries/something/temp/coverage/index.html file in a browser
Note that the ignore.ts file is included in code coverage even though it has the /* istanbul ignore file */ code comment at the top of the file

If you then proceed to update the following packages to the versions shown below in libraries/something/package.json:

{
  "devDependencies": {
    "@rushstack/heft-node-rig": "^1.13.1",
    "@rushstack/heft": "^0.50.7"
  }
}

Once updated please run the following commands:

rush update
rush build

Now open the libraries/something/temp/coverage/index.html file in a browser
Note that the ignore.ts file is now not included in code coverage

Expected result:

We expect the latest versions of @rushstack/heft and @rushstack/heft-node-rig to exclude files with /* istanbul ignore file */ included in them.

Actual result:

The latest versions of @rushstack/heft and @rushstack/heft-node-rig are including any file with /* istanbul ignore file */ included in them in our code coverage.

Details

I'm not sure what is causing this error, so some suggestions would be great thank you :)

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question	Answer
`@rushstack/heft` version?	`v0.63.4`
Operating system?	Linux
Would you consider contributing a PR?	No
Node.js version (`node -v`)?	`18.18.0`

Answer 1 · 2023-12-19T22:03:47.000Z

Thanks for writing this up. This topic has come up a couple times before, but we don't seem to have a GitHub issue tracking it:

#heft chat: Coverage with heft-jest-plugin
#heft chat: jest is not ignoring files for code coverage
An upstream Jest issue: jestjs/jest#11188

Where we left off

How I remember things, that discussion left off with:

Traditionally Jest checked coverage by injecting instrumented code (coverageProvider: 'babel'), but now they also support Node.js native instrumentation (coverageProvider: 'v8') whose advantage is speed
But even in the latest LTS Node.js, the v8 provider seems to still have some limitations
V8 uses a different suppression syntax /* c8 ignore next */ instead of /* istanbul ignore next */ so you need to migrate your suppressions...
...however people from the community seem to be reporting that in some circumstances ignore next doesn't work at all
In some version, Heft switched its default from babel to v8 but it was not communicated very clearly. The issue reports are mainly coming from people upgrading to 0.51, but supposedly the change was much earlier than that (?)
We asked Microsoft why they didn't encounter this problem themselves; the answer was that their monorepo rarely uses /* istanbul ignore next */ and the V8 speedup was significant

We should double-check my facts above. But assuming all these things are true, what I had recommended was:

Possible action plan

Revert heft-jest-plugin's default configuration to the old value of coverageProvider: 'babel'.

(We certainly prioritize speed, but "everything just works" is an even higher priority.)
Update the Heft docs to explain the tradeoffs and show how to switch between the two modes.

If we can agree on this, then it should be easy to implement.

@D4N14L @iclanton @elliot-nelson

Answer 2 · 2023-12-19T22:04:13.000Z

@nathanberry97 could you check this against your own observations, and let us know if anything is incorrect?

Answer 3 · 2023-12-20T17:11:27.000Z

Hi @octogonz thanks for getting back to my issue so quick

I've been playing around with our rush repo at work today

To get the same effect as /* istanbul ignore file */ I needed to add /* c8 ignore start */ to the top of the file, but we have quite a lot of projects we would have to migrate over to this new syntax if we were to do this

Note when using /* c8 ignore next */ it wasn't ignoring the whole file

Then I thought I'll just change the provider back to babel in our jest.config.json:

"coverageProvider": "babel"

Which works but our code coverage then uses the js files rather than our ts files, not sure if there is something else I would need to add our jest.config.json file for it to point towards the src directory rather than the lib directory

Ideally I would like the option to continue to use babel if this is possible and if so would be useful to have this documented somewhere

Answer 4 · 2023-12-20T19:18:16.000Z

A possible explanation for why the coverage data is showing up for the js files instead of the ts files may be that Babel is not consuming the existing sourcemaps produced by TypeScript. Pointing at the src folder may work. Given that you'll already be paying the penalty for using Babel in the first place, the performance hit from also transforming TS to JS may not that much worse. You may also look into adding configuration options into a .babelrc to consume the TypeScript sourcemaps.

Another option would be to write a Heft plugin that postprocesses the js files to replace /* istanbul ignore file */ with /* c8 ignore start */, although that wouldn't solve the other issue with /* c8 ignore next */. This would be kinda similar to writing a TypeScript transformer, but Heft doesn't currently provide a way to inject one, although that would be an interesting feature to consider adding support for.

From the Heft/rig side - I'm not sure which approach we should take here other than documenting that you need to use V8-style coverage suppressions.

Answer 5 · 2023-12-21T16:32:52.000Z

Thanks @iclanton and @octogonz for your help.

Just to let you know we decided to migrate over to use v8 rather than continuing to use babel.

Note it wasn't as painful as I thought it would be migrating over thankfully

Basically any file we wanted to ignore we put /* c8 ignore start */ at the top of the file. It works a bit differently than /* istanbul ignore file */ i.e. the file is in the code coverage but shows that the file has 100% coverage.

Important if /* c8 ignore start */ is not at the top of the file it won't show as having 100% coverage

In terms of files we don't have control over i.e. generated types for example when using openapi-typescript-codegen these files include /* istanbul ignore file */ which doesn't work when using v8.
So I just updated the jest.config.json to include the follow:

{
  "coveragePathIgnorePatterns": [
    "/path/to/ignore/"
  ]
}

I'm not sure if you would like me to close this issue now? But in terms of documentation it would have been useful to have some kind of doc showing how to migrate from babel to v8.

Answer 6 · 2023-12-21T23:40:54.000Z

Let me add some docs to the website before we close this issue