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

Support multipart extensions like ".test.js" #4442

Merged
merged 4 commits into from Oct 9, 2020

Conversation

@jordanstephens
Copy link
Contributor

@jordanstephens jordanstephens commented Sep 11, 2020

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
Copy link

@jsf-clabot jsf-clabot commented Sep 11, 2020

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

@coveralls coveralls commented Sep 11, 2020

Coverage Status

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

@jordanstephens jordanstephens force-pushed the jordanstephens:multipart-extensions branch from e716568 to 41ec48d Sep 11, 2020
@boneskull
Copy link
Member

@boneskull boneskull commented Sep 11, 2020

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

@jordanstephens
Copy link
Contributor Author

@jordanstephens jordanstephens commented Sep 12, 2020

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
Copy link
Member

@boneskull boneskull commented Sep 14, 2020

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
Copy link
Member

@boneskull boneskull commented Sep 14, 2020

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

@jordanstephens
Copy link
Contributor Author

@jordanstephens jordanstephens commented Sep 15, 2020

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.

Copy link
Member

@boneskull boneskull left a comment

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
Copy link
Contributor Author

@jordanstephens jordanstephens commented Oct 1, 2020

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
Copy link
Member

@boneskull boneskull commented Oct 9, 2020

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 boneskull merged commit b216fcd into mochajs:master Oct 9, 2020
5 checks passed
5 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 94.013%
Details
licence/cla Contributor License Agreement is signed.
Details
netlify/mocha/deploy-preview Deploy preview ready!
Details
@boneskull boneskull added this to the v8.2.0 milestone Oct 9, 2020
@boneskull
Copy link
Member

@boneskull boneskull commented Oct 12, 2020

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

@jordanstephens
Copy link
Contributor Author

@jordanstephens jordanstephens commented Oct 12, 2020

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.