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

guard against undefined timeout option in constructor; closes #3813 #3816

Merged
merged 1 commit into from
Mar 7, 2019

Conversation

boneskull
Copy link
Contributor

IMO Suite#timeout should eventually do nothing if its parameter is undefined. it should not be up to Mocha's constructor to guard against this.

Signed-off-by: Christopher Hiller <boneskull@boneskull.com>
@boneskull boneskull self-assigned this Mar 7, 2019
@boneskull boneskull added the type: bug a defect, confirmed by a maintainer label Mar 7, 2019
@boneskull
Copy link
Contributor Author

cleaned up the constructor unit tests a little while I was in there

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 91.629% when pulling f44ee90 on boneskull/issue/3813 into e654253 on master.

@plroebuck
Copy link
Contributor

As undefined is not a valid value for any Mocha option, should the user "options" object have them stripped before merged with default values?

Otherwise, won't this just turn into a game of whack-a-mole?

@boneskull
Copy link
Contributor Author

boneskull commented Mar 7, 2019

As undefined is not a valid value for any Mocha option, should the user "options" object have them stripped before merged with default values?

Otherwise, won't this just turn into a game of whack-a-mole?

I'm thinking no, because prior to v6, anybody trying to pass undefined values via options in a way which was not previously guarded against would find a thrown exception.

The problem here, in particular, is that I accidentally removed such a guard.

If someone tries this with some other option that doesn't handled undefined nicely, they will get an exception--not new behavior--and I suppose we can live with that for now.

@boneskull
Copy link
Contributor Author

A 0.06% decrease in coverage should be a warning. 0.08 means your IDE gets impounded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants