Generic glob patterns for exclusions

[cgalvarez]

Description

Improve the default glob patterns for exclusions:

• **/.?*/**: excludes all hidden folders (must have a minimum of 1 character after the dot).
• **/node_modules/**: excludes all Node.js dependency modules.
• **/packages/!(local-test_?*.js): excludes all meteor package files except the one related to the current test (starting with local-test_....js).
• **/+([^:]):+([^:])/**: excludes other meteor package paths, like lmieulet:meteor-coverage/node_modules/depd/index.js.
• **/@(test|tests|spec|specs)/**: excludes folders containing test files (presumably).
• **/@(test.?*|?*.test.?*|?*.tests.?*|spec.?*|?*.spec.?*|?*.specs.?*|app-test.?*|?*.app-test.?*|?*.app-tests.?*|app-spec.?*|?*.app-spec.?*|?*.app-specs.?*): excludes test files according to the Meteor testing guide.

These patterns are applied with exclude.general, which is the second filter (after include) before instrumentation and report generation, so all these files are neither instrumented nor listed in the reports.

Meteor package(s) developers that want to tests their package(s) only need to include an item in the include array of the file .coverage.json inside their meteor package(s) with the following syntax: "**/packages/<author>_<package>.js". A perfect example can be seen in the .coverage.json file of this package:

{
  "include": [
    "**/packages/lmieulet_meteor-coverage.js"
  ]
}

These patterns make maintaining the code and developing Meteor code an easier task, because:

• You don't have to deal with and take care of the Meteor bundled packages (manually excluding them).
• You don't have to deal with and take care of the Node.js dependencies (manually excluding folders like node_modules or .npm).
• You don't have to worry about hidden folders that could contain scripts that distort the coverage (like .coverage/prettify.js.
• You don't have to exclude manually all your test files.

Because of instrumenter filters apply before instrumentation and before report generation, and their order is include -> exclude.general -> exclude.{server|client}, applying all previous patterns in exclude.general allows filtering both sides (instrumentation/reports) and shaves some loop cycles.

Motivation and Context

It's not a required change, but a very desirable one.

Right now each user testing a meteor app should exclude manually all the packages used by its app and meteor itself. This PR does this automatically, and exclude a lot of paths that potentially distort the coverage.

Meteor package(s) developers only need to include an item in the include array of the file .coverage.json inside their meteor package(s) with the following syntax: "**/packages/<author>_<package>.js".

This PR does not solve any specific problem, but increases the easiness of use of this package and prevents a lot of headaches, while helping to keep the coverage accurate.

How Has This Been Tested?

Tested with lmieulet:meteor-coverage and one package of my own. For lmieulet:meteor-coverage I saved the verbose stdout of the command before and after applying the PR, like this:

export COVERAGE_VERBOSE=1
spacejam test-packages ./ --coverage "out_lcovonly out_html" --driver-package practicalmeteor:mocha-console-runner > output_before.txt
spacejam test-packages ./ --coverage "out_lcovonly out_html" --driver-package practicalmeteor:mocha-console-runner > output_after.txt

Then I did a diff of output_before.txt and output_after.txt. The only differences I found were the configuration object (obvious, because it's the change this PR introduces) and that coffeescript.js was being instrumented before but not after (should be this way).

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change): User now has to include manually his/her package file when testing a package; if s/he doesn't include it, spacejam will fail. Nevertheless, every package needs some coverage customization to include/exclude paths, and this PR minimizes this effort considerably (to one only path!).

[serut]

Ok, I will give a try to this configuration. I don't see a single issue in your pull request and I share your point of view.

I'm not using meteor-coverage on a real meteor app, so correct me if I'm wrong, but each time there is a new dependency in an app, you need to add that dependency in the .coverage.json file . It's not something that I would like to do every time... With this conf, you just have to edit .coverage.json file when you create a new package or a new app, which is quite accepteable.

I will need to be rewritte the readme.md correspondly to merge this pull request 🎅

Again @cgalvarez , Thanks for this pull request. I will test it very soon.

[cgalvarez]

@serut I'm still testing only packages, cannot say anything proven about full meteor app testing, but I think nobody should create a custom .coverage.json with this configuration (because any package added with meteor add author:package should be matched with those glob patterns like happens with the Meteor bundled ones, and so with any test file).

The only time you should create that file is when you are testing meteor packages to include the final concatenated and minified (by meteor) version of your package(s) under test (done with include: "**/packages/lmieulet_meteor-coverage.js") or to change the output folder.

readme.md

I can rewrite the readme.md file and merge into this PR. I think the Config file section should be rewritten with the following issues (I want to know what you think about it):

• include, exclude.general, exclude.(client|server) are applied in that order before both, instrumentation and coverage report.
  In your example code it's stated that exclude.client excludes before client side instrumentation, exclude.server excludes before server side instrumentation, and exclude.general excludes anything from coverage report, but I've found this is not that way.

  You just have to take a look at Instrumenter.shouldIgnore.
  It's the only code chunk which uses the include/exclude configuration.
  And when called from CoverageData.filterCoverageReport, it applies before coverage report the same filters as before instrumentation.
  The method exits on first filter match, without applying the remaining ones, so you can leverage this to override any undesired behavior (because include it's always the first filter).

• Another fact is that coverage report includes instrumented files and files included in their source maps when they exist. If this were not this way, you should not see the referenced files in your coverage report (you only instrument lmieulet_meteor-coverage.js, but you get in your report coverage a lot of files like main.js, report-service.js and so on).

• Users should be warned that customizing a visible output folder (a non-hidden one starting with a dot .) may cause report generation errors, because of firing a meteor live reloading in the middle of a report generation.

  If user request multiple report types, the initial (on boot) folder creation may fire the live reloading, and after that any of the report generators, because they create files. So it's just a matter of what it takes to notice the file system change by Meteor.
  I've suffered this issue, and the only way that I've found to overcome that issue is using a hidden folder like the default one of yours (.my-reports or whatever).

• Users should know what folders are excluded by default to avoid headaches:

  • hidden folders like .npm, .coverage or .meteor.
  • special folders like node_modules.
  • all meteor packages (bundled and/or manually installed ones) like meteor/underscore, meteor/accounts-password or aldeed:simple-schema.
  • all tests file(s) like *.(specs?|tests?|app-specs?|app-tests?).* or (specs?|tests?|app-specs?|app-tests?).*, and all folder(s) like (specs?|tests?|app-specs?|app-tests?).

What do you think about? Anything to add/remove?

[serut]

I already edited a lot the README yesterday but I didn't published it yet. Thanks for these recommandations I will add them.

[thiagodelgado111]

I really like this PR, nice work @cgalvarez and @serut! LGTM!

[cgalvarez]

@serut Some more suggestions for readme.md regarding istanbul (I know this is out of the scope of this project, but it is very helpful):

• Do you get confused with the colors of the istanbul reports? Read this closed issue.
• To have an accurate coverage, you will need how to ignore code chunks. For example, if you code an if block without else, istanbul marks the else branch as not covered (although it doesn't exist), decreasing your code coverage, which is false.

And another one that users will only know if they read spacejam docs:

• The following two commands are equivalent, but the second one is shorter and thus less typing error-prone:

  spacejam test-packages ./ --driver-package practicalmeteor:mocha-console-runner --coverage "out_lcovonly out_html"
  spacejam-mocha test-packages ./ --coverage "out_lcovonly out_html"

[serut]

published with version0.9.6