Mitigate No-Conditional-Expect ESLint Rule#423
Mitigate No-Conditional-Expect ESLint Rule#423astarinmymind wants to merge 13 commits intodevelopfrom
Conversation
Gudahtt
left a comment
There was a problem hiding this comment.
Due to typescript constraints, the object could possibly be undefined. So expect checks are placed inside an if block.
I encountered this recently in #387, and used the non-null assertion to get past the type error. I had to add an ignore comment to use it as well. Not ideal, but, This was a situation that TypeScript was ill-equipped to understand apparently. My attempts to use type guards to teach it that it wasn't undefined failed for reasons I don't understand.
But, a similar approach is an option here too - we could use ignore comments for these cases, or use a non-null assertion and use an ignore comment for that too.
Some of the rules we have in @metamask/eslint-config are meant more as sane defaults than absolute rules. The idea is that future authors at least try to avoid a conditional expect if they can.
src/util.test.ts
Outdated
| expect(e.message).toContain('timeout'); | ||
| error = e; | ||
| } | ||
| expect(error).toBeUndefined(); |
There was a problem hiding this comment.
This doesn't seem right 🤔 The test was certainly broken before, but it remains broken now. The test passes and continues onto the next test before the timeout resolves.
So this expect call makes sense - it's asserting that the error doesn't appear right away. But there needs to be a second expect to ensure the error is thrown eventually. Also this test block should be async to allow it to wait for the timeout to resolve.
There was a problem hiding this comment.
Agreed, like I mentioned in #425 I think it is best if I fix this in a separate PR.
| return new Promise((res) => setTimeout(res, 800)); | ||
| }); | ||
| } catch (e) { | ||
| expect(e.message).toContain('timeout'); |
There was a problem hiding this comment.
😂 wow. Let me guess - this line was never reached in the original test?
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable jest/no-conditional-expect */ | |||
There was a problem hiding this comment.
Could we leave this rule enabled, but use a type guard to keep the undefined check around?
e.g. instead of
expect(message).not.toBeUndefined();
if (message) {
expect(message.status).toBe('rejected');
}
We could do this:
if (!message) {
throw new Error('Message undefined')
}
expect(message.status).toBe('rejected');
We are allowed to throw conditionally, and TypeScript is supposed to be smart enough to infer that after a check like that, the type can't be undefined anymore.
Leaving this rule disabled for entire modules seems less than ideal, because it is really easy to write tests that are completely broken if some expectations are conditional.
|
With the rebases required, I think it'll be cleaner to start this one over from scratch. |
… core monorepo (#420)" (#423) This reverts commit d10c455. This breaking change is blocking a release fix that will include: 1. MetaMask/eth-json-rpc-middleware#421 2. MetaMask/eth-json-rpc-middleware#422 Will readd this commit after those two PRs are merged and a patch release is cut <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues or other links reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 -->
Update dependencies that Socket warned about in #423 <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues or other links reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Updates yarn.lock to bump several packages (e.g., cross-spawn 7.0.6, form-data 3.0.4, get-intrinsic 1.3.1) and add required transitive deps to address audits. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit a6eefaab549ac8b5980443e43b5c3a3b847682ab. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
…d core monorepo (#420)" (#423) (#426) This reverts commit d8129a6 (Reapplies d10c455) <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues or other links reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Overhauls the dev toolchain: upgrades ESLint to v9 with new configs/plugins, moves to TypeScript 5.x, adds TypeDoc/depcheck/ts-bridge/ATTW tooling, updates Prettier to v3, and refreshes dependencies. > > - **Dev toolchain sync/upgrade** > - **ESLint 9**: Bump core and configs (MetaMask v14), migrate plugins/resolvers (e.g., `eslint-plugin-import-x`, `eslint-import-resolver-typescript`), and update related utilities. > - **TypeScript 5.x**: Update TypeScript, `typescript-eslint` (v8), `ts-node`, and supporting packages. > - **Formatting & docs**: Upgrade **Prettier** to v3 and `prettier-plugin-packagejson`; add **TypeDoc**. > - **New tooling**: Add `@arethetypeswrong/cli`, `@ts-bridge/cli`, and **depcheck** for dependency hygiene. > - **Dependencies**: Broad dependency updates and lockfile refresh to align with core template. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5ef56a47b1975d8ded4b528f0e5bd76585e4bac1. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This PR attempts to remove this eslint rule:
This rule is violated in two of the following ways: placing expects in a catch or if block.
I fixed all the cases that fall under type 1, but can't think of a way to alter type 2.
In the second commit 41e711d , the test was flawed so I fixed that as well.
I propose we leave this rule, or if there are ideas for a way to get around the typescript complaint, I would be happy to implement it.