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

docs: Add warning about async callback for describe #5046

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/index.md
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Author

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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 😌


## Synchronous Code

When testing synchronous code, omit the callback and Mocha will automatically continue on to the next test.
Expand Down