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.

Sign up for GitHub

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

Jump to bottom

[Document]: Error/undefined params to the 'done' callback #3192

Merged
merged 2 commits into from Jan 9, 2018
Merged

[Document]: Error/undefined params to the 'done' callback #3192

merged 2 commits into from Jan 9, 2018

Conversation

maraisr
Copy link
Contributor

@maraisr maraisr commented Jan 3, 2018 •
edited

Resolves: #3134

@jsf-clabot
Copy link

jsf-clabot commented Jan 3, 2018 •
edited

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling 0a3569d on maraisr:fixes/issue-3134 into dc12bd5 on mochajs:master.

boneskull
boneskull suggested changes Jan 3, 2018
View reviewed changes
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added some clarification

docs/index.md Outdated
@@ -155,7 +155,7 @@ Mocha allows you to use any assertion library you wish. In the above example, w

## Asynchronous Code

Testing asynchronous code with Mocha could not be simpler! Simply invoke the callback when your test is complete. By adding a callback (usually named `done`) to `it()`, Mocha will know that it should wait for this function to be called to complete the test.
Testing asynchronous code with Mocha could not be simpler! Simply invoke the callback when your test is complete. By adding a callback (usually named `done`) to `it()`, Mocha will know that it should wait for this function to be called to complete the test. This callback accepts both an `Error` instance, or `undefined`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3134 isn't quite accurate about what needs to be documented.

We need to document that any non-falsy, non-Error instance passed into done() will cause Mocha to fail.

Subclasses of Error are allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if that is accurate?

docs/index.md Outdated
@@ -184,6 +184,7 @@ describe('User', function() {
});
```


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove

@boneskull boneskull added area: documentation anything involving docs or mochajs.org semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Jan 3, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling ccddce5 on maraisr:fixes/issue-3134 into a723b8f on mochajs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling ccddce5 on maraisr:fixes/issue-3134 into a723b8f on mochajs:master.

@maraisr
Copy link
Contributor Author

maraisr commented Jan 4, 2018

@boneskull do you think we can get this merged? Kinda excited to have committed something that was merged into Mocha.

boneskull
boneskull suggested changes Jan 4, 2018
View reviewed changes
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for updating this. I have a couple more issues, if you don't mind?

docs/index.md Outdated
@@ -171,7 +171,7 @@ describe('User', function() {
});
```

To make things even easier, the `done()` callback accepts an error, so we may use this directly:
To make things even easier, the `done()` callback also accepts an `Error` instance (ie, `new Error()`), so we may use this directly:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be i.e. not ie

docs/index.md Outdated
@@ -155,7 +155,7 @@ Mocha allows you to use any assertion library you wish. In the above example, w

## Asynchronous Code

Testing asynchronous code with Mocha could not be simpler! Simply invoke the callback when your test is complete. By adding a callback (usually named `done`) to `it()`, Mocha will know that it should wait for this function to be called to complete the test.
Testing asynchronous code with Mocha could not be simpler! Simply invoke the callback when your test is complete. By adding a callback (usually named `done`) to `it()`, Mocha will know that it should wait for this function to be called to complete the test. This callback accepts both an `Error` (or subclassed) instance, or `undefined`, any non-falsy will cause a failed test.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, let's just eliminate undefined altogether here. it's also a bit of a run-on sentence. how about:

This callback accepts both an `Error` instance (or subclass thereof) *or* a falsy value; anything else will cause a failed test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling 5e1cacf on maraisr:fixes/issue-3134 into a723b8f on mochajs:master.

@boneskull boneskull merged commit cb09e8b into mochajs:master Jan 9, 2018
@boneskull
Copy link
Member

thanks!

@boneskull boneskull added this to the v5.0.0 milestone Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boneskull boneskull boneskull approved these changes

Assignees
No one assigned
Labels
area: documentation anything involving docs or mochajs.org semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@maraisr @jsf-clabot @coveralls @boneskull