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

Add a setting for collecting coverage #330

Closed
wants to merge 4 commits into from
Closed

Add a setting for collecting coverage #330

wants to merge 4 commits into from

Conversation

stephtr
Copy link
Member

@stephtr stephtr commented May 21, 2018

Jest/#5836 enabled switching whether or not Jest should collect coverage information by a workspace attribute. This PR adds a setting to control that attribute.

Points to talk about:

  • I defaulted the (boolean) setting to null, since supplying false would add the --no-coverage flag.
  • Toggling coverage overlay may not display the overlay if collectCoverage isn't true, it therefore shows a warning message in that case. That behavior isn't perfect if collectCoverage has been kept at null and the jest command/config has been modified to nevertheless collect the information.
  • The same is true for showCoverageOnLoad, but without a warning message.

An alternative would be that, additional to the coverageCollect switch, the setting could be temporarily overwritten when toggling the coverage overlay.

By the way: Sorry that I accidentally pushed these commits to the master branch, that shouldn't have happened, but I had my local branch tracking upstream instead of origin.

@coveralls
Copy link

coveralls commented May 21, 2018

Pull Request Test Coverage Report for Build 393

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 75.432%

Files with Coverage Reduction New Missed Lines %
src/JestExt.ts 3 53.13%
Totals Coverage Status
Change from base Build 391: -0.2%
Covered Lines: 641
Relevant Lines: 811

💛 - Coveralls

@connectdotz
Copy link
Collaborator

just to play devil's advocate, why do we want to force users to learn yet another coverage flag if they can already put it in their settings (like jest.pathToJest) or package.json script with --coverage...?

@stephtr
Copy link
Member Author

stephtr commented May 21, 2018

One concrete use case would be if the default value of pathToJest would be fine for a project, one doesn't have to manually add the path to jest.
We furthermore don't force the user to use the new setting (maybe I should remove the warning message), he could still specify the --coverage flag within pathToJest.
I think it's generally cleaner to separate the path setting from the executable's extra arguments (like pathToConfig, which can be also supplied via a setting or as a flag).

@connectdotz
Copy link
Collaborator

I think it's generally cleaner to separate the path setting from the executable's extra arguments (like pathToConfig, which can be also supplied via a setting or as a flag).

I like this idea a lot better, and I think it will probably give us more bang for the buck in the long run... But can you give us a little bit more background on what exactly the problem we are trying to solve with this PR?

@stephtr
Copy link
Member Author

stephtr commented May 21, 2018

There's no problem with the current status, it would be just a simplification/enhancement. But maybe it would be better to continue the discussion in #226.

@connectdotz
Copy link
Collaborator

oh i see, will do

@seanpoulter
Copy link
Member

I prefer having folks configure Jest separate from configuring the UI. Jest has great documentation. If anything, I’d want to help change the config for users.

@stephtr
Copy link
Member Author

stephtr commented May 21, 2018

In what direction would you then like #226 to go?

@stephtr stephtr closed this May 21, 2018
@seanpoulter
Copy link
Member

I'm not sure @stephtr and while it's a good conversation to have I'd urge us to focus on our core functionality instead of the nice to have issues. While trying to fix a few regressions we've introduced in the last few weeks I seen all of these:

  • test decorators not updating properly
  • inline error messages that aren't being cleared
  • colour escape codes appearing in messages
  • the status bar message that hangs
  • inline error messages in the wrong spot

I'll skim #226 and give my two cents.

@stephtr stephtr deleted the coverage-option branch July 1, 2018 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants