Do not silently fail on bad opts argument

[samhh]

For #2576.

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage decreased (-0.06%) to 88.125% when pulling a257b6b on SamHH:master into a2fc76c on mochajs:master.

[samhh]

Ah, missed the tests. If we want to cover that if (shouldThrowError) block, is this the right file for that? https://github.com/mochajs/mocha/blob/master/test/integration/options.spec.js

[ScottFreeCode]

(DISCLAIMER: the following is a thought on an impression after taking one look at the change and not thoroughly investigating the code. It would need at least as much review as the PR itself.)

Could this be equivalently fixed by removing the try-catch altogether and replacing it with only executing the contents of the try block if either the opts file was specified or the default opts file exists? I suppose that would make it also throw an error if there's an opts file but it can't be parsed or anything like that, but frankly, I think that would be a similar improvement to throwing an error when the specified file is missing. I wasn't around when this code was created, so I don't know for sure what was intended, but at a glance it looks as though try-catch is being used as a lazy hack to avoid error on the default file not existing instead of just checking for the file's existence, and I would point to it swallowing real errors as a perfect example of why try-catch should not be used that way in the first place... Adding more complex logic to determine when to rethrow the error after all just seems like the tail wagging the dog, unless I'm missing some legit rationale for suppressing errors other than "default file does not exist" (which I'm pretty sure could be properly tested instead).

[ScottFreeCode]

Re. tests -- that file does look like it's the place where commandline arguments (of which --opts is one) are tested, yes. In fact, even if we don't merge anything for this fix, we could use --opts tests added there, since there don't appear to be any...

[samhh]

I see what you mean, I do tend to steer clear of try/catch blocks in my own work. Figured though that for my first PR for a big project like mocha I should keep it adhering to what's already there haha. I can certainly try my hand at dropping the try/catch if consensus agrees that's the way to go. In which case, how far down in Node versions does Mocha support? Asking so I know what modern JS I can('t) use.

[ScottFreeCode]

@mochajs/core Any opinions on try-catch here?

Re. Node versions, I believe we're testing all the supported versions on Travis (so... looks like Node 0.10?), if I recall correct.

[boneskull]

I agree with @ScottFreeCode; let's just let it throw.

[dasilvacontin]

I'd rename this to wasOptsProvided. It makes the code / reasoning more clearer imo.

[dasilvacontin]

Nevermind the feedback in case the variable is no longer necessary.

[dasilvacontin]

@ScottFreeCode @boneskull Yeah, but how do you check if the file exists otherwise? (to get the default one, if it exists). fileExists and others have been deprecated. I recall not being able to check for file existence without using a method that throws. But maybe I'm wrong.

[ScottFreeCode]

Hmm... according to the docs: "Note that fs.exists() is deprecated, but fs.existsSync() is not." https://nodejs.org/dist/latest-v8.x/docs/api/fs.html#fs_fs_existssync_path

[boneskull]

you can use fs.stat() in place of fs.exists()

[ScottFreeCode]

So, fs.stat if asynchronous and fs.existsSync if synchronous?

[boneskull]

@ScottFreeCode ...apparently? Either way, checking file existence is usually an anti-pattern

[ScottFreeCode]

Usually; although using try-catch for program logic rather than just error handling is also an antipattern unless there's no good alternative... I guess it comes down to whether we're more concerned by the race condition that the file could be deleted in the split instant between the existence check and the attempt to use it, resulting in a clear error that doesn't recur on subsequent runs, or by the risk that we screw up the try-catch handling, resulting in things like this issue about errors being silently (!) suppressed that should not have been suppressed at all.

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!