Make sure `mocha -h` consistently shows help #2746

Zarel · 2017-03-17T18:17:09Z

Previously, having options in test/mocha.opts would add those to process.argv to be passed to program.parse, which would make it stop showing help.

Fixes #2745

jsf-clabot · 2017-03-17T18:17:10Z

All committers have signed the CLA.

Zarel · 2017-03-17T18:20:40Z

The problem is when program.parse is called at line 189 of bin/_mocha:

program.parse(process.argv);

process.argv has been filled with the contents of test/mocha.opts

['/usr/local/bin/node', '/Users/zarel/Workspace/mocha/bin/_mocha', 'test', '-h']

commander seems to only show help if process.argv is exactly

['/usr/local/bin/node', '/Users/zarel/Workspace/mocha/bin/_mocha', '-h']

coveralls · 2017-03-17T18:37:12Z

Coverage increased (+2.2%) to 88.154% when pulling 993853e on Zarel:patch-2 into b4ebabd on mochajs:master.

ScottFreeCode · 2017-03-18T01:30:52Z

This looks good to me; @mochajs/core do we want to add a test for this behavior?

dasilvacontin · 2017-03-18T01:34:18Z

There should be, imo.

boneskull · 2017-05-12T05:17:18Z

@Zarel Thank you for the PR. Would you be willing to add a test (probably somewhere in here)?

Zarel · 2017-05-12T07:30:44Z

@boneskull, I tried. I'm not sure if I did it the way you'd prefer, but this is definitely a test that fails without this commit and passes with it.

Zarel · 2017-05-12T07:35:39Z

I'm not sure why the test failed in AppVeyor. Does anyone have advice? (It works on my local computer.)

coveralls · 2017-05-12T07:54:35Z

Coverage increased (+2.3%) to 88.298% when pulling 29883fd on Zarel:patch-2 into b4ebabd on mochajs:master.

ScottFreeCode · 2017-05-14T05:31:55Z

I can't see the actual failure in the AppVeyor test either...

Travis is only failing the new test on the oldest version of Node. Unfortunately, its not too clear why it's failing (except that it's the assertion that failed) since the assertion library used in that test file is boolean-based. It may be necessary to switch it to one of the other assertion libraries used in Mocha's tests that supports a contains assertion, in order to find out what is wrong there.

Zarel · 2017-05-14T12:18:21Z

@ScottFreeCode can you do that? I don't really know how to.

kunagpal · 2017-05-31T21:07:44Z

test/integration/options.spec.js

+
+  describe('--help', function () {
+    it('works despite the presence of mocha.opts', function (done) {
+      var output = '';


This initialization is redundant since an implicit casting is being done on line 195.

kunagpal · 2017-05-31T21:09:09Z

test/integration/options.spec.js

+        var mochaLoc = path.resolve(__dirname, '../../bin/mocha');
+        output = '' + childProcess.execSync(mochaLoc + ' -h', {cwd: path.resolve(__dirname, 'test-env')});
+      } catch (e) {}
+      try {


The sequence of try...catch blocks here are for setup and teardown, and should be shifted to before and after blocks, outside this it block.

kunagpal · 2017-05-31T21:12:07Z

test/integration/options.spec.js

+        fs.rmdirSync(path.resolve(__dirname, 'test-env'));
+      } catch (e) {}
+      assert(output.indexOf('Usage:') >= 0);
+      done();


The entire test is synchronous, and as such, the done callback is not needed here.

kunagpal · 2017-05-31T21:22:09Z

@Zarel What @ScottFreeCode was referring to was tweaking:

assert(output.indexOf('Usage:') >= 0)

to something like:

expect(output).to.contain('Usage:');

Here, expect here is an available global, documented here: https://github.com/Automattic/expect.js.

Additionally, you could also log the actual output so that we can see what exactly is coming up in the CI builds.

ScottFreeCode · 2017-06-14T20:14:59Z

Sorry about the late followup...

Additionally, you could also log the actual output so that we can see what exactly is coming up in the CI builds.

This should happen automatically if a more detailed assertion is used such as that contains suggestion, and that avoids interleaving console logs with the test output.

Aside from that, @Zarel, @kunagpal's suggestions all look good to me at a glance; feel free to try applying any/all of them.

This might also end up having to be... merged a bit with other updates that have been made to master; but I'd be happy to take care of that.

ScottFreeCode · 2017-06-14T20:19:59Z

Actually, looked at the conflict and -- seriously -- GitHub was confused because two entirely separate sets of tests were added to the same file? It couldn't figure out to just keep both additions? Eh, whatever; I went ahead and resolved that.

coveralls · 2017-06-14T21:26:01Z

Coverage increased (+0.01%) to 88.405% when pulling 406a0f0 on Zarel:patch-2 into 7647e18 on mochajs:master.

Zarel · 2017-06-14T21:26:22Z

It looks like the problem for AppVeyor is:

'C:\projects\mocha\bin\mocha' is not recognized as an internal or external command,
operable program or batch file.

Is Mocha built/run a different way in Windows?

coveralls · 2017-06-14T21:41:33Z

Coverage increased (+0.01%) to 88.405% when pulling 45143c8 on Zarel:patch-2 into 7647e18 on mochajs:master.

coveralls · 2017-06-15T00:23:49Z

Coverage increased (+0.01%) to 88.405% when pulling b9c8c1e on Zarel:patch-2 into 7647e18 on mochajs:master.

ScottFreeCode · 2017-06-15T00:29:23Z

The Travis failure on Node 0.10 turns out to be due to child_process.execSync not existing till Node 0.11...

The Windows error... sounds to me as though it is trying to run the Mocha script as though it were an executable?

I'm sure we have some helpers in Mocha's testsuite somewhere that will run a separate CLI instance of Mocha properly on both OSes and all versions of Node; let me see if I can find them and get back to you.

ScottFreeCode · 2017-06-15T03:45:33Z

So, I found an integration test helper and used it to do this (added a commit to the branch), although... it runs Mocha from the current working directory, so if we wanted to be absolutely sure that the opts file contains "foo" (rather than presuming that a mocha.opts file at all would trigger the issue we're fixing), then we'd have to tweak the helper to make it possible to override the current working directory. Hmm.

coveralls · 2017-06-15T03:59:39Z

Coverage increased (+0.01%) to 88.405% when pulling 336661c on Zarel:patch-2 into 7647e18 on mochajs:master.

Zarel · 2017-06-15T04:01:26Z

Unfortunately, only certain things in mocha.opts trigger this issue. foo is one of those things.

Zarel · 2017-06-22T06:22:43Z

Still, though, this level of struggling seems excessive to leave a bug unfixed. (I don't have mocha options memorized and it's mildly annoying to have to cd out of my project directory to look up help for it.)

Zarel · 2017-10-17T05:06:22Z

It seems weird to leave a bug unfixed because your test framework doesn't support testing the fix?

boneskull · 2017-10-17T17:19:29Z

@ScottFreeCode does this need anything or can it be merged?

Previously, having options in `test/mocha.opts` would add those to `process.argv` to be passed to `program.parse`, which would make it stop showing help. Fixes mochajs#2745

…rrent working directory, so make test/mocha.opts be available if necessary without overwriting existing one if any

coveralls · 2017-10-17T23:28:36Z

Changes Unknown when pulling 8621eb0 on Zarel:patch-2 into ** on mochajs:master**.

Zarel · 2017-10-17T23:41:10Z

Rebased to fix the merge conflict.

Zarel · 2017-10-17T23:41:20Z

For the record, I have trouble testing this; I get a different error every time I run npm test; most recently:

17 10 2017 18:39:16.173:INFO [launcher]: Starting browser PhantomJS
17 10 2017 18:39:16.218:ERROR [karma]: { Error: spawn ENOTDIR
    at _errnoException (util.js:1021:11)
    at ChildProcess.spawn (internal/child_process.js:325:11)
    at exports.spawn (child_process.js:494:9)
    at spawnWithoutOutput (/Users/zarel/Workspace/mocha/node_modules/karma/lib/launchers/process.js:168:24)
    at Object.ProcessLauncher._execCommand (/Users/zarel/Workspace/mocha/node_modules/karma/lib/launchers/process.js:76:21)
    at Object.PhantomJSBrowser._start (/Users/zarel/Workspace/mocha/node_modules/karma-phantomjs-launcher/index.js:80:10)
    at Object.<anonymous> (/Users/zarel/Workspace/mocha/node_modules/karma/lib/launchers/process.js:19:10)
    at emitOne (events.js:120:20)
    at Object.emit (events.js:210:7)
    at Object.BaseLauncher.start (/Users/zarel/Workspace/mocha/node_modules/karma/lib/launchers/base.js:42:10)
    at Object.j (/Users/zarel/Workspace/mocha/node_modules/karma/lib/launcher.js:116:17)
    at Object.setTimeout.bind.j (/Users/zarel/Workspace/mocha/node_modules/qjobs/qjobs.js:143:18)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:306:5)
    at Timer.listOnTimeout (timers.js:266:5)
 code: 'ENOTDIR', errno: 'ENOTDIR', syscall: 'spawn' }
make: *** [test-browser-unit] Error 1

….opts

ScottFreeCode · 2017-10-18T03:14:11Z

I think the initial test was correct but didn't work on Windows, then my attempt to use the existing cross-platform helpers (because Node really makes portable spawning/forking difficult) worked on Windows but didn't actually prove that the fix works (unlike the original test).

I verified this by pulling down the code and trying the test with and without the fix in the source. So... had to think about it a while, but I should have something that can test this correctly cross-platform and will be pushing it up shortly. If that passes CI we should go ahead and merge this already -- it's waited longer than it should have.

coveralls · 2017-10-18T03:20:26Z

Changes Unknown when pulling da32727 on Zarel:patch-2 into ** on mochajs:master**.

ScottFreeCode · 2017-10-18T03:47:57Z

All CI passed except linting (oops, should have done that before pushing); pushed a fix for the linting error, going to merge.

* Make sure `mocha -h` consistently shows help Previously, having options in `test/mocha.opts` would add those to `process.argv` to be passed to `program.parse`, which would make it stop showing help. Fixes mochajs#2745

drazisil added type: bug a defect, confirmed by a maintainer pr-needs-tests labels Mar 30, 2017

kunagpal reviewed May 31, 2017

View reviewed changes

Zarel force-pushed the patch-2 branch from b9c8c1e to 406a0f0 Compare June 14, 2017 21:11

boneskull added the pr-needs-work label Oct 17, 2017

boneskull removed the pr-needs-tests label Oct 17, 2017

Make sure mocha -h consistently shows help

d98abd3

Previously, having options in `test/mocha.opts` would add those to `process.argv` to be passed to `program.parse`, which would make it stop showing help. Fixes mochajs#2745

Zarel force-pushed the patch-2 branch from 336661c to 8621eb0 Compare October 17, 2017 23:17

Zarel and others added 3 commits October 17, 2017 18:18

Add tests for mocha -h fix

46112fa

Improve test for mocha -f fix

cb6b159

Try using the helpers to invoke Mocha; this will require using the cu…

8621eb0

…rrent working directory, so make test/mocha.opts be available if necessary without overwriting existing one if any

Fix test so that it actually proves the behavior with the right mocha…

da32727

….opts

Fix linting [ci skip]

1cb9f90

ScottFreeCode merged commit da901da into mochajs:master Oct 18, 2017

boneskull removed the pr-needs-work label Dec 12, 2017

boneskull added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Dec 29, 2017

boneskull added this to the v4.1.0 milestone Dec 29, 2017

Zarel mentioned this pull request Apr 30, 2023

Policy Suggestion: Require Mechanics Fixing PRs to Have Unit Tests smogon/pokemon-showdown#8868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure `mocha -h` consistently shows help #2746

Make sure `mocha -h` consistently shows help #2746

Zarel commented Mar 17, 2017

jsf-clabot commented Mar 17, 2017 •

edited

Zarel commented Mar 17, 2017 •

edited

coveralls commented Mar 17, 2017

ScottFreeCode commented Mar 18, 2017

dasilvacontin commented Mar 18, 2017

boneskull commented May 12, 2017

Zarel commented May 12, 2017

Zarel commented May 12, 2017 •

edited

coveralls commented May 12, 2017

ScottFreeCode commented May 14, 2017

Zarel commented May 14, 2017

kunagpal May 31, 2017

kunagpal May 31, 2017

kunagpal May 31, 2017

kunagpal commented May 31, 2017 •

edited

ScottFreeCode commented Jun 14, 2017

ScottFreeCode commented Jun 14, 2017

coveralls commented Jun 14, 2017

Zarel commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 15, 2017

ScottFreeCode commented Jun 15, 2017

ScottFreeCode commented Jun 15, 2017

coveralls commented Jun 15, 2017

Zarel commented Jun 15, 2017 •

edited

Zarel commented Jun 22, 2017

Zarel commented Oct 17, 2017

boneskull commented Oct 17, 2017

coveralls commented Oct 17, 2017

Zarel commented Oct 17, 2017

Zarel commented Oct 17, 2017

ScottFreeCode commented Oct 18, 2017

coveralls commented Oct 18, 2017

ScottFreeCode commented Oct 18, 2017

Make sure mocha -h consistently shows help #2746

Make sure mocha -h consistently shows help #2746

Conversation

Zarel commented Mar 17, 2017

jsf-clabot commented Mar 17, 2017 • edited

Zarel commented Mar 17, 2017 • edited

coveralls commented Mar 17, 2017

ScottFreeCode commented Mar 18, 2017

dasilvacontin commented Mar 18, 2017

boneskull commented May 12, 2017

Zarel commented May 12, 2017

Zarel commented May 12, 2017 • edited

coveralls commented May 12, 2017

ScottFreeCode commented May 14, 2017

Zarel commented May 14, 2017

kunagpal May 31, 2017

Choose a reason for hiding this comment

kunagpal May 31, 2017

Choose a reason for hiding this comment

kunagpal May 31, 2017

Choose a reason for hiding this comment

kunagpal commented May 31, 2017 • edited

ScottFreeCode commented Jun 14, 2017

ScottFreeCode commented Jun 14, 2017

coveralls commented Jun 14, 2017

Zarel commented Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 15, 2017

ScottFreeCode commented Jun 15, 2017

ScottFreeCode commented Jun 15, 2017

coveralls commented Jun 15, 2017

Zarel commented Jun 15, 2017 • edited

Zarel commented Jun 22, 2017

Zarel commented Oct 17, 2017

boneskull commented Oct 17, 2017

coveralls commented Oct 17, 2017

Zarel commented Oct 17, 2017

Zarel commented Oct 17, 2017

ScottFreeCode commented Oct 18, 2017

coveralls commented Oct 18, 2017

ScottFreeCode commented Oct 18, 2017

Make sure `mocha -h` consistently shows help #2746

Make sure `mocha -h` consistently shows help #2746

jsf-clabot commented Mar 17, 2017 •

edited

Zarel commented Mar 17, 2017 •

edited

Zarel commented May 12, 2017 •

edited

kunagpal commented May 31, 2017 •

edited

Zarel commented Jun 15, 2017 •

edited