feat: add no-unnecessary-assertions rule to disallow assertions that can never fail - #400
Open
voidmatcha wants to merge 2 commits into
Open
feat: add no-unnecessary-assertions rule to disallow assertions that can never fail#400voidmatcha wants to merge 2 commits into
voidmatcha wants to merge 2 commits into
Conversation
|
|
Collaborator
|
voidmatcha
added a commit
to voidmatcha/e2e-skills
that referenced
this pull request
Jul 27, 2026
…4f is official eslint-plugin-playwright v2.11.0 shipped `no-unnecessary-assertions`, the rule this project contributed upstream in mskelton/eslint-plugin-playwright#470, and enables it through that plugin's `recommended` config. Tier 1 therefore catches #4f on any `eslint-plugin-playwright@>=2.11.0` with no extra package. Verified end to end: the scanner surfaces the rule's own diagnostic and Tier 2 cedes `sg-4f` to Tier 1 as designed. So `eslint-plugin-playwright-silent-pass` is deprecated on npm and archived, and scan.sh no longer downloads or resolves it for Playwright — keeping it would pull a deprecated package and double-report the same finding. The Cypress companion is untouched: `eslint-plugin-cypress` has no equivalent rule yet, so eslint-plugin-cypress-silent-pass stays the Tier-1 path there. The same rule is now proposed upstream for Cypress in cypress-io/eslint-plugin-cypress#400. Docs follow the release: SKILL.md and README x4 say the rule shipped in v2.11.0 rather than "ships in the next release", and the deprecated package's npm badge is dropped from all four READMEs. The Cypress badge stays. Two follow-ups from reviewing how the tiers treat a project's own ESLint setup. Tier 1 previously ignored that setup entirely: the scanner passed its generated config with -c, which makes ESLint skip the project's config, so a team that had deliberately set a rule to 'off' still saw it reported and the only escape hatch dropped Tier 1 wholesale. A project's flat config is now appended after the baseline — flat config is an array and later entries win, so the disable takes effect. Severity edits are still ignored (severity here is P0/P1, ours to assign), and legacy .eslintrc cannot be imported from an ESM config so it keeps the old behavior, which the scanner now says outright. Tiers 2 and 3 are unchanged and keep reporting regardless: they ask whether a test can fail, not whether a lint policy permits it, which is what keeps the count reproducible across hosts. The closing summary now splits the pattern IDs that fired into ones a lint rule could enforce — naming the rule, and flagging when it sits outside `recommended` — and ones no ESLint rule expresses. The mapping was checked against the published rule lists rather than assumed: #5a, #5b, #6, #8a, #9c and #17 all have upstream rules and were miscounted as review-only in the first cut, which would have told users to keep reviewing something they could enforce once. Roughly half the catalog has no ESLint equivalent at all, which is the point the split makes visible. README x4 and SKILL.md document which tier follows whose rules. Constraint: the Cypress side cannot be retired until #400 lands; until then two different paths cover #4f per framework, and the docs must not imply otherwise. Rejected: removing the Cypress companion at the same time (its upstream rule is unmerged, so #4f would lose Tier-1 coverage for Cypress). Confidence: high — the Playwright path was verified against the published 2.11.0 package, not just the release notes. Scope-risk: scan.sh no longer installs the companion for Playwright, so a project pinned below eslint-plugin-playwright 2.11.0 loses the Tier-1 #4f signal. Tier 2 and Tier 3 still cover it, and ci-local is green. Not-tested: behavior against an eslint-plugin-playwright older than 2.11.0 (the rule simply is not in its recommended config, so it is not enabled rather than erroring).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add
no-unnecessary-assertionsrule to disallow chai assertions on a Cypress chainable that can never fail.cy.get()returns a chainable — an object that queues the command. It is not the element, and it exists whether or not the selector ever matches. Soexpect(cy.get('.badge')).to.existpasses on a blank page, and passes when.badgewas renamed three refactors ago.The equivalent rule for Playwright's
Locatorwas recently contributed toeslint-plugin-playwright(mskelton/eslint-plugin-playwright#470, shipped in v2.11.0).Changes:
lib/rules/no-unnecessary-assertions.js— new rule; matchesexpect()whose subject is an inlinecy.<query>()chain and whose chai terminal is satisfied by any object (.to.exist,.to.be.ok,.to.not.be.null,.to.not.be.undefined,.to.be.an('object'))tests/lib/rules/no-unnecessary-assertions.js— 25 tests covering valid.should()usage, values yielded into.then(($el) => …), non-Cypress subjects, assertions that can genuinely fail, and the autofix boundariesdocs/rules/no-unnecessary-assertions.md— rule documentationlib/index.js— rule registered in plugin indexMotivation
This shape turns up in suites written with unit-test habits, where
expect(x).to.existis the reflex:It reads like a check and counts as coverage, but it asserts against the chainable object rather than the page, so it stays green on a blank page.
What makes it worse than an ordinary broken selector is that it never goes red. A rotted positive assertion fails on the next run and gets fixed; this one is indistinguishable from a passing test, so a suite accumulates coverage it does not have.
The fix rewrites to
.should('exist')— the same claim the original made, so a hidden-but-present element does not start failing after--fix. It applies only when the chain's last command is a query and the assertion is a standalone statement. Chains ending in.its()/.invoke()are reported but left alone, since an element assertion on a string or number would fail at runtime.On
recommended: I left the rule opt-in (recommended: false) rather than adding it to the recommended config. Since releases go out through semantic-release, adding an error-level rule torecommendedwould fail existing consumers' CI on a minor upgrade — that seemed like your call rather than mine. Happy to promote it here if you'd prefer.