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

Unable to ignore lines for coverage in projen project #3550

Closed
michanto opened this issue May 1, 2024 · 1 comment · Fixed by #3601 or #3611
Closed

Unable to ignore lines for coverage in projen project #3550

michanto opened this issue May 1, 2024 · 1 comment · Fixed by #3601 or #3611
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@michanto
Copy link

michanto commented May 1, 2024

I have an AwsCdkConstructLibrary with some code I would like ignored:

    /* istanbul ignore next */
    if (TreeInspectable.TREE_INSPECTABLE_SERVICE.has(scope)) {
      throw new Error(`Should not be able to get here.  ${scope.node.path} is already inspectable.`);
    }

If I run npm run test the line counts against me in the coverage report.

If I run npx jest the same line is ignored. I can see (by running npm run test --verbose) that the test command is:

jest --passWithNoTests --coverageProvider=v8 --updateSnapshot

I have tried:

  • /* istanbul ignore next */
  • /* istanbul ignore if */
  • /* c8 ignore next */
  • /* v8 ignore next */

In all cases, npm run test counts the line against me.

I tried replacing the test command, but I cannot because it already exists. I tried removing it first, but I cannot because it is referenced elsewhere. I don't know how to change the test command to allow me to ignore lines.

@mrgrain mrgrain added bug Something isn't working documentation Improvements or additions to documentation labels May 10, 2024
@mrgrain mrgrain changed the title Unable to ignore lines in projen project. Unable to ignore lines for coverage in projen project May 25, 2024
@mrgrain
Copy link
Contributor

mrgrain commented May 25, 2024

Fixing this so you can change the coverage provider.

For v8 I recommend using /* c8 ignore start */ + /*c8 ignore end */. I have more success with that.
The bug aside, it is obviously not projen's job to make sure the ignore instructions work.

mrgrain added a commit that referenced this issue May 25, 2024
We previously forced the coverage provider as a CLI option, which prevented the value from being changed.

This PR replaces the forced CLI option with a changed default config value.

Considering this a fix since the default functionality is not changed and the option was effectively unusable before.

Also removes the check for Node 14, since this is old version is not supported by projen anymore and presumably has no projen usage.

Fixes: #3550
@mergify mergify bot closed this as completed in #3601 May 26, 2024
mergify bot pushed a commit that referenced this issue May 26, 2024
We previously forced the coverage provider as a CLI option, which prevented the value from being changed.
This PR replaces the forced CLI option with a changed default config value.
Considering this a fix since the default functionality is not changed and the option was effectively unusable before.

Also removes the check for Node 14, since this is old version is not supported by projen anymore and presumably has no projen usage.

Fixes: #3550

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
mrgrain added a commit that referenced this issue May 28, 2024
We previously forced the coverage provider as a CLI option, which prevented the value from being changed.
This PR replaces the forced CLI option with a changed default config value.
Considering this a fix since the default functionality is not changed and the option was effectively unusable before.

Also removes the check for Node 14, since this is old version is not supported by projen anymore and presumably has no projen usage.

Fixes: #3550

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
mergify bot pushed a commit that referenced this issue May 28, 2024
The `Jest` component previously defaulted to the `v8` coverage provider via a CLI argument. This approach prevented the value from being changed. The conditions for this default were scoped to `NodeProject`s. Anyone using the component with a project type _not_ based on `NodeProject` or manually resetting the CLI options, would not have been effected by this. 

Coverage provide has now been updated to _always_ default `v8` via a config option. With this it is now possible again to change the coverage provider. This change can effect projects if they were previously relying on using `babel` as coverage provider.

BREAKING CHANGE: The jest coverage provider now always to `v8` via config. Most projects won't be effected by this change since this default was already set in most circumstances. To use `babel` instead, set the [coverageProvider](https://projen.io/docs/api/javascript?_highlight=coveragepro#projen.javascript.JestConfigOptions.property.coverageProvider) option on `jestConfig`.

We are making this change to fix coverage provider not being configurable.
We are changing the default to be more consistent across all projects and remove the conditionality of it.

Fixes: #3550
Re-rolls: #3601

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Fixes #

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
2 participants