feat: deprecation message when an async callback is used in a Suite #4921

alcuadrado · 2022-09-13T21:48:46Z

I'm sending this PR after having proposed this feature in #4920, getting some positive reactions, and researching the history of a similar feature that was removed

Description of the Change

This PR introduces a change that prints a depreciation message if an async callback is used in a Suite.

Alternate Designs

v6.0.0 introduced a similar feature, but it printed a deprecation warning if a Suite returned any value other than undefined, which was not practical in some cases (see #3744), so it was removed.

This PR is a specialization of that feature that prints a warning when Promises are returned instead.

Also, I originally proposed throwing a fatal error, but that would be a breaking change, so I changed it to a deprecation message. This same criteria was applied to the previous version of this feature.

Why should this be in core?

I don't think this can be implemented outside of core.

As this PR benefits users who don't have a clear understanding of how a Suite works, taking this functionality out of core will have much lower, as there wouldn't be a clear path for those users to discover how to enable it.

Benefits

Many users don't realize that Suite's callbacks can't be async. In some cases, they get away with async callbacks, which further increases the confusion.

This PR is intended to clarify that confusion.

Possible Drawbacks

I don't think this version of the feature has any serious drawback.

It may print deprecation warnings for people who were using async callbacks in Suites with some success (and a ton of luck), but those users were already at high risk of their code breaking. This change is intended to help them.

Applicable issues

Fixes #4920
semver-patch
(copying the semver criteria of #3550)

linux-foundation-easycla · 2022-09-13T21:48:49Z

The committers listed above are authorized under a signed CLA.

✅ login: alcuadrado / name: Patricio Palladino (9425614, b7412b3, 61e34db, ba71bcb)

test/integration/fixtures/suite/suite-async-callback.fixture.js

juergba · 2022-09-20T17:20:48Z

@alcuadrado thank you, I will review within the next few days.

I certainly don't agree with the type of warning (deprecation), since we have never offered a feature "asynchronous describe".

juergba · 2022-09-20T18:15:17Z

@wdavidw are you still working with Mocha and CoffeeScript?
I don't understand your comment in #3744:

One way to please everyone is to detect if the return value found in the suite is not the same as the one returned by it.

Does this PR work with CoffeeScript? What happens if the last it in a describe returns a Promise?

alcuadrado · 2022-09-20T19:12:06Z

I certainly don't agree with the type of warning (deprecation), since we have never offered a feature "asynchronous describe".

I copied this from the previous implementation. I guess the reasoning behind that is that some users are getting away with async callbacks now, and a fatal error can be seen as breaking by them.

coveralls · 2022-09-27T17:25:28Z

Coverage increased (+0.003%) to 94.329% when pulling 9b338f6 on alcuadrado:issue/4920 into 41567df on mochajs:master.

juergba

@alcuadrado I'm quite sure this change will trigger deprecation warnings to CoffeeScript users in certain circumstances. Anyway this PR is much softer than the original proposal and there is a work-around by adding a return statement to the suites. Otherwise they will have to live with a single deprecation warning.

Could you please do some manual browser testing, since we don't run our integration tests on browsers. And confirm the outcome, thank you.

lib/interfaces/common.js

JoshuaKGoldberg

Looks good as a general re-implementation of #3550! Heck, I even think we should go further and fully re-implement to check all truthy values - not just Promises.

Thoughts?

lib/interfaces/common.js

JoshuaKGoldberg · 2024-07-02T16:58:26Z

👋 ping @alcuadrado, is this still something you have time for?

alcuadrado · 2024-07-03T14:47:36Z

Hey, sorry I dropped this. I can take a look over the weekend.

iirc I dropped this as I don't have the necessary setup to run the browser tests locally.

Co-authored-by: Franco Victorio <[email protected]>

alcuadrado · 2024-07-04T23:47:41Z

I rebased against main, added the "[mocha]" tag to the deprecation message, but I can't get the tests to run, not even in main.

I'm using [email protected], [email protected]. Maybe using a devcontainer is bringing problems?

alcuadrado · 2024-07-04T23:51:38Z

Sorry, maybe I was wrong. I was getting messages like this one

/tmp/mocha-9cPS1H/test.js:7
    done((;
          ^

SyntaxError: Unexpected token ';'
    at wrapSafe (node:internal/modules/cjs/loader:1281:20)
    at Module._compile (node:internal/modules/cjs/loader:1321:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at Module.require (node:internal/modules/cjs/loader:1233:19)
    at require (node:internal/modules/helpers:179:18)
    at /workspaces/mocha/mocha/lib/mocha.js:414:36
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/workspaces/mocha/mocha/lib/mocha.js:411:14)
    at Mocha.run (/workspaces/mocha/mocha/lib/mocha.js:972:10)
    at Object.run (/workspaces/mocha/mocha/lib/cli/watch-run.js:263:22)
    at FSWatcher.<anonymous> (/workspaces/mocha/mocha/lib/cli/watch-run.js:184:14)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

and tons of

⚠ [mocha] cleaning up, please wait...
ℹ [mocha] waiting for changes...
ℹ [mocha] waiting for changes...
ℹ [mocha] waiting for changes...
⚠ [mocha] cleaning up, please wait...
ℹ [mocha] waiting for changes...
⚠ [mocha] cleaning up, please wait...
ℹ [mocha] waiting for changes...
ℹ [mocha] waiting for changes...

yet the tests seem to pass.

alcuadrado · 2024-07-04T23:53:34Z

Regarding checking against !== undefined. While I like the idea, I haven't implemented that yet, as that's a bigger product change, and I don't feel in a position to make that decision.

I'll gladly implement that and update the tests if that is a change that you (as in the maintainers) are willing to accept it.

JoshuaKGoldberg · 2024-07-06T23:08:28Z

can't get the tests to run

Do you mean for the GitHub PR? If so: we have to click an Approve and run every time you push a new commit. I've done that just now.

alcuadrado · 2024-07-08T19:20:25Z

can't get the tests to run

Do you mean for the GitHub PR? If so: we have to click an Approve and run every time you push a new commit. I've done that just now.

No, sorry, the error messages during the test run confused me.

Should I do anything about the netlify deploy preview failure? I don't have permission to see the details

JoshuaKGoldberg · 2024-07-08T19:25:10Z

Should I do anything about the netlify deploy preview failure? I don't have permission to see the details

No, that's just hanging around. ou don't have to worry about that.

alcuadrado · 2024-07-29T21:55:49Z

Anything I can do to help move this forward?

JoshuaKGoldberg · 2024-08-03T13:12:09Z

Oh, sorry, I'd misinterpreted your messages and didn't realize this was ready for re-review! Thanks for the ping. Added back to our queue of PRs to review.

Edit: and, https://github.com/mochajs/mocha/pull/4921/files#r1511274453 is still in discussion, I believe?

Co-authored-by: Josh Goldberg ✨ <[email protected]>

alcuadrado · 2024-08-03T18:39:58Z

Edit: and, https://github.com/mochajs/mocha/pull/4921/files#r1511274453 is still in discussion, I believe?

Yes, I think so. I'm not familiar with the decision process for things like this within mocha. But anyway, I committed your suggestion, assuming that this will probably/hopefully be accepted.

JoshuaKGoldberg · 2024-08-28T00:44:24Z

lib/interfaces/common.js

+            errors.deprecate(
+              '[mocha] Suite "' +
+                suite.fullTitle() +
+                '" returned a Promise. ' +


A Promise

Now that it's !== undefined above, this'll need to be updated.

JoshuaKGoldberg · 2024-08-28T00:44:47Z

lib/interfaces/common.js

+              '[mocha] Suite "' +
+                suite.fullTitle() +
+                '" returned a Promise. ' +
+                'Asynchronous suites are not supported, use a ' +


Asynchronous

...and this too.

JoshuaKGoldberg · 2024-08-28T00:46:10Z

test/integration/suite.spec.js

[Testing] This has a very clean + good unit test for the async (returning a Promise) case. But since the common.js change now is the more general !== undefined check, could you also add a unit test for a non-undefined, non-Promise value?

JoshuaKGoldberg

Progress! 🚀

Now that the check is for any non-undefined value, rather than just Promises, the messaging will need to be updated.

JoshuaKGoldberg · 2024-10-08T14:45:29Z

👋 Ping @alcuadrado, is this still something you have energy & time for?

JoshuaKGoldberg · 2024-10-29T20:36:20Z

Closing out to free up the backing issue. @alcuadrado if you end up seeing this and having time again, no worries, feel free to re-open the PR or send a new one.

If anybody else sees this message and there isn't a new PR, feel free to send one yourself. Just make sure to add a co-author attribution if it takes code from this PR.

Cheers all! 🤎

fvictorio reviewed Sep 13, 2022

View reviewed changes

test/integration/fixtures/suite/suite-async-callback.fixture.js Outdated Show resolved Hide resolved

juergba added area: usability concerning user experience or interface semver-minor implementation requires increase of "minor" version number; "features" labels Sep 27, 2022

juergba added this to the next milestone Sep 27, 2022

juergba reviewed Sep 27, 2022

View reviewed changes

lib/interfaces/common.js Show resolved Hide resolved

lib/interfaces/common.js Outdated Show resolved Hide resolved

juergba removed this from the next milestone Oct 9, 2022

JoshuaKGoldberg requested changes Mar 4, 2024

View reviewed changes

lib/interfaces/common.js Outdated Show resolved Hide resolved

JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 4, 2024

JoshuaKGoldberg changed the title ~~Deprecation message when an async callback is used in a Suite~~ feat: deprecation message when an async callback is used in a Suite Mar 4, 2024

JoshuaKGoldberg mentioned this pull request Mar 4, 2024

docs: Add warning about async callback for describe #5046

Merged

Wodann mentioned this pull request Mar 27, 2024

Sporadic test failures when running hardhat test due to incorrect usage of chai matchers when hardhat >= 2.21.0 NomicFoundation/hardhat#5036

Closed

JoshuaKGoldberg added the stale this has been inactive for a while... label Jul 2, 2024

alcuadrado and others added 3 commits July 4, 2024 23:29

Deprecation message when an async Suites are used

9425614

Update test/integration/fixtures/suite/suite-async-callback.fixture.js

ba71bcb

Co-authored-by: Franco Victorio <[email protected]>

Add "[mocha]" tag to the deprecation message

b7412b3

alcuadrado force-pushed the issue/4920 branch from 9b338f6 to b7412b3 Compare July 4, 2024 23:32

Update lib/interfaces/common.js

61e34db

Co-authored-by: Josh Goldberg ✨ <[email protected]>

JoshuaKGoldberg reviewed Aug 28, 2024

View reviewed changes

JoshuaKGoldberg requested changes Aug 28, 2024

View reviewed changes

JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed and removed status: needs review a maintainer should (re-)review this pull request labels Aug 28, 2024

JoshuaKGoldberg added the stale this has been inactive for a while... label Oct 8, 2024

JoshuaKGoldberg closed this Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deprecation message when an async callback is used in a Suite #4921

feat: deprecation message when an async callback is used in a Suite #4921

alcuadrado commented Sep 13, 2022

linux-foundation-easycla bot commented Sep 13, 2022 •

edited

Loading

juergba commented Sep 20, 2022 •

edited

Loading

juergba commented Sep 20, 2022

alcuadrado commented Sep 20, 2022

coveralls commented Sep 27, 2022

juergba left a comment •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Jul 2, 2024

alcuadrado commented Jul 3, 2024

alcuadrado commented Jul 4, 2024 •

edited

Loading

alcuadrado commented Jul 4, 2024

alcuadrado commented Jul 4, 2024

JoshuaKGoldberg commented Jul 6, 2024

alcuadrado commented Jul 8, 2024 •

edited

Loading

JoshuaKGoldberg commented Jul 8, 2024

alcuadrado commented Jul 29, 2024

JoshuaKGoldberg commented Aug 3, 2024 •

edited

Loading

alcuadrado commented Aug 3, 2024

JoshuaKGoldberg Aug 28, 2024

JoshuaKGoldberg Aug 28, 2024

JoshuaKGoldberg Aug 28, 2024

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Oct 8, 2024

JoshuaKGoldberg commented Oct 29, 2024

feat: deprecation message when an async callback is used in a Suite #4921

feat: deprecation message when an async callback is used in a Suite #4921

Conversation

alcuadrado commented Sep 13, 2022

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

linux-foundation-easycla bot commented Sep 13, 2022 • edited Loading

juergba commented Sep 20, 2022 • edited Loading

juergba commented Sep 20, 2022

alcuadrado commented Sep 20, 2022

coveralls commented Sep 27, 2022

juergba left a comment • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jul 2, 2024

alcuadrado commented Jul 3, 2024

alcuadrado commented Jul 4, 2024 • edited Loading

alcuadrado commented Jul 4, 2024

alcuadrado commented Jul 4, 2024

JoshuaKGoldberg commented Jul 6, 2024

alcuadrado commented Jul 8, 2024 • edited Loading

JoshuaKGoldberg commented Jul 8, 2024

alcuadrado commented Jul 29, 2024

JoshuaKGoldberg commented Aug 3, 2024 • edited Loading

alcuadrado commented Aug 3, 2024

JoshuaKGoldberg Aug 28, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 28, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 28, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Oct 8, 2024

JoshuaKGoldberg commented Oct 29, 2024

linux-foundation-easycla bot commented Sep 13, 2022 •

edited

Loading

juergba commented Sep 20, 2022 •

edited

Loading

juergba left a comment •

edited

Loading

alcuadrado commented Jul 4, 2024 •

edited

Loading

alcuadrado commented Jul 8, 2024 •

edited

Loading

JoshuaKGoldberg commented Aug 3, 2024 •

edited

Loading