chore(eslint): add mocha/no-async-suite rule and fix violations#6498
chore(eslint): add mocha/no-async-suite rule and fix violations#64980xAxiom wants to merge 1 commit intoOpenZeppelin:masterfrom
Conversation
Closes OpenZeppelin#4943 Adds `eslint-plugin-mocha` and enables the `mocha/no-async-suite` rule (formerly `no-async-describe` in older versions of the plugin). This catches `describe('...', async function () { ... })` patterns where the `async` keyword has no effect and silently swallows top-level errors. Removes the unused `async` keyword from three existing call sites: - test/token/ERC6909/ERC6909.behavior.js - test/access/manager/AccessManager.test.js - test/crosschain/BridgeERC1155.behavior.js None of the three describe bodies awaited at the top level, so removing `async` is a no-op for runtime behavior.
🦋 Changeset detectedLatest commit: ba1d4e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughThis pull request introduces ESLint tooling to catch unintended async usage on Mocha describe blocks. It adds eslint-plugin-mocha as a development dependency, configures the mocha/no-async-suite rule in the ESLint configuration, and documents the change in a changeset. Additionally, it updates three existing test files that previously used async describe blocks, converting them to regular functions to comply with the new rule. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eslint.config.mjs (1)
14-31: 💤 Low valueConsider scoping the Mocha plugin config to test files only.
The
mochaplugin andno-async-suiterule are applied to every file in the project. While this causes no false positives today (non-test files don't usedescribe), best practice for ESLint flat config is to scope Mocha rules to the test directory.♻️ Suggested scope restriction
export default [ js.configs.recommended, prettier, { - plugins: { mocha }, languageOptions: { ecmaVersion: 2022, globals: { ...globals.browser, ...globals.mocha, ...globals.node, artifacts: 'readonly', contract: 'readonly', web3: 'readonly', extendEnvironment: 'readonly', expect: 'readonly', }, }, - rules: { - 'mocha/no-async-suite': 'error', - }, }, + { + files: ['test/**/*.js'], + plugins: { mocha }, + rules: { + 'mocha/no-async-suite': 'error', + }, + }, includeIgnoreFile(path.resolve(__dirname, '.gitignore')), ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.mjs` around lines 14 - 31, The Mocha plugin and rule (plugins: { mocha } and rules: { 'mocha/no-async-suite' }) are currently applied globally; restrict them to test files by moving the mocha plugin and the 'mocha/no-async-suite' rule into an ESLint overrides entry that targets your test file globs (e.g., tests/**/*.js, **/*.spec.js, or similar), so only files matching those patterns get the Mocha globals and rule; update the top-level config to remove the global mocha plugin/rule and add an overrides object containing the same languageOptions.globals additions and the mocha plugin + rule for the test file patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@eslint.config.mjs`:
- Around line 14-31: The Mocha plugin and rule (plugins: { mocha } and rules: {
'mocha/no-async-suite' }) are currently applied globally; restrict them to test
files by moving the mocha plugin and the 'mocha/no-async-suite' rule into an
ESLint overrides entry that targets your test file globs (e.g., tests/**/*.js,
**/*.spec.js, or similar), so only files matching those patterns get the Mocha
globals and rule; update the top-level config to remove the global mocha
plugin/rule and add an overrides object containing the same
languageOptions.globals additions and the mocha plugin + rule for the test file
patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5df552d5-36e0-4d74-80fc-3be1a9196df7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.changeset/eslint-no-async-suite.mdeslint.config.mjspackage.jsontest/access/manager/AccessManager.test.jstest/crosschain/BridgeERC1155.behavior.jstest/token/ERC6909/ERC6909.behavior.js
|
Closing as duplicated: #6438 |
2 participants
Closes #4943.
What
eslint-plugin-mocha(v11) as a dev dependency.mocha/no-async-suiterule ineslint.config.mjs— this is the v11 successor tono-async-describereferenced in the issue. It flagsdescribe('...', async function () {...})where theasynckeyword has no effect and silently absorbs top-level rejections.asyncfrom three pre-existing violators:test/token/ERC6909/ERC6909.behavior.js(line 145)test/access/manager/AccessManager.test.js(line 1111)test/crosschain/BridgeERC1155.behavior.js(line 30)In each case the describe body has no top-level
await, so removingasyncis a no-op at runtime.Why
Per #4943, an unintended
asyncon adescribeblock can mask test failures by swallowing the returned promise. The plugin rule prevents future regressions cheaply.The issue also noted catching missing
awaits would require a TypeScript migration; this PR does not attempt that broader change.Verification
npm run lint:jspasses.async describeinto a temp file intest/and confirmed the rule errors withmocha/no-async-suite.npx hardhat test test/access/manager/AccessManager.test.js→ 489 passing.npx hardhat test test/token/ERC6909/ERC6909.test.js test/crosschain/BridgeERC1155.test.js→ 47 passing.Changeset
Added a no-bump changeset (tooling-only, no public API or contract changes).