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

#1577 Add "--exclude" Option #3210

Merged
merged 12 commits into from
Apr 1, 2018
Merged

#1577 Add "--exclude" Option #3210

merged 12 commits into from
Apr 1, 2018

Conversation

alexbainter
Copy link

@alexbainter alexbainter commented Jan 18, 2018

Description of the Change

Adds option --exclude to exclude matched files.
For example, this will match any files ending with "*.spec.js" unless they're inside the "node_modules" directory:

mocha "**/*.spec.js" --exclude "node_modules/**"

This required a small change to bin/mocha.js. As the file arguments are evaluated by mocha, they are checked against the pattern(s) provided by --exclude. Any files matching any of the patterns in --exclude are ignored.

I used minimatch to check the filenames against the patterns. This is the same library used by glob.

As noted in #1577, this isn't strictly necessary. mocha "./{,!(node_modules)/**/}*.spec.js" would have the same effect as above. However, the --exclude option is easier for users who don't speak glob.

Why should this be in core?

Situations like the above example are common. Other tools (like Karma) include this behavior as core functionality.

Benefits

This is easier for users to understand how to exclude specific files or patterns, giving them more granular control over which files are used for testing without having to use complex glob patterns.

Possible Drawbacks

This is probably less performant than using glob patterns, and it's yet another option to support.
(If there is a more performant approach for implementing this behavior I'd be happy to change my code).

Applicable issues

Resolves #1577.
Enhancement.

@jsf-clabot
Copy link

jsf-clabot commented Jan 18, 2018

CLA assistant check
All committers have signed the CLA.

@alexbainter alexbainter changed the title #1577 --exclude Option #1577 Add "--exclude" Option Jan 18, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling db0d1fd on metalex9:master into 2fe2d01 on mochajs:master.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

👍 Thanks for this! Great work.

Can you please address these changes?

  • --exclude should allow itself to be specified multiple times instead of expecting a comma-delimited value
  • ensure no unnecessary work is being done to check the type of newFiles on L500
  • move the filter on L503 to chain on the call to utils.lookupFiles() on L489
  • rename fixtures to have extension .fixture.js
  • see if you can use runMocha() instead of directInvoke() in options.spec.js

bin/_mocha Outdated
@@ -205,7 +206,8 @@ program
.option('--allow-uncaught', 'enable uncaught errors to propagate')
.option('--forbid-only', 'causes test marked with only to fail the suite')
.option('--forbid-pending', 'causes pending tests and test marked with skip to fail the suite')
.option('--file <file>', 'include a file to be ran during the suite', collect, []);
.option('--file <file>', 'include a file to be ran during the suite', collect, [])
.option('--exclude <files>', 'comma-delimited list of files or glob patterns to ignore', list, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer this works like --file above, where it can be provided multiple times.

if (typeof newFiles === 'string') {
newFiles = [newFiles];
}
newFiles = newFiles.filter(fileName => program.exclude.every(pattern => !minimatch(fileName, pattern)));
Copy link
Contributor

Choose a reason for hiding this comment

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

you may be able to just chain this (.filter and everything after) to L489.

@@ -0,0 +1,7 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

fixture files should all have the extension .fixture.js

* Calls handleResult with the result
*/
function runExcludeTest (args, handleResult, done) {
directInvoke(args, function (error, result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

unless there's a Good Reason to use directInvoke(), please use runMocha() instead

@@ -494,6 +496,13 @@ args.forEach(arg => {
throw err;
}

if (typeof newFiles !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think newFiles will be anything other than an Array of strings, will it?

Copy link
Author

@alexbainter alexbainter Jan 20, 2018

Choose a reason for hiding this comment

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

newFiles is the result of utils.lookupFiles which can return an array of strings, a string, or undefined.

@boneskull boneskull added semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal labels Jan 19, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling d17863b on metalex9:master into 2fe2d01 on mochajs:master.

@alexbainter
Copy link
Author

Changes made:

  • --exclude should allow itself to be specified multiple times instead of expecting a comma-delimited value
  • rename fixtures to have extension .fixture.js
  • use runMocha() instead of directInvoke() in options.spec.js

Regarding these two:

  • ensure no unnecessary work is being done to check the type of newFiles on L500
  • move the filter on L503 to chain on the call to utils.lookupFiles() on L489

newFiles is returned from utils.lookupFiles. If you look at the definition for lookupFiles in lib/utils.js L477, it looks to me like it could technically return an array of strings (L488, L523), a string (L495), or undefined (L499). Perhaps the documentation for that function needs to be updated?

Thanks for looking at this!

@boneskull boneskull added the area: node.js command-line-or-Node.js-specific label Apr 1, 2018
@boneskull
Copy link
Contributor

This looks good. Not worried about conflicting with --file, as that doesn't accept a glob.

@boneskull boneskull merged commit 8e740c5 into mochajs:master Apr 1, 2018
@boneskull boneskull added this to the v5.1.0 milestone Apr 1, 2018
@joaovieira
Copy link

@boneskull any idea on when v5.1.0 is gonna be released? This option comes in handy :)

sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
* Add --exclude as option.

* Add tests for --exclude option.

* Implement --exclude.

* Update --exclude description.

* Remove old exclude fixture.

* Add new exclude fixture.

* Fix tests for --exclude option.

* Add name to package.contributors.

* Filter new files before they're added.

* Allow multiple --exclude flags rather than using a comma-delimited list.

* Use .fixture extension for exclude fixtures.

* Use runMochaJSON (run) instead of of directInvoke.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants