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

adds ability to pass custom reporter options #9572

Merged
merged 5 commits into from
Feb 15, 2020

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented Feb 14, 2020

Istanbul-reporters recently changed their coverage reporting behaviour by introducing the projectRoot options.

This PR attempts to right the wrong and let a jest user configure the projectRoot right from the jest config by introducing the configuration through the well known tuple pattern.

Fixes #4103

@flovilmart flovilmart changed the title adds ability to pass custom reporter config adds ability to pass custom reporter options Feb 14, 2020
@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Merging #9572 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9572      +/-   ##
==========================================
+ Coverage   65.04%   65.08%   +0.03%     
==========================================
  Files         286      286              
  Lines       12140    12143       +3     
  Branches     3008     3009       +1     
==========================================
+ Hits         7897     7903       +6     
  Misses       3605     3605              
+ Partials      638      635       -3
Impacted Files Coverage Δ
packages/jest-reporters/src/coverage_reporter.ts 53.89% <100%> (+2.67%) ⬆️

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 f47ade4...bf6d69f. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Feb 14, 2020

This is great 👍 Could you add a test? And a changelog entry

@flovilmart
Copy link
Contributor Author

@SimenB I was troubled because I didn't spot any test initially on the Istanbul-reporters loading 🤦‍♂ . I added a test that covers both use cases (simple loading as well as tuple loading).

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 wonder if jest should always set projectRoot to config.rootDir? Wouldn't that fix #7176?

maxCols: process.stdout.columns || Infinity,
});
expect(istanbulReports.create).toHaveBeenCalledWith('lcov', {
maxCols: process.stdout.columns || Infinity,
Copy link
Member

Choose a reason for hiding this comment

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

Should be called with 10 no? I think it makes sense for user options to always override Jest's defaults

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 think it makes sense for user options to always override Jest's defaults

I wasn't sure what your stance would be about this option, I'll change it

@flovilmart
Copy link
Contributor Author

flovilmart commented Feb 15, 2020

I wonder if jest should always set projectRoot to config.rootDir? Wouldn't that fix #7176?

In the case of #7176, I believe the reporter involved is text-reporter and projectRoot is not set for this reporter. (https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-reports/lib/text-summary/index.js) It seems to be consistent with the implementation (I commented on the issue regarding this).

@drew-gross

This comment has been minimized.

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 great, thanks! We could set projectRoot, but that's separate

@SimenB SimenB merged commit 9d1236b into jestjs:master Feb 15, 2020
@flovilmart flovilmart deleted the reporter-option branch February 16, 2020 18:18
@flovilmart
Copy link
Contributor Author

Thanks @SimenB !

@flovilmart
Copy link
Contributor Author

@SimenB how does the release schedule usually look like? We have ugly workarounds in our codebase waiting for this patch, and I’d be happy to prep a release if is can help getting a patch out :)

@SimenB
Copy link
Member

SimenB commented Feb 16, 2020

Quite slow 🙁 I'll try to get a release out after we've verified fixes for #9457, but it's always slow as I don't have publish access and I need to get somebody at FB to make a release.

I recommend using patch-package or some such in the meanwhile.

@flovilmart
Copy link
Contributor Author

Thanks @SimenB !

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

Pass options to coverageReporter
5 participants