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

Merged
merged 3 commits into from
Jul 2, 2024
Merged

docs: Add warning about async callback for describe #5046

merged 3 commits into from
Jul 2, 2024

Conversation

mcdurdin
Copy link
Contributor

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 (#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 ✨ <[email protected]>
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?

docs/index.md Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
Member

👋 @mcdurdin is this still something you have time for? Just checking in!

Co-authored-by: Josh Goldberg ✨ <[email protected]>
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.

I'll go ahead and take the liberty of merging this now, as it's a docs change. Thanks for the productive conversation @mcdurdin! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit c43930c into mochajs:main Jul 2, 2024
2 checks passed
@mcdurdin mcdurdin deleted the patch-1 branch July 5, 2024 07:10
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.

3 participants