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

feat: rework .nycrc to support --all #226

Merged
merged 3 commits into from
Apr 30, 2019
Merged

feat: rework .nycrc to support --all #226

merged 3 commits into from
Apr 30, 2019

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Apr 18, 2019

what is this?

This reworks our .nycrc to better support the flag --all; this lets us know we've missed writing coverage for files:

Screen Shot 2019-04-18 at 3 47 18 PM

Note: this update requires nyc@14.

oddities

I think the rules:

"src/*{/*,/**/*}.js",
"src/*/v*/*.js"

were an attempt to not collect coverage in the src/v1, src/v3beta1 directory, etc.

  1. "src/*{/*,/**/*}.js", feels a little heavy handed, and was stopping us from collecting coverage for some libraries.
  2. do we not want to collect coverage for the file3s in src/v1? seems like in a perfect world we'd eventually have tests that exercise these APIs?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 18, 2019
@JustinBeckwith JustinBeckwith added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 18, 2019
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

Good start! Just flagging this as do not merge so we make sure the updates to all other libs go out first.

"src/*{/*,/**/*}.js",
"src/*/v*/*.js",
"test/**/*.js",
"build/test"
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice this doesn't do anything funky with TypeScript. Are we getting coverage from what's in build/src, or from src/? How do we know this isn't being counted twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coverage uses source-maps, so in reports it will point the user to the files in src/ as the files missing coverage; in practice it’s always the files in build/src that are executed, but will not show up in reports due to them including source-maps.

@JustinBeckwith
Copy link
Contributor

So this LGTM. Just please submit a hammer PR first, then lets land this one.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 25, 2019
@bcoe bcoe removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 30, 2019
@bcoe bcoe merged commit cd54e14 into master Apr 30, 2019
@bcoe bcoe deleted the nycrc-update branch April 30, 2019 01:16
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants