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

Coverage thresholds can be set up for individual files #4185

Merged
merged 3 commits into from
Aug 24, 2017

Conversation

krishnagoth
Copy link
Contributor

@krishnagoth krishnagoth commented Aug 2, 2017

Currently only global coverage threshold is available which is convenient when a team can allow themselves 100 coverage all over the codebase right off the bat. Unfortunately it's not always possible, including times where significant design changes are required to get files and testing harnesses up to the standard. My goal is to provide teams with the ability to exclude certain files from 'global' threshold and allow their coverage to be managed separately.

When file paths or globs (which are then expanded to full file paths) are added to coverageThreshold config, reporter subtracts their coverage stats from global coverage and applies thresholds separately to global and to each file individually. If threshold file has no coverage data, error is returned as well, indicating misconfiguration. Until paths are added, coverage is calculated as usual.

The behaviour can be illustrated by the following sequence.

coverageThreshold: {
  'global': {
    statements: 59.28,
    branches: 49.33
  },
}

in this example this threshold is not met and returns an error:

Jest: Coverage for statements (58.99%) does not meet global threshold (59.28%)
Jest: Coverage for branches (48.22%) does not meet global threshold (49.33%)

Now let's add files to coverageThreshold

coverageThreshold: {
  'global': {
    statements: 59.28,
    branches: 49.33
  },
  './src/components/*-covered-file.ts': {
    statements: 100,
    branches: 100
  },
  './src/components/unknown-file.ts': {
    statements: 100,
    branches: 100
  },
}

which expands to

coverageThreshold: {
  'global': {
    statements: 59.28,
    branches: 49.33
  },
  './src/components/partially-covered-file.ts': {
    statements: 100,
    branches: 100
  },
  './src/components/under-covered-file.ts': {
    statements: 100,
    branches: 100
  },
  './src/components/unknown-file.ts': {
    statements: 100,
    branches: 100
  },
}

the errors are now

Jest: Coverage for statements (42.42%) does not meet ./src/components/partially-covered-file.ts threshold (100%)
Jest: Coverage for statements (92.75%) does not meet ./src/components/under-covered-file.ts threshold (100%)
Jest: Coverage for branches (61.29%) does not meet ./src/components/under-covered-file.ts threshold (100%)
Jest: Coverage data for ./src/components/unknown-file.ts was not found.

Since these files have been excluded from summary, they are no longer counted towards global coverage and global threshold is met.

@aaronabramov
Copy link
Contributor

we've had this idea for a while, but there's too many things that we need to design properly before we can do this (config format, what exactly happens when the threshold is not met, what happens if we run only a subset of tests, etc..)

@aaronabramov
Copy link
Contributor

that's a good start though!
i think we need to at least support globs

'my_directory/**/*.js': {statements: 90}

what do you think? @cpojer @thymikee

}, undefined);

const errors = [].concat(
...Object.keys(globalConfig.coverageThreshold)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixing array concat and spread. For now just use concat, because we're not transforming array spreading (not supported on Node 4, that's why Circle fails)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I use [].concat.apply([], arrays); then?

return testReporter
.onRunComplete(new Set(), {}, mockAggResults)
.then(() => {
expect(testReporter.getLastError()).toBeTruthy();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to at least once assert on what's the content of the error, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree, will do.

} else {
summary = fileCov.toSummary();
}
return summary;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return summary !== undefined && summary !== null 
  ? summary.merge(fileCov.toSummary()) 
  : fileCov.toSummary()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if I may use ternary in this code base. Now I am :)

} else {
return [
`Jest: Coverage data for ${thresholdKey} was not found.`,
];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we could push these logic branches into check function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like check function being unaware of such details and just sticking to comparing numbers. I can add move this to check as a two distinct conditions if actuals or thresholds do not contain given key respectively. What do you think?

@thymikee
Copy link
Collaborator

thymikee commented Aug 3, 2017

@aaronabramov LGTM! Also think it's a good start and we can follow this up with glob support.

@cpojer
Copy link
Member

cpojer commented Aug 3, 2017

Yep, agree that this should have glob support.

@codecov-io
Copy link

codecov-io commented Aug 7, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@84b2502). Click here to learn what that means.
The diff coverage is 95%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4185   +/-   ##
========================================
  Coverage          ?   56.6%           
========================================
  Files             ?     184           
  Lines             ?    6284           
  Branches          ?       6           
========================================
  Hits              ?    3557           
  Misses            ?    2724           
  Partials          ?       3
Impacted Files Coverage Δ
...ckages/jest-cli/src/reporters/coverage_reporter.js 54.9% <95%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84b2502...8037af9. Read the comment docs.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neeeds glob support if we are going to add this to 21.

@krishnagoth krishnagoth force-pushed the flexible-coverage-thresholds branch 2 times, most recently from c4e335b to b71ccdb Compare August 24, 2017 20:00
…re currently setup for 'global'. Supports globs along absolute and relative paths. Each path expanded from a glob gets assigned the threshold from a glob. Tests now use mock-fs to mock filesystem for glob testing. If match is not found, thresholds would not be met.
@krishnagoth
Copy link
Contributor Author

@cpojer hi! I added glob support a while ago. Did I misunderstand you and it is not what you meant?

@krishnagoth
Copy link
Contributor Author

krishnagoth commented Aug 24, 2017

Also, after I rebased on latest master to resolve conflicts, CI is failing with

/home/circleci/jest/packages/jest-cli/src/reporters/coverage_reporter.js
  34:1  error  'glob' should be listed in the project's dependencies. Run 'npm i -S glob' to add it  import/no-extraneous-dependencies

but 'glob' is in package.json? Did I do something wrong?

@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

@krishnagoth packages/jest-cli/package.json is where it'll need to be added.

@krishnagoth
Copy link
Contributor Author

@cpojer too bad there's no "d'oh" reaction emoticon.

@cpojer cpojer merged commit 90a79fc into jestjs:master Aug 24, 2017
@cpojer
Copy link
Member

cpojer commented Aug 24, 2017

Thanks for this diff. @krishnagoth: would you like to send a follow-up PR to extend the documentation with this feature? (See the docs/ folder in this repo)

@krishnagoth
Copy link
Contributor Author

Will do @cpojer

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants