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

Fix duplication of mocha.opts on process.argv #1910

Merged
merged 1 commit into from
Oct 21, 2015

Conversation

nexdrew
Copy link

@nexdrew nexdrew commented Sep 29, 2015

Found by @jameswomack (see yargs issue 267), mocha is reading options from test/mocha.opts and adding them to process.argv twice - once in bin/mocha and then again in bin/_mocha. This PR removes the 2nd one and adds a unit test to cover this scenario.

Wasn't quite sure where to put the unit test, but test/integration/regression.js seems like a decent place.

Let me know if there's anything you'd like me to change. Thanks.

@danielstjules
Copy link
Contributor

This could be an issue for those who invoke mocha using _mocha rather than the normal bin (unfortunate, but I've seen it a lot)

@nexdrew
Copy link
Author

nexdrew commented Sep 29, 2015

@danielstjules Hmm, ok. Would you recommend adding something to bin/options.js then, to check if mocha.opts has already been read/applied?

@danielstjules
Copy link
Contributor

Could we keep it in _mocha, and remove the duplicate from mocha instead?

@danielstjules
Copy link
Contributor

Woops, sorry! :) Having a slow morning

@nexdrew
Copy link
Author

nexdrew commented Sep 29, 2015

@danielstjules Yeah, that should work. I will fix and push soon.

@nexdrew nexdrew force-pushed the fix/mocha-opts-dupe branch from fa80d9b to 11f4d9c Compare September 29, 2015 16:08
@nexdrew
Copy link
Author

nexdrew commented Sep 29, 2015

@danielstjules Should be good now, I think.

@nexdrew
Copy link
Author

nexdrew commented Oct 7, 2015

Any more feedback on this?

@nexdrew nexdrew force-pushed the fix/mocha-opts-dupe branch from 11f4d9c to f8294e6 Compare October 7, 2015 13:29
@jameswomack
Copy link

LGTM 🚢 🇮🇹

@nexdrew
Copy link
Author

nexdrew commented Oct 21, 2015

Looks like there are several PRs stuck in limbo. Anything I can do to help?

@travisjeffery
Copy link
Contributor

Could've sworn there was a reason they were duplicated, but perhaps not.

travisjeffery pushed a commit that referenced this pull request Oct 21, 2015
Fix duplication of mocha.opts on process.argv
@travisjeffery travisjeffery merged commit 20ebec8 into mochajs:master Oct 21, 2015
@nexdrew nexdrew deleted the fix/mocha-opts-dupe branch October 21, 2015 13:31
@nexdrew
Copy link
Author

nexdrew commented Oct 21, 2015

@travisjeffery Thanks!

@mattfysh
Copy link

thanks for the fix :) any idea when this will be published to npm?

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

Successfully merging this pull request may close these issues.

5 participants