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

#5067 Add forceCoverageRegex configuration option #5081

Merged
merged 1 commit into from
Dec 18, 2017
Merged

#5067 Add forceCoverageRegex configuration option #5081

merged 1 commit into from
Dec 18, 2017

Conversation

efegurkan
Copy link
Contributor

Summary

As explained in #5067 if you have tests in source files there is no way to collect coverage from those files since jest returns false for should instrument checks.

This PR adds forceCoverageRegex config option to allow collecting coverage even
if the file is ignored. This way you can collect coverage from the
source files with tests or specific files you ignored manually.

Test plan
Create a file which includes the tests with source files as following:

// sum.t.js

export function sum(a,b) {
  return a + b;
}

if (process.env.NODE_ENV === 'test') {
  test('sum', () => {
    expect(sum(1, 2)).toBe(3);
  });
}

Add 'forceCoverageRegex' field to the jest options same as testRegex. Run jest with collectCoverage option.

In this PR I add unit tests for jest-runtime/src/should_instrument.js also.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is failing on Appveyor.

Could you also update the changelog?

{
...
"jest": {
"forceCollectCoverage": ".*\\.t.js$"
Copy link
Member

Choose a reason for hiding this comment

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

Should it include pattern in the name to indicate it's a regexp?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like it's called forceCoverageRegex elsewhere, can you make sure it's consistent?

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 actually wrote it wrong on the docs :) it is forceCoverageRegex. Just updated the PR

@codecov-io
Copy link

Codecov Report

Merging #5081 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5081      +/-   ##
==========================================
+ Coverage   60.65%   60.76%   +0.11%     
==========================================
  Files         199      199              
  Lines        6643     6645       +2     
  Branches        4        4              
==========================================
+ Hits         4029     4038       +9     
+ Misses       2614     2607       -7
Impacted Files Coverage Δ
packages/jest-config/src/normalize.js 91.95% <ø> (ø) ⬆️
packages/jest-config/src/defaults.js 100% <ø> (ø) ⬆️
packages/jest-config/src/valid_config.js 100% <ø> (ø) ⬆️
packages/jest-config/src/index.js 23.8% <ø> (ø) ⬆️
packages/jest-runtime/src/should_instrument.js 100% <100%> (+41.17%) ⬆️

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 cfb7ee1...3681ee9. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I see no reason to not support this. Thanks for the PR!

@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

I have a few concerns about the naming here. Jest's options normally use "Pattern" rather than "Regex", even if they are regex. Most users only use testMatch which is based on globs. Should we change this option to forceCoverageMatch and use globs instead?


Default: `''`

The pattern to enforceCollecting coverage even if the file is in the ignored
Copy link
Member

Choose a reason for hiding this comment

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

Can you rewrite this? What about "Test files are normally ignored from collecting code coverage. With this option, you can overwrite this behavior and include otherwise ignored files in code coverage."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. 👍

@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

One more thought: if this is primarily to enable coverage for test files, should we instead make this a boolean flag "includeTestsInCoverage" instead of having people fumble with regex/globs?

@efegurkan
Copy link
Contributor Author

@cpojer for the "includeTestsInCoverage" concerns of yours, I have some cases which we have regular tests like __tests__/something.spec.js and source files with tests like packages/src/sourcefile.t.js. You probably don't want to add all of your tests to the coverage.

It is probably a better idea to use globs though I agree, I will update the PR accordingly

Add forceCoverageMatch config option to allow collecting coverage even
if the file is ignored. This way you can collect coverage from the
source files with tests or specific files you ignored manually.
@efegurkan
Copy link
Contributor Author

Hey I updated the PR and changed regex to Glob.

@cpojer cpojer merged commit 9afeb9c into jestjs:master Dec 18, 2017
@cpojer
Copy link
Member

cpojer commented Dec 18, 2017

Thanks for the PR and for making changes to it :)

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

5 participants