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.

Sign up for GitHub

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

Jump to bottom

Accept globs as excludes #71

Merged
merged 1 commit into from
Jan 13, 2016
Merged

Accept globs as excludes #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ with:

coffeeCoverage --exclude 'node_modules,.git,test' ...

You can also use globs. If you have your specs next to your code you might e.g. use:

coffeeCoverage --exclude 'node_modules,.git,**/*.spec.coffee'

#### --path

Only used when `--inst jscoverage` is specified. Path can be given one of three different
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"argparse": "^1.0.2",
"coffee-script": ">=1.6.2",
"lodash": "^3.7.0",
"glob": "^6.0.4",
"pkginfo": ">=0.2.3"
},
"devDependencies": {
Expand Down
10 changes: 10 additions & 0 deletions src/utils/helpers.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ fs = require 'fs'
path = require 'path'
_ = require 'lodash'
{EXTENSIONS} = require '../constants'
glob = require 'glob'

exports.stripLeadingDotOrSlash = (pathName) -> pathName.replace(/^\//, "").replace(/^\.\//, "")

Expand Down Expand Up @@ -64,6 +65,15 @@ exports.excludeFile = (fileName, options) ->
# Only instrument files that are inside the project.
excluded = true

# For each exclude value try to use it as a pattern to exclude files
exclude.map (pattern) ->
glob.sync pattern,
Copy link
Contributor

Choose a reason for hiding this comment

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

The one thing that's bad about this is that we'll end up re-running glob.sync for the same variables over and over again. But, without rewriting half this project, I guess this works. :P I'll merge this, and see if I can come up with a clever way to make this a little better. We could cache the result of glob.sync() for each value in options.exclude...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be fine: glob caches these kind of requests internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's cool. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I'm 90% through making this work by just doing one call to glob. Haha. :P I also made one very small change - historically this would let you specify an include starting with a "/" where / was the project root. This works the same as, say, .gitignore or .npmignore. If we pass in root: basePath in addition it cwd: basePath, then we get exactly this behavior - the leading / is effectively optional. I'm also going to strip out the old style "prefix match" code - if we're going to make a breaking change, may as well break it good. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Have a look at #73 and tell me what you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not completely sure where you're heading. If you could explain a little, I might be able to help :)
Not sure though, if glob is the real performance bottleneck for coffee-coverage :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to make this "all glob based", and get rid of the prefix matching found here. It's a bit weird that we prefix match glob patterns.

So, I figured I'd take each match returned by glob, add a '/' to the end, and compare against that. The problem is, this works fine when your exclude list is ['/node_modules'](since the matched globs will be the same list.) When your exclude list is ['/node_modules/*/'], on the other hand, glob returns potentially thousands of strings for us to compare against. In my largish project, this was the difference between a 9 second run and a 20 second run.

I think I'm just trying to optimize prematurely though. :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I get it. And thinking about it I realized that I was thinking about the problem all wrong: If we're just excluding files there is no need to hit the filesystem at all to make the comparison, which should significantly speed up the globbing process and allow for something like what you just suggested.
I'll be working on a PR on one of the next days :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Thanks, sir. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#74 it is :)

dot: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why dot: true, BTW?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without dot: true, patterns that should normally include files starting with . would not include these files. That is fine if you are using patterns to include files, but if you use patterns to exclude files you usually want the dot files to be excluded, too.

cwd: basePath
.forEach (file) ->
if relativeFilename is path.normalize file
excluded = true

components = relativeFilename.split path.sep
for component in components
if component in exclude
Expand Down
17 changes: 17 additions & 0 deletions test/tests.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,23 @@ describe "Coverage tests", ->
expect(global[COVERAGE_VAR]['a/foo.coffee'], "Should instrument a/foo.coffee").to.exist
expect(global[COVERAGE_VAR]['b/bar.coffee'], "Should not instrument b/bar.coffee").to.not.exist

it "should exclude files based on globs when dynamically instrumenting code", ->

coffeeCoverage.register(
path: "relative"
basePath: path.resolve __dirname, '../testFixtures/testWithExcludes'
exclude: ["**/*r.coffee"]
coverageVar: COVERAGE_VAR
log: log
)

require '../testFixtures/testWithExcludes/a/foo.coffee'
require '../testFixtures/testWithExcludes/b/bar.coffee'

expect(global[COVERAGE_VAR], "Code should have been instrumented").to.exist
expect(global[COVERAGE_VAR]['a/foo.coffee'], "Should instrument a/foo.coffee").to.exist
expect(global[COVERAGE_VAR]['b/bar.coffee'], "Should not instrument b/bar.coffee").to.not.exist

it "should handle nested recursion correctly", ->
# From https://github.com/benbria/coffee-coverage/pull/37
instrumentor = new coffeeCoverage.CoverageInstrumentor({
Expand Down