-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
docs: Add warning about async callback for describe #5046
base: master
Are you sure you want to change the base?
Conversation
I couldn't find any reference to `describe` not supporting an async function. It seems like a natural idea given `it` and `before` do. I spent considerable time trying to figure out why an async test was failing before I discovered the reason deep in an issue discussion (mochajs#2975 (comment)), so hope that this helps others design their test suites!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a couple small touchups for typos.
Note that #4921 should also help with this. |
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify it a bit?
@@ -296,6 +296,10 @@ describe('#find()', function () { | |||
}); | |||
``` | |||
|
|||
### Limitations of asynchronous callbacks | |||
|
|||
You can use all three asynchronous patterns (`done`, `Promise`, and `async`/`await`) in callbacks for `it()` and for all hooks (`before()`, `after()`, `beforeEach()`, `afterEach()`), but `describe()` will not work correctly with an asynchronous callback -- it must be synchronous. This is because in the "parsing" cycle all `describe` callbacks are executed and the test hierarchy (`runner`) is created before any tests are run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use all three asynchronous patterns (`done`, `Promise`, and `async`/`await`) in callbacks for `it()` and for all hooks (`before()`, `after()`, `beforeEach()`, `afterEach()`), but `describe()` will not work correctly with an asynchronous callback -- it must be synchronous. This is because in the "parsing" cycle all `describe` callbacks are executed and the test hierarchy (`runner`) is created before any tests are run. | |
You can use all asynchronous callbacks (`done`, `Promise`, and `async`/`await`) in callbacks for `it()`, `before()`, `after()`, `beforeEach()`, `afterEach()`) but not `describe()` -- it must be synchronous. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for all changes except that I feel like understanding the 'why' really helped me here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm thinking its maybe getting a bit too much into the current implementation of things, I'll have a think about it.
If we include it then maybe as a new paragraph, like Technically this is caused by...
? Thoughts @mochajs/maintenance-crew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we can just say that they're synchronous / run synchronously. I don't think we need to go deep here.
I think we have two options that we can do well:
- Give a cursory explanation ("this is just how it is")
- Give a deep explanation on why
The deep explanation option would be far too much information for a root-level docs site. As a general ideology, one should never have to rely on deep architectural explanations in root-level docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, and it can always be found by looking in the GitHub history of that documentation line as it will reference this PR 😌
Requirements
Description of the Change
This is a documentation only change.
I couldn't find any reference to
describe
not supporting an async callback. It seems like a natural conclusion to draw, given mocha specializes in async tests, and givenit
and hook do.I spent considerable time trying to figure out why an async test was failing in one of my modules before I discovered the reason deep in an issue discussion (#2975 (comment))!
Alternate Designs
Why should this be in core?
This is a documentation change.
Benefits
I hope that this helps others design their test suites (I saw a number of comments about spending 'lots of time' trying to figure this out).
Possible Drawbacks
Applicable issues