chore: ban new snapshot assertions in lint#27226
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Snapshot ban silently overridden by later
no-restricted-syntaxoverrides- Duplicated the snapshot-ban selectors into the confirmations and view-test
no-restricted-syntaxoverrides so they are not replaced by later overrides.
- Duplicated the snapshot-ban selectors into the confirmations and view-test
Or push these changes by commenting:
@cursor push 2aad551ba1
Preview (2aad551ba1)
diff --git a/.eslintrc.js b/.eslintrc.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -190,6 +190,29 @@
.join('|')}/]`,
message: 'Avoid using global network selectors in confirmations',
},
+ {
+ selector: "CallExpression[callee.property.name='toMatchSnapshot']",
+ message:
+ 'Snapshot tests are banned. Use explicit behavioral assertions instead.',
+ },
+ {
+ selector:
+ "CallExpression[callee.property.name='toMatchInlineSnapshot']",
+ message:
+ 'Inline snapshot tests are banned. Use explicit behavioral assertions instead.',
+ },
+ {
+ selector:
+ "CallExpression[callee.property.name='toThrowErrorMatchingSnapshot']",
+ message:
+ 'Snapshot-based error assertions are banned. Assert on the error explicitly instead.',
+ },
+ {
+ selector:
+ "CallExpression[callee.property.name='toThrowErrorMatchingInlineSnapshot']",
+ message:
+ 'Inline snapshot-based error assertions are banned. Assert on the error explicitly instead.',
+ },
],
},
},
@@ -227,6 +250,29 @@
message:
'Only Engine and react-native-device-info can be mocked in component-view tests.',
},
+ {
+ selector: "CallExpression[callee.property.name='toMatchSnapshot']",
+ message:
+ 'Snapshot tests are banned. Use explicit behavioral assertions instead.',
+ },
+ {
+ selector:
+ "CallExpression[callee.property.name='toMatchInlineSnapshot']",
+ message:
+ 'Inline snapshot tests are banned. Use explicit behavioral assertions instead.',
+ },
+ {
+ selector:
+ "CallExpression[callee.property.name='toThrowErrorMatchingSnapshot']",
+ message:
+ 'Snapshot-based error assertions are banned. Assert on the error explicitly instead.',
+ },
+ {
+ selector:
+ "CallExpression[callee.property.name='toThrowErrorMatchingInlineSnapshot']",
+ message:
+ 'Inline snapshot-based error assertions are banned. Assert on the error explicitly instead.',
+ },
],
},
},
georgewrmarshall
added
the
team-design-system
georgewrmarshall
force-pushed
the
chore/ban-new-snapshot-lint
branch
from
9e4fb71 to
b2461d4
Compare
|
✅ E2E Fixture Validation — Schema is up to date |
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
There are no changes to:
The new script only analyzes test files during CI and does not affect runtime behavior, build output, app initialization, wallet flows, confirmations, network logic, or any user-facing functionality. Therefore, no E2E test tags are required to validate this PR. Running Detox suites would not provide additional safety for this CI-only change. Risk assessment: Low – Infrastructure/lint-only change with no impact on application runtime or E2E flows. Performance Test Selection: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Snapshot check job not wired into merge gate
- Added new-snapshot-assertions to all-jobs-pass.needs so the merge gate enforces it.
Or push these changes by commenting:
@cursor push 487624b5ac
Preview (487624b5ac)
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -726,6 +726,7 @@
check-workflows,
js-bundle-size-check,
sonar-cloud-quality-gate-status,
+ new-snapshot-assertions,
]
outputs:
ALL_JOBS_PASSED: ${{ steps.jobs-passed-status.outputs.ALL_JOBS_PASSED }}| if: ${{ github.event_name == 'pull_request' && github.base_ref != '' }} | ||
| run: git fetch origin ${{ github.base_ref }} --depth=1 | ||
| - run: yarn lint:snapshots:new | ||
|
|
There was a problem hiding this comment.
Snapshot check job not wired into merge gate
High Severity
The new new-snapshot-assertions job is not listed in the needs array of the all-jobs-pass gate job. Since all-jobs-pass is the sole input to check-all-jobs-pass (the merge gate), a failure in new-snapshot-assertions is silently ignored. PRs that add new snapshot assertions will pass CI and can still be merged, completely defeating the purpose of this PR.
Additional Locations (1)
|
|
This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions. |
github-actions
Bot
added
the
stale
1 participant
Description
This PR adds an ESLint guard that rejects new snapshot assertions in
*.test.*files.The motivation is that mobile currently relies heavily on snapshot testing, and in many cases snapshots appear to be used for coverage rather than to validate the specific behavior described in the test. That creates a lot of churn for broad UI or token updates and makes it harder to review what actually changed.
A good example is the recent grey palette update:
+9,103 -9,103)+37 -37)That delta reflects a real difference in test strategy. Extension does not rely on UI snapshots nearly as much, while mobile absorbs large snapshot diffs for changes that are often unrelated to the behavior the test claims to cover.
Representative examples from mobile:
This PR is intentionally small. It does not remove, deprecate, or migrate the repository's existing snapshot tests. It only prevents new snapshot-based assertions from being added, so we can stop growing the problem while any broader migration is discussed separately.
Scope of enforcement:
toMatchSnapshottoMatchInlineSnapshottoThrowErrorMatchingSnapshottoThrowErrorMatchingInlineSnapshotThe lint rule is also applied in overlapping ESLint overrides, including confirmation tests and view tests, so it is not silently dropped in those paths.
Changelog
CHANGELOG entry: null
Related issues
Fixes:
Manual testing steps
Verified with:
The two
__snapshot-ban-proof__files were temporary local proof files used to verify the overlapping override paths, and were removed afterward.Screenshots/Recordings
Before
Not applicable.
After
Not applicable.
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Low code risk (dev-only tooling), but it changes CI enforcement and can block merges if the base ref detection or snapshot matcher detection behaves unexpectedly.
Overview
Adds a new CI job,
new-snapshot-assertions, that runsyarn lint:snapshots:newon PRs to prevent introducing additional snapshot-based assertions.Introduces
scripts/check-new-snapshot-assertions.jsand thelint:snapshots:newpackage script, which compares changed test files against the PR base revision and fails the build if new snapshot matcher calls (e.g.,toMatchSnapshot, inline snapshot, and snapshot-throw variants) are added.Written by Cursor Bugbot for commit ee04356. This will update automatically on new commits. Configure here.