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 #172

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@klown
Copy link
Contributor

klown commented Apr 20, 2018

@the-t-in-rtf, @amb26 Theses changes to .nycrc here were originally part of a pull request against Tony's GPII-2439 branch/pull. His pull was merged into master yesterday. I've rebased my pull against the current windows master and created this pull request in its stead.

My original pull request and dialogue with Tony is here:
the-t-in-rtf#1

Also, the changes to .nycrc are not quite right, but more coverage is reported than before. Unfortunately, it pulls in some stuff (not all) under /node_modules/gpii-universal. A copy of the report generated by this pull is available:
https://clown.idrc.ocad.ca/GPII/windows/reports/index.html

klown added some commits Apr 13, 2018

GPII-2493: Updated .nycrc file for `npm test` script.
Added file exclusion patterns for generating a report about test
coverage.
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/**"
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

gpii-bot commented Apr 20, 2018

"!**/gpii/node_modules/spiSettingsHandler/index.js",
"!**/gpii/node_modules/spiSettingsHandler/src/SpiChildProcess.js",
"!**/gpii/node_modules/spiSettingsHandler/src/SpiSettingsHandler.js",
"!**/gpii/node_modules/userListeners/index.js",

This comment has been minimized.

@the-t-in-rtf

the-t-in-rtf May 7, 2018

Member

These rules are exactly why the universal content under ./node_modules are included in the overall report.

This comment has been minimized.

@klown

klown May 14, 2018

Author Contributor

Thanks for the pointers, @the-t-in-rtf . But, I don't understand what you mean by adjusting lines 21-24. Taking the nihilistic approach, I tried deleting them altogether and seeing what happens. Here's what I get:

  1. there is no reportage for spiSettingsHandler gpii module even though it has tests under "./gpii/node_modules/spiSettingsHandler/tests/". By "no reportage", I mean there not even a row in red suggesting that it has poor coverage -- nothing shows up in the table.

  2. even though userListeners's index.js file has been removed (line 24), the report continues to show results from node_modules, i.e., "./node_modules/gpii-universal/node_modules/userListeners/src".

  3. the report continues to include processReporter code from under node_modules.

Secondly, can you explain why these need adjustment? Compare them with the displaySettingsHandler on lines 8-11, repeated below. It's the same pattern, namely, the index.js file followed by each individual source file under the src directory:

"!**/gpii/node_modules/displaySettingsHandler/index.js",
"!**/gpii/node_modules/displaySettingsHandler/src/dpiWindows8.js",
"!**/gpii/node_modules/displaySettingsHandler/src/displaySettingsHandler.js",
"!**/gpii/node_modules/displaySettingsHandler/src/dpiWindows10.js",

Why does that work where the same pattern for spiSettingsHandler does not?

Here's a copy of the report I get when I delete lines 21-24:
http://clown.idrc.ocad.ca/GPII/windows/GPII-2493/reports/

This comment has been minimized.

@the-t-in-rtf

the-t-in-rtf May 15, 2018

Member

The problem is that you only want to negate the exclude for things that are in gpii/node_modules, and not in node_modules/gpii-universal/gpii/node_modules. There's no display settings handler in universal, so that's not a problem. The problem with userListeners (and processReporter) is that both projects have that subdirectory under gpii/node_modules.

Negated excludes are a kind of trump card, they will always win, so anything you "include" that way should be tightly tailored. What you need is to be much more specific with the rules for the process reporter and user listeners. Try using ./gpii instead of **/gpii for those for example.

This comment has been minimized.

@the-t-in-rtf

the-t-in-rtf May 15, 2018

Member

Just to be clear, I'm not 100% sure why displaySettingsHandler still shows up without those rules. Contrary to all previous experience, the include for gpii/node_modules/** might be working for once, not sure there. You might simply be able to remove the negated excludes for the processReporter as well.

This comment has been minimized.

@klown

klown May 15, 2018

Author Contributor

I might have been unclear, but, for the record, the negative exclusion rules I cited for displaySettingsHandlers were not removed. I used those rules as an example to ask why they do not involve gpii-universal whereas the same rules pattern for spiSettingsHandlers does. And, you've answered that question. To put it bluntly:

  1. there is no displaySettingsHandlers module in ./gpii-universal/gpii/node_modules, and
  2. there is an spiSettingsHandlers module in both ./gpii/node_modules and ./gpii-universal/gpii/node_modules, and
  3. the double star, **, in the exclusion rule selects both:
    3.1 "./gpii/node_modules/", and
    3.2 "./node_modules/gpii-universal/gpii/node_modules"

Actually, this implies a way around the problem, namely, don't use the same module name within both gpii-universal and the platform OS (windows, linux, android, and so on). For example, there is a deviceReporter module in gpii-universal, but there is no such thing in linux nor windows. In linux, the device reporter implementation is handled by the packagekit module, and by the registryResolver module in windows. The names of the modules are completely different here. In contrast, the process reporter module name is processReporter in all three repositories (universal, linux, and windows). If I had to do it over again, I'd individuate the module names and at least use linuxProcessReporter and windowProcessReporter to distinguish it from universal's processReporter module name. And thereby avoid headaches from trying to parse/wrestle-with global regular expressions.

Hmmm... this might be the way forward...

@the-t-in-rtf

This comment has been minimized.

Copy link
Member

the-t-in-rtf commented May 7, 2018

Thanks for resubmitting, @klown. I commented inline on the lines I think need to be adjusted to fix the ./node_modules/gpii-universal content that's getting pulled into the report.

klown added a commit to klown/clown.idrc.ocad.ca that referenced this pull request May 14, 2018

Added test code report based on removal of lines 21-24, namely:
"!**/gpii/node_modules/spiSettingsHandler/index.js",
"!**/gpii/node_modules/spiSettingsHandler/src/SpiChildProcess.js",
"!**/gpii/node_modules/spiSettingsHandler/src/SpiSettingsHandler.js",
"!**/gpii/node_modules/userListeners/index.js",

See Tony's comment:
GPII/windows#172 (review)
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.