Conversation
|
@claude please review this PR |
|
@claude review |
| ); | ||
|
|
||
| const startEvent = createMockEvent([{ pageX: 200, pageY: 300 }]); | ||
| const startGesture = createMockGestureState(1); | ||
| instance._handlePanResponderGrant(startEvent, startGesture); | ||
|
|
||
| const moveEvent = createMockEvent([{ pageX: 250, pageY: 350 }]); | ||
| const moveGesture = createMockGestureState(1, 50, 50, 250, 350, 2, 2); | ||
| instance._handlePanResponderMove(moveEvent, moveGesture); | ||
|
|
||
| const releaseEvent = createMockEvent([{ pageX: 250, pageY: 350 }]); | ||
| const releaseGesture = createMockGestureState(1, 50, 50, 250, 350, 2, 2); | ||
| instance._handlePanResponderEnd(releaseEvent, releaseGesture); | ||
|
|
||
| expect(true).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🟡 Several tests use expect(true).toBe(true) as their sole assertion, making them tautological — they always pass regardless of code behavior. Most critically, panMomentumEnabled triggers decay animation on release (advanced.test.tsx:569) and cleans up intervals on unmount (rendering.test.tsx:244) never verify the behavior they claim to test; two fallback paths in callbacks.test.tsx (lines 515, 614) also silently pass when the primary mock path is unavailable. Replace these with real assertions such as expect(animations.getPanMomentumDecayAnim).toHaveBeenCalled() or verifying that the interval is cleared.
Extended reasoning...
What the bug is and how it manifests
Four locations in the new test suite use expect(true).toBe(true) as their only or final assertion. Because this expression is a tautology, the containing test() block can never fail. The tests execute real production code paths — increasing line and branch coverage metrics — but provide zero behavioral guarantee: even if the feature is completely removed or broken, the test still passes green.
The specific code paths affected
-
advanced.test.tsx line 569 —
panMomentumEnabled triggers decay animation on release: The test performs a full gesture sequence (grant → move → release) with panMomentumEnabled: true and a custom panMomentumDecayFactor. After the release, instead of asserting that the decay animation was invoked, it falls back to expect(true).toBe(true). The getPanMomentumDecayAnim mock is already set up in the file and a .toHaveBeenCalled() assertion would be trivial to add. -
rendering.test.tsx line 244 —
cleans up intervals on unmount: The test calls instance.componentDidMount(), confirms instance.measureZoomSubjectInterval is defined, then calls instance.componentWillUnmount(). The only assertion after unmount is expect(true).toBe(true) — nothing checks that the interval was actually cleared. -
callbacks.test.tsx line 515 —
onShouldBlockNativeResponder defaults to true(else branch): When RN.PanResponder.create.mock?.calls is empty, the test skips all real assertions and executes only expect(true).toBe(true), meaning the entire test can pass without the mock even existing. -
callbacks.test.tsx line 614 —
onShouldBlockNativeResponder returns true when prop not provided(else branch): Same pattern as Fix incorrect original measurements when initial zoom level and offsets are present #3.
Why existing code does not prevent it
Jest has no built-in mechanism to require that at least one meaningful assertion was made. Without expect.hasAssertions() or a real predicate, a test body that only calls expect(true).toBe(true) is indistinguishable from a well-written test from Jest's perspective.
Impact
The primary harm is false confidence. The PR description justifies these tests as comprehensive coverage; the coverage report will show lines and branches covered for momentum and unmount paths, but a developer who accidentally deletes the momentum animation call or breaks the interval cleanup will see no test failures. This defeats the stated purpose of the PR.
Step-by-step proof for case #1
- The production component calls getPanMomentumDecayAnim(...) inside _handlePanResponderEnd when panMomentumEnabled is true.
- Imagine a regression removes that call entirely.
- The test still sets up the component with panMomentumEnabled: true, still calls _handlePanResponderGrant, _handlePanResponderMove, and _handlePanResponderEnd — none of which throw.
- The test reaches expect(true).toBe(true) and passes.
- CI is green. The regression ships undetected.
How to fix
- Case 1: Replace the tautological assertion with expect(animations.getPanMomentumDecayAnim).toHaveBeenCalled() — the mock is already imported and configured in the file.
- Case 2: Assert expect(instance.measureZoomSubjectInterval).toBeNull() or spy on clearInterval and assert it was called after componentWillUnmount().
- Cases 3 and 4: Add expect.hasAssertions() at the top of each test, or restructure so the primary if-branch always executes (e.g., ensure the PanResponder mock is set up before mountComponent is called rather than relying on a conditional fallback).
Note
Adds extensive Jest tests for ReactNativeZoomableView, configures Jest setup/coverage, and updates CI to Node 20 with Codecov uploads.
ReactNativeZoomableViewcovering gestures, callbacks, rendering, methods, integration, and advanced cases undersrc/__tests__/with shared helpers.collectCoverageFromand globalcoverageThresholdinpackage.json.jest.setup.jsto mock RN modules and animation frame APIs.setupFilesAfterEnv,testMatch, coverage collection, and ignore patterns; add ESLint overrides for test files.CIrunning on push/PR tomaster; use Node 20, cache yarn, run TypeScript, ESLint, Jest with coverage, and upload reports viacodecov-action@v3.Written by Cursor Bugbot for commit 3591335. Configure here.