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
feat: deprecation message when an async callback is used in a Suite #4921
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,7 +145,16 @@ module.exports = function (suites, context, mocha) { | |
throw createUnsupportedError('Pending test forbidden'); | ||
} | ||
if (typeof opts.fn === 'function') { | ||
opts.fn.call(suite); | ||
const suiteResult = opts.fn.call(suite); | ||
if (suiteResult instanceof Promise) { | ||
errors.deprecate( | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'Suite "' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add "[mocha] Suite ..." to the message as done in other parts of our code. The origin of a warning is clearer that way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alcuadrado ping - can you make this change? |
||
suite.fullTitle() + | ||
'" returned a Promise. ' + | ||
'Asynchronous suites are not supported, use a ' + | ||
'synchronous callback instead.' | ||
); | ||
} | ||
suites.shift(); | ||
} else if (typeof opts.fn === 'undefined' && !suite.pending) { | ||
throw createMissingArgumentError( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
'use strict'; | ||
|
||
describe('a suite with an async callback', async function () {}); |
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.
Going back to https://github.com/mochajs/mocha/pull/3550/files#diff-28d67f27fa361db19b8a3aa9181d89c5f0d5ae1089167c55f0341675386e3a28R115, I think we no longer need to prioritize supporting CoffeeScript users. The implicit returns that caused #3550 to be reverted in #3759 are not a good idea for the exact reason we saw here: that they unintentionally return values even when those values shouldn't be returned!
https://typescript-eslint.io/rules/no-confusing-void-expression is the right direction to go IMO.