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

Conversation

mcdurdin
Copy link

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

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 given it 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

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!
Copy link

linux-foundation-easycla bot commented Dec 12, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

docs/index.md Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 4, 2024
@JoshuaKGoldberg
Copy link
Member

Note that #4921 should also help with this.

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
Copy link
Member

@voxpelli voxpelli left a 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.
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 😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author waiting on response from OP - more information needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants