Support multipart extensions like ".test.js"

[jordanstephens]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

Allow extensions configuration option to support multipart extensions, ex: test.js. extension matching support is currently implemented by matching against node's path.extname which

returns the extension of the path, from the last occurrence of the . (period) character to end of string in the last portion of the path.

This prevents support for multipart extensions like .test.js.

By instead using String.prototype.endsWith, we can support the existing behavior while also enabling support for multipart extensions.

Alternate Designs

You could have also accomplished this by using a RegExp, I suppose, but endsWith is simpler.

Why should this be in core?

Even without adding support for multipart extensions, I think this implementation is simpler than the existing one for the currently supported use-case. But because multipart extensions are relatively common, and the extensions configuration is the only way I can see to support restricting input to these files without resorting to more intricate globbing, I think opening up support for this is also a benefit.

Benefits

Support for multipart extensions, simpler implementation.

Possible Drawbacks

I don't see any from here.

Applicable issues

I looked for relevant issues and was surprised that not to find any. If I have overlooked something, let me know.

Thanks!

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.1%) to 94.075% when pulling 41ec48d on jordanstephens:multipart-extensions into b3eb2a6 on mochajs:master.

[boneskull]

Thanks for the PR. To be clear: is using globs a workaround for this?

[jordanstephens]

Thanks for the PR. To be clear: is using globs a workaround for this?

@boneskull sure it's possible to match the files I'm looking for using globs—you could use globs instead of the existing extensions configuration option as well—but it can be incredibly cumbersome. I like the extensions feature because it allows me to easily scope a test run to a directory path without having to tack a glob pattern at the end of it every time.

Let me tell you about our project setup: We have many widgets each with their own groups of modules. Tests are nested within the src tree adjacent to their source files. We often want to test one widget at a time or one module or a collection of either. So yes, we could run mocha with the following way right now:

% mocha ./widget-a/**/*.test.js
% mocha ./widget-a/src/group-b/**/*.test.js
# etc...

If I run that last example in zsh, I'll run these tests:

./widget-a/src/group-b/foo.test.js
./widget-a/src/group-b/group-b1/bar.test.js

But if I run this in bash, I'll only run this one:

./widget-a/src/group-b/group-b1/bar.test.js

Unless I'm running bash >= v4 (and I have globstar enabled).

It's a huge pain to have to specify this glob pattern all the time when we want to easily scope our test runs by directory path, and I would like to codify this testing pattern in our project README without having to enumerate subtle differences between shells. This led me to try to configure this behavior via the extensions option where I discovered that multipart extensions were not supported.

I want to be able to run the following:

% mocha ./widget-a
% mocha ./widget-a/src/group-b
# etc...

Without mocha trying to read all of the *.js files adjacent to the *.test.js files I have in the tree.

Given that you already offer the extension configuration (presumably to ease the pain of this very issue), it does not seem unreasonable to expect support for multipart extensions as well.

Thanks!

[boneskull]

Ah... well, you can quote your globs--which will be handled by the glob module--and you won't have to worry about the shell (or even if it runs on Windows).

mocha "src/foo/**/*.test.js"

should work fine either way. likewise you can throw this into your .mocharc.json:

{ "spec": "blah/**/*.test.js" }

[boneskull]

(relying on the presence of a certain shell or configuration thereof for glob parsing is--as you have found out--fraught with peril)

[jordanstephens]

Ah, I hadn't realized that you could have mocha expand the globs from a positional argument—but even so, I would still be required to type **/*.test.js at the end of every path I want to test every day.

I'm not doing this for the "run every test in the project for CI" scenario. Your proposals would work find in that case. I'm concerned with the "run tests in this directory" scenario because I'm iterating on something locally and I'm trying to keep my feedback loop tight.

I'm trying to extend the supported configuration functionality so that I can avoid having to type **/*.test.js to the end of every argument I might want to pass. I am hoping to be able to run mocha ./foo/bar or mocha ./baz/qux and have mocha only match **/*.test.js no matter where in my project I am testing.

[boneskull]

I see; thanks for explaining your use-case further. Given that hasMatchingExtname() only operates on files as determined by a call to fs.stat(), this looks OK. Otherwise we could run into trouble if you have, say, a directory name matching the extension (e.g., project/.test.js/foo.test.js). Why that'd be a thing, I don't know, but it's a case that path.extname() handles (it still returns .js); as written, hasMatchingExtname() would return test.js/foo.test.js. maybe it'd be good to add a comment noting this limitation? Otherwise, LGTM

[jordanstephens]

maybe it'd be good to add a comment noting this limitation?

I'm having trouble writing a comment, because I don't think I understand the limitation you are perceiving.

we could run into trouble if you have, say, a directory name matching the extension (e.g., project/.test.js/foo.test.js). Why that'd be a thing, I don't know, but it's a case that path.extname() handles (it still returns .js)

I'm not sure I follow you here. path.extname has no notion of the type of thing at the path (or if anything even exists at that path), and neither does hasMatchingExtname.

If we want to prevent directories from being matched, I think we do exactly as we have done in this module and stat the path to see what's there.

Would you prefer to rename this to something like pathHasMatchinExtname to make it clearer that this just operates on "paths" and has no notion of the thing at the path?

[boneskull]

I think I'm confused too. nevermind 😄

this looks ok. I think I want to change this so it supports a leading . as well, but I'll deal with that later

[boneskull]

@jordanstephens merged #4472 which allows extensions to be specified as js, .js, or foo.js or .foo.js

[jordanstephens]

@boneskull wow! I found the lack of the leading dot to be strange as well, but I wasn't ready to take it on. Thanks for taking on that extra bit of work!