microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.92k stars 595 forks source link

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

Open nathanberry97 opened 10 months ago

nathanberry97 commented 10 months ago

Summary

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 ```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

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

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
octogonz commented 10 months ago

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:

Where we left off

How I remember things, that discussion left off with:

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

Possible action plan

  1. 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.)

  2. 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

octogonz commented 10 months ago

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

nathanberry97 commented 10 months ago

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

iclanton commented 10 months ago

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.

nathanberry97 commented 10 months ago

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.

octogonz commented 10 months ago

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