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

GPII-2493: Added file exclusion patterns to .nycrc for generating test coverage report for Windows code #1

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@klown
Copy link

commented Apr 13, 2018

Hi @the-t-in-rtf,

While this started as a concern for test/code coverage of the windows implementation of the process reporter components, it ended up as tweaking the .nycrc file to include/exclude files for the coverage report. I think it's close after a lot of fiddling, but some points:

This is a pull request against your GPII-2493 branch, not GPII's master. As such it won't automatically start CI, and I don't know how to make that happen (invite @gpii-bot here, maybe?)

It's based on the .nycrc file in universal, using the same "exclude-to-include" syntax.

A copy of the report is available from my repository. The last entry is for "root/node_modules/gpii-universal/gpii/node_modules/userListeners/src/index.html", which is inside the "node_modules". According to the nyc documentation, the "node_modules" directory is automatically excluded. That is the case here except for this one instance. I've tried various patterns, but it always shows up in the report. Maybe it's supposed to be included?

Anyhow, hope that's useful.

GPII-2493: Updated .nycrc file for `npm test` script.
Added file exclusion patterns for generating a report about test
coverage.
],
"exclude": [
"!**/node_modules/"
"!**/gpii/node_modules/WindowsUtilities/WindowsUtilities.js",

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

Why is the content in ./gpii/node_modules being excluded? It's code checked into the repo, it should be tested and covered.

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

I guess you're excluding things that should only be hit by node from the browser tests, I see that now.

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

Sorry, got lost in the similar syntax with this and gpii-testem. You're definitely excluding node code from node coverage reporting, which should not be the case.

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

Sorry, I missed the negated part of these. I see what you're doing, although I'd still like to confirm with a copy of your branch. More in a bit.

],
"exclude": [
"!**/node_modules/"

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

Why is ./node_modules not being excluded? You need something to exclude that.

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

So, now that I'm awake enough, I realise that we're talking about nyc and its blood feud with **/node_modules/**. I also see what you're doing with the negated exclusion, but I'm not 100% positive that it's doing what you want. I'll have to check out your branch and look at the reports.

.nycrc Outdated
".nyc-output",
"gpii.js",
"Gruntfile.js",
"index.js"

This comment has been minimized.

Copy link
@the-t-in-rtf

the-t-in-rtf Apr 16, 2018

Owner

index.js should not be excluded from the nyc tests.

@the-t-in-rtf

This comment has been minimized.

Copy link
Owner

commented Apr 16, 2018

OK, I managed to run the report with the settings provided. The negates seem like a reasonable starting point. Here's a screenshot of the overview page.

screen shot 2018-04-16 at 16 45 49

For starters, index.js isn't showing up in the report, and it should. There's also content coming in from ./node_modules/gpii-universal that needs to be excluded. In previous work I had to avoid negating the main rule that screens out ./node_modules and instead add a negated exclude for every single directory I wanted to include from gpii/node_modules. My guess is you're most of the way there, you'd just need to back out the negated rule for **/node_modules/*.

@klown

This comment has been minimized.

Copy link
Author

commented Apr 16, 2018

@the-t-in-rtf I haven't had a lot time to work on this today, but here a few quick questions regarding your last comment.

OK, I managed to run the report with the settings provided. The negates seem like a reasonable starting point. Here's a screenshot of the overview page. ...

That screenshot closely matches the one I linked to in my original comment. The main difference is that the urls in my report all start with "root" -- for example: "root/gpii/node_modules/WindowsUtilities" They start with "gpii" in your screen shot. Did you change anything in your run?

... In previous work I had to avoid negating the main rule that screens out ./node_modules and instead add a negated exclude for every single directory I wanted to include from gpii/node_modules. My guess is you're most of the way there, you'd just need to back out the negated rule for **/node_modules/*

The documentation for nyc states:

Note: Since version 9.0 files under node_modules/ are excluded by default. add the exclude rule !**/node_modules/ to stop this.

By "main rule that screens out ./node_modules" do you mean that documented default exclusion? Also, I don't see how I could back out the negated rule for "**/node_modules/*" since there isn't such a rule in the .nycrc I pushed.

Meanwhile, I'll put index.js back in, see what it does, and see what else I can tweak.

@the-t-in-rtf

This comment has been minimized.

Copy link
Owner

commented Apr 17, 2018

By "main rule that screens out ./node_modules" do you mean that documented default exclusion? Also, I don't see how I could back out the negated rule for "**/node_modules/*" since there isn't such a rule in the .nycrc I pushed.

Yes, I meant the implied exclusion. I missed the minus sign there, that was something you removed, and is why the node_modules content is found in the final report.

That screenshot closely matches the one I linked to in my original comment. The main difference is that the urls in my report all start with "root" -- for example: "root/gpii/node_modules/WindowsUtilities" They start with "gpii" in your screen shot. Did you change anything in your run?

I didn't change anything, I ran npm test from the root of the repo in a Vagrant VM.

Merge branch 'GPII-2493'
* GPII-2493:
  GPII-2493: Added workaround for "closeProcessByName" tests failing with exit code 0.
  GPII-2493: Added `listeners` directory to `.gitignore`.
  GPII-2493: Removed built listeners.
  GPII-2493: Fixed broken package.json dependencies following recent merge.
  GPII-2493: Fixed bulid issues following merge with master.
  GPII-2493: Removed sub-module tests from the coverage report.
  GPII-2495: Made SPI tests work again.
  GPII-2493: Updated istanbul command to instrument all the code in the `gpii` subdirectory.
  Added custom istanbul excludes so that code in gpii/node_modules is included in code coverage reports.
  GPII-2493: Added an `npm test` script, including code coverage reporting.

klown added some commits Apr 20, 2018

GPII-2493: Improved .nycrc file, but still not quite right.
- Added negative exclusion patterns for .js files under gpii/node_modules,
- Added "node_modules" and "node_modules/gpii-universal/**" exclusion patterns,
- Removed top-level "index.js" from exclusion patterns.

The report, however, still includes some items from "node_modules/gpii-universal/**"
@klown

This comment has been minimized.

Copy link
Author

commented Apr 20, 2018

Closing this pull request; replaced by pull #172.

@klown klown closed this Apr 20, 2018

the-t-in-rtf pushed a commit that referenced this pull request Oct 10, 2018

Merge pull request #1 from JavierJF/GPII-2212
GPII-2212: Fixed tests execution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.