feat: Automated test suite — unit, component#863
Open
tuliomir wants to merge 20 commits into
Open
Conversation
Establishes a multi-layered test infrastructure for the wallet creation workflow as a proof of concept for the full test suite. Layer 1 (Unit): utils, reducer wallet lifecycle — 38 tests Layer 2 (Integration): wallet start/reset sagas — 3 tests Layer 3 (Component): InitWallet, BackupWords screens — 16 tests New dependencies: @testing-library/react-native, redux-saga-test-plan Fixes stale mocks in jestMockSetup.js (unleash, removed modules) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix historyNanoContract tests to match current saga flow (locking mechanism, new step ordering). Add clearLoadingLocksForTesting export for test isolation. - Fix registerNanoContract tests for getBlueprintId/getBlueprintInformation saga steps - Fix App.test.tsx redux mock (legacy_createStore) - Add mocks: react-native-camera-kit, @walletconnect/react-native-compat, @react-native-firebase/app, wallet-lib sub-path imports (axiosWrapper) - Add testPathIgnorePatterns for helpers/, fixtures, and e2e/ - Result: 9 suites passing, 83 tests, 0 failures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jest-circus@^30 transitively pulls unrs-resolver, which has an install script. LavaMoat allow-scripts requires every such package to be explicitly enumerated; default it to disallowed (false) like the other entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
reown.test.js (added on master by 8db34bb) drove its generators with the wrong gen.next() arity — it forgot that gen.next(arg) returns the NEXT yield, not the previous one. The 3 broken tests effectively read yields off-by-one and missed the saga's getPendingSessionRequests re-fetch after put(reownReject). Pre-existing master bug exposed once this branch ran the test for the first time. Fixes: - rejectSinglePendingRequest "rejects request and refreshes list": capture respondSessionRequest from gen.next(mockReownClient) (one fewer gen.next call). - rejectAllPendingRequests with 3 pending: gen.next(pending) already yields respond #1, so only 2 more bare gen.next calls cover #2/#3; added the missing re-fetch step and feed [] to it. - rejectAllPendingRequests with empty pending: gen.next([]) directly yields put(reownReject); added the missing re-fetch step. Renamed "clears list" → "resyncs list" to reflect the saga's actual contract (it re-reads pending after rejection, not blindly setting []). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds extensive test infrastructure and many new tests: helper utilities for store/navigation/rendering/initial state, expanded Jest mocks and config, numerous reducer/saga/screen/utils tests, a test-only saga utility export, and contributor-facing testing documentation and guidance files. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 5
🧹 Nitpick comments (2)
__tests__/screens/BackupWords.test.tsx (2)
162-185: Early return silently passes test without assertions.If
wrongWord === correctWord, the test returns early at line 174 without any assertions. While this won't happen with the current test data ('abandon' !== 'able'), this pattern is fragile. Consider usingexpect.assertions(n)or throwing an error instead of silently passing.♻️ Suggested improvement
it('shows failure modal when wrong word is selected', async () => { const { getByText, queryByText } = render( <BackupWords navigation={mockNavigation} route={mockRoute} /> ); // Select a WRONG word (not the one at position FIXED_INDEXES[0]) const correctWord = TEST_WORDS_ARR[FIXED_INDEXES[0] - 1]; // Find any option that is NOT the correct word // Options are around index (FIXED_INDEXES[0]-1), which is 2. Options: indexes 0-4 const wrongWord = TEST_WORDS_ARR[0]; // 'abandon' — not 'able' - if (wrongWord === correctWord) { - // Fallback if they happen to be the same - return; - } + // Ensure we actually have a wrong word to test with + expect(wrongWord).not.toBe(correctWord); await act(async () => { fireEvent.press(getByText(wrongWord)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/screens/BackupWords.test.tsx` around lines 162 - 185, The test silently returns if wrongWord === correctWord which can mask failures; instead ensure the test always asserts by replacing the early return with a guard that fails the test (e.g., throw new Error(...) or use expect(wrongWord).not.toEqual(correctWord())) or declare expect.assertions(1) and then proceed; update the logic around wrongWord, correctWord, TEST_WORDS_ARR and FIXED_INDEXES in the 'shows failure modal when wrong word is selected' test to either pick a guaranteed different wrongWord or explicitly fail when they match so the test always performs the modal assertion.
206-232: Test lacks assertions for dismiss behavior.This test verifies the failure modal appears but doesn't actually test the
onDismiss → goBackbehavior described in the comments. Consider either completing the test by simulating the dismiss and assertingnavigation.goBackwas called, or renaming the test to accurately reflect what it tests (e.g.,'shows failure modal for wrong selection').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/screens/BackupWords.test.tsx` around lines 206 - 232, The test "failure modal dismiss navigates back" currently only asserts the failure modal appears; update it to actually simulate the dismiss and assert navigation.goBack was called (or rename the test to reflect it only shows the modal). After the waitFor that verifies queryByText('Wrong word.') is truthy, trigger the FeedbackModal/BackdropModal dismiss (e.g., fireEvent.press on the backdrop element found by testID like 'backdrop' or the modal's dismiss button, or call the FeedbackModal's onDismiss handler via the rendered tree) and then expect(mockNavigation.goBack).toHaveBeenCalled(); if you cannot simulate dismiss reliably, rename the it(...) description to 'shows failure modal for wrong selection' to match the current assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/helpers/mockNavigation.ts`:
- Around line 46-67: The helper setupNavigationMocks currently calls jest.mock()
at runtime inside the function (wrapping the mocks for
'@react-navigation/native' and '../../src/hooks/navigation'), but jest.mock
calls are hoisted and must live at module scope; move the mocking out of
setupNavigationMocks: export createMockNavigation and createMockRoute (or export
mockNav and mockRoute factories) and perform jest.mock(...) at the top level in
tests (or at the top of this module if you want shared defaults) so
useNavigation/useRoute/useParams are mocked before modules are imported;
alternatively, change the helper to only return mock objects and update tests to
call jest.mock(...) at module scope using those mocks (refer to
setupNavigationMocks, createMockNavigation, createMockRoute, and the mocked
hooks useNavigation/useRoute/useParams).
In `@__tests__/sagas/nanoContracts/historyNanoContract.test.js`:
- Around line 91-95: The test fails because it compares two distinct Error
instances with toStrictEqual; capture the Error instance produced earlier and
reuse it in the expectation (or assert on its shape instead of instance
identity). Locate the generator advance that yields the error (the variable used
when creating the error at line 86) and use that same error variable in the
expect against put(onExceptionCaptured(...)) or change the assertion to match
error.message/type (e.g.,
expect(put(onExceptionCaptured(expect.objectContaining({message: 'network
error'})), false)) so the comparison does not rely on Error reference equality.
In `@__tests__/sagas/wallet.test.ts`:
- Around line 109-110: The test is asserting against the outer mockWallet object
(mockWallet.stop) which is unrelated to the saga run with state.wallet = null,
making the assertion vacuously true; instead assert that the saga did not yield
a call effect to wallet.stop by using expectSaga's effect matchers (e.g.,
replace expect(mockWallet.stop).not.toHaveBeenCalled() with
expectSaga(...).not.call.fn(mockWallet.stop) or use .not.call()/.not.call.fn()
to ensure no call effect to wallet.stop was yielded), targeting the wallet.stop
function and the expectSaga invocation that runs the saga under test.
In `@jestMockSetup.js`:
- Around line 61-72: The mock currently only provides a default callable but
tests import named exports; update the mock to export named symbols alongside
the default factory: export getMessaging (or a getMessaging factory returning
the same object), and named functions/values getToken, deleteToken,
hasPermission, requestPermission, getInitialNotification, onMessage,
onNotificationOpenedApp, setBackgroundMessageHandler,
isDeviceRegisteredForRemoteMessages, registerDeviceForRemoteMessages, and
AuthorizationStatus so imports in src/sagas/pushNotification.js and
src/workers/backgroundListeners.js resolve; keep existing jest.fn()
implementations and Promise.resolve behaviors and ensure the default export
still returns the same object.
In `@package.json`:
- Around line 120-121: The package.json lists "jest": "29.7.0" but
"jest-circus": "^30.3.0", causing a major-version mismatch; update the
jest-circus dependency to match Jest's major/minor (for example change
"jest-circus" to "29.7.0" or "^29.7.0") so both jest and jest-circus use the
same 29.x release and avoid compatibility issues.
---
Nitpick comments:
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 162-185: The test silently returns if wrongWord === correctWord
which can mask failures; instead ensure the test always asserts by replacing the
early return with a guard that fails the test (e.g., throw new Error(...) or use
expect(wrongWord).not.toEqual(correctWord())) or declare expect.assertions(1)
and then proceed; update the logic around wrongWord, correctWord, TEST_WORDS_ARR
and FIXED_INDEXES in the 'shows failure modal when wrong word is selected' test
to either pick a guaranteed different wrongWord or explicitly fail when they
match so the test always performs the modal assertion.
- Around line 206-232: The test "failure modal dismiss navigates back" currently
only asserts the failure modal appears; update it to actually simulate the
dismiss and assert navigation.goBack was called (or rename the test to reflect
it only shows the modal). After the waitFor that verifies queryByText('Wrong
word.') is truthy, trigger the FeedbackModal/BackdropModal dismiss (e.g.,
fireEvent.press on the backdrop element found by testID like 'backdrop' or the
modal's dismiss button, or call the FeedbackModal's onDismiss handler via the
rendered tree) and then expect(mockNavigation.goBack).toHaveBeenCalled(); if you
cannot simulate dismiss reliably, rename the it(...) description to 'shows
failure modal for wrong selection' to match the current assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6c0652c-a995-493f-b1e2-6fc492754973
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
__tests__/App.test.tsx__tests__/helpers/mockNavigation.ts__tests__/helpers/mockStore.ts__tests__/helpers/renderWithProviders.tsx__tests__/reducers/reducer.wallet.test.ts__tests__/sagas/nanoContracts/historyNanoContract.test.js__tests__/sagas/nanoContracts/registerNanoContract.test.js__tests__/sagas/reown.test.js__tests__/sagas/wallet.test.ts__tests__/screens/BackupWords.test.tsx__tests__/screens/InitWallet.test.tsx__tests__/utils.test.tsdocs/testing-ci-plan.mdjestMockSetup.jspackage.jsonsrc/sagas/nanoContract.js
Comment on lines
+61
to
+72
| jest.mock('@react-native-firebase/messaging', () => () => ({ | ||
| getToken: jest.fn(() => Promise.resolve('mock-token')), | ||
| deleteToken: jest.fn(() => Promise.resolve()), | ||
| requestPermission: jest.fn(() => Promise.resolve(1)), | ||
| getInitialNotification: jest.fn(() => Promise.resolve(null)), | ||
| onMessage: jest.fn(() => jest.fn()), | ||
| onNotificationOpenedApp: jest.fn(() => jest.fn()), | ||
| setBackgroundMessageHandler: jest.fn(), | ||
| isDeviceRegisteredForRemoteMessages: true, | ||
| registerDeviceForRemoteMessages: jest.fn(() => Promise.resolve()), | ||
| AuthorizationStatus: { AUTHORIZED: 1, NOT_DETERMINED: -1, DENIED: 0, PROVISIONAL: 2 }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Imports from `@react-native-firebase/messaging` =="
rg -n "from '@react-native-firebase/messaging'" -C2
echo
echo "== Named symbols referenced in app/tests =="
rg -nP "\b(getMessaging|getToken|deleteToken|hasPermission|requestPermission|onMessage|registerDeviceForRemoteMessages|AuthorizationStatus)\b" src __tests__Repository: HathorNetwork/hathor-wallet-mobile
Length of output: 2922
Mock must export named symbols used by Firebase Messaging consumers.
The mock currently exposes only a default callable factory, but src/sagas/pushNotification.js and src/workers/backgroundListeners.js import named exports like getMessaging, getToken, deleteToken, hasPermission, requestPermission, onMessage, registerDeviceForRemoteMessages, setBackgroundMessageHandler, and AuthorizationStatus. These will be undefined in tests.
✅ Suggested fix (export named API alongside default callable)
-jest.mock('@react-native-firebase/messaging', () => () => ({
- getToken: jest.fn(() => Promise.resolve('mock-token')),
- deleteToken: jest.fn(() => Promise.resolve()),
- requestPermission: jest.fn(() => Promise.resolve(1)),
- getInitialNotification: jest.fn(() => Promise.resolve(null)),
- onMessage: jest.fn(() => jest.fn()),
- onNotificationOpenedApp: jest.fn(() => jest.fn()),
- setBackgroundMessageHandler: jest.fn(),
- isDeviceRegisteredForRemoteMessages: true,
- registerDeviceForRemoteMessages: jest.fn(() => Promise.resolve()),
- AuthorizationStatus: { AUTHORIZED: 1, NOT_DETERMINED: -1, DENIED: 0, PROVISIONAL: 2 },
-}));
+jest.mock('@react-native-firebase/messaging', () => {
+ const AuthorizationStatus = {
+ AUTHORIZED: 1,
+ NOT_DETERMINED: -1,
+ DENIED: 0,
+ PROVISIONAL: 2,
+ EPHEMERAL: 3,
+ };
+
+ const instance = {
+ getToken: jest.fn(() => Promise.resolve('mock-token')),
+ deleteToken: jest.fn(() => Promise.resolve()),
+ requestPermission: jest.fn(() => Promise.resolve(AuthorizationStatus.AUTHORIZED)),
+ getInitialNotification: jest.fn(() => Promise.resolve(null)),
+ onMessage: jest.fn(() => jest.fn()),
+ onNotificationOpenedApp: jest.fn(() => jest.fn()),
+ setBackgroundMessageHandler: jest.fn(),
+ isDeviceRegisteredForRemoteMessages: true,
+ registerDeviceForRemoteMessages: jest.fn(() => Promise.resolve()),
+ };
+
+ return {
+ __esModule: true,
+ default: jest.fn(() => instance),
+ getMessaging: jest.fn(() => ({})),
+ getToken: instance.getToken,
+ deleteToken: instance.deleteToken,
+ hasPermission: jest.fn(() => Promise.resolve(AuthorizationStatus.AUTHORIZED)),
+ requestPermission: instance.requestPermission,
+ onMessage: instance.onMessage,
+ registerDeviceForRemoteMessages: instance.registerDeviceForRemoteMessages,
+ setBackgroundMessageHandler: instance.setBackgroundMessageHandler,
+ AuthorizationStatus,
+ };
+});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jestMockSetup.js` around lines 61 - 72, The mock currently only provides a
default callable but tests import named exports; update the mock to export named
symbols alongside the default factory: export getMessaging (or a getMessaging
factory returning the same object), and named functions/values getToken,
deleteToken, hasPermission, requestPermission, getInitialNotification,
onMessage, onNotificationOpenedApp, setBackgroundMessageHandler,
isDeviceRegisteredForRemoteMessages, registerDeviceForRemoteMessages, and
AuthorizationStatus so imports in src/sagas/pushNotification.js and
src/workers/backgroundListeners.js resolve; keep existing jest.fn()
implementations and Promise.resolve behaviors and ensure the default export
still returns the same object.
Pin behavior, state-shape, and action-type contracts for the in-scope reducer surface so the upcoming RTK-slices refactor surfaces drift in explicit, reviewable test diffs rather than silent regressions. - reducer.wallet.test.ts: add START_WALLET_NOT_STARTED case, initial state shape contract, action-type contract. - reducer.reown.test.ts (new): same three-layer pattern for the REOWN_SET_PENDING_REQUESTS handler added on this branch. - testing-guide.md: rewrite docs/testing-ci-plan.md as a contributor guide covering the four-layer pyramid, PR checklist, design principles, and the slice safety-net pattern. Old CI content kept as an appendix. Reducer tests: 14 → 38. Full suite: 116 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- package.json: pin jest-circus to 29.7.0 (was ^30.3.0). Aligns with jest 29.7.0 to avoid major-version drift, and matches the repo-wide rule of using exact versions to reduce supply-chain attack surface. - package.json: drop the now-stale lavamoat allowScripts entry for jest-circus>jest-runtime>jest-resolve>unrs-resolver. That transitive dep only existed under jest-circus 30.x and is gone after the pin. - package-lock.json: regenerated by npm install; jest-circus subtree resolves to 29.7.0 with no caret/tilde ranges. - __tests__/helpers/mockNavigation.ts: remove the broken setupNavigationMocks helper (zero callers, and jest.mock inside a function body is not hoisted by Babel so it silently no-ops anyway). Keep createMockNavigation/createMockRoute factories which are correct and reusable. - __tests__/screens/BackupWords.test.tsx: replace silent `return` guard with an explicit assertion that wrongWord !== correctWord, so a fixture change can never silently mask the test. Rename the 'failure modal dismiss navigates back' test to 'shows failure modal for wrong selection' since it only verifies modal appearance; the dismiss → goBack assertion is left as a follow-up. Other review findings checked but unchanged: - historyNanoContract.test.js Error compare: false positive; toStrictEqual structurally compares Error since Jest 24. - wallet.test.ts mockWallet.stop assertion: defensible — beforeEach jest.clearAllMocks() makes it a real isolation check, and the reviewer's expectSaga.not.call.fn alternative has the same target weakness. - jestMockSetup.js Firebase named exports: latent but unexercised by any current test path; no fix needed today. Full suite: 116 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add inline comments at three sites flagged in review so future reviewers (and other AI agents) see the rationale up front and don't re-raise the same complaint: - historyNanoContract.test.js: toStrictEqual structurally compares Error since Jest 24, so two `new Error(...)` instances match. - wallet.test.ts: jest.clearAllMocks() in beforeEach makes the mockWallet.stop assertion a real cross-test isolation check; the expectSaga.not.call.fn alternative has the same target weakness. - jestMockSetup.js: the Firebase messaging mock returns a default callable; production code imports named exports that resolve to undefined, but no current test path invokes them. Comment lists the named exports that must be added if/when a test exercises pushNotification.js or backgroundListeners.js. No behavior changes; only comments. Suite still 116 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
1-1:⚠️ Potential issue | 🔴 CriticalPipeline failure: package-lock.json is out of sync.
The CI pipeline reports:
npm ci can only install when package.json and package-lock.json are in sync; lock file does not include typescript@6.0.3This indicates the lockfile references a dependency version not declared in
package.json. Thepackage.jsonshowstypescript@4.8.4, but the error mentions6.0.3.Please regenerate the lockfile:
rm package-lock.json npm installThen commit the updated
package-lock.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 1, CI fails because package-lock.json is out of sync with package.json (lock references typescript@6.0.3 while package.json lists typescript@4.8.4); fix by regenerating the lockfile: remove the current package-lock.json, run npm install (or npm ci after ensuring package.json is correct) to produce a lockfile matching package.json, then commit the updated package-lock.json so npm ci succeeds; ensure you run this locally and push the regenerated package-lock.json.jestMockSetup.js (1)
95-121:⚠️ Potential issue | 🟠 MajorFix Unleash mock exports to match actual imports.
The mock exports
HathorUnleashClientas a named export, but the code imports a default export:import UnleashClient, { FetchTogglesStatus } from '@hathor/unleash-client';Additionally,
FetchTogglesStatusis used insrc/sagas/featureToggle.js:74but is not exported by the mock. Without these exports, the saga will fail at runtime.Fix
jest.mock('@hathor/unleash-client', () => ({ + default: jest.fn(() => ({ - HathorUnleashClient: jest.fn(() => ({ getAllToggles: () => ({}), isEnabled: () => false, getVariant: () => ({}), updateContext: () => {}, getContext: () => ({ appName: 'hathor-wallet-mobile', environment: 'test', userId: 'test-user', sessionId: 'test-session', remoteAddress: undefined, properties: {}, }), setContextField: () => {}, start: () => {}, stop: () => {}, on: () => {}, })), EVENTS: { INIT: 'INIT', ERROR: 'ERROR', READY: 'READY', UPDATE: 'UPDATE', IMPRESSION: 'IMPRESSION', }, + FetchTogglesStatus: { + Updated: 'updated', + Fetched: 'fetched', + NotFetched: 'not-fetched', + }, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jestMockSetup.js` around lines 95 - 121, The mock currently provides HathorUnleashClient as a named export but your code imports a default UnleashClient and also expects the FetchTogglesStatus export; update the jest.mock for '@hathor/unleash-client' so it exports a default (UnleashClient) that maps to the existing HathorUnleashClient mock and add a named export FetchTogglesStatus with the same enum/values used in src/sagas/featureToggle.js (and keep EVENTS and other methods intact) so imports like "import UnleashClient, { FetchTogglesStatus } from '@hathor/unleash-client';" resolve at test runtime.
🧹 Nitpick comments (4)
docs/testing-guide.md (1)
207-207: Optional: Consider more concise wording.The phrase "lags behind" is clear, but could be simplified to "stays in sync with" or "remains current with" for slightly more direct language.
✍️ Suggested alternative wording
-This keeps the contract honest — it never lags behind the code. +This keeps the contract honest — it stays in sync with the code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing-guide.md` at line 207, Replace the phrase "lags behind the code" in the sentence "keeps the contract honest — it never lags behind the code." with a more concise alternative such as "stays in sync with the code" (resulting in "keeps the contract honest — it never stays in sync with the code."), or better yet "remains current with the code" to yield "keeps the contract honest — it remains current with the code."; update the line in docs/testing-guide.md accordingly to use the chosen concise wording.__tests__/sagas/wallet.test.ts (1)
13-26: Multiple unused imports.Several imported actions appear unused in the test file:
setWallet,setServerInfo,startWalletSuccess,setUseWalletService,setAvailablePushNotification,setUniqueDeviceId,firstAddressRequest,walletRefreshSharedAddress,setFullNodeNetworkName.These may have been intended for additional tests or are remnants from development. Consider removing them to reduce noise.
♻️ Proposed cleanup
import { startWalletRequested, - setWallet, - setServerInfo, - startWalletSuccess, startWalletFailed, resetWalletSuccess, - setUseWalletService, - setAvailablePushNotification, - setUniqueDeviceId, - firstAddressRequest, - walletRefreshSharedAddress, - setFullNodeNetworkName, } from '../../src/actions';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/sagas/wallet.test.ts` around lines 13 - 26, The import list in the test imports many unused action creators (setWallet, setServerInfo, startWalletSuccess, setUseWalletService, setAvailablePushNotification, setUniqueDeviceId, firstAddressRequest, walletRefreshSharedAddress, setFullNodeNetworkName); remove these unused symbols from the import statement at the top of __tests__/sagas/wallet.test.ts so only actually used actions (e.g., startWalletRequested, resetWalletSuccess) remain, or reintroduce any you intend to use in new assertions—update the import to reference only the needed action names to eliminate lint/test noise.__tests__/screens/BackupWords.test.tsx (2)
162-181: Duplicate test coverage for failure modal.
it('shows failure modal when wrong word is selected')(lines 162-181) andit('shows failure modal for wrong selection')(lines 202-223) test the same behavior with identical assertions. The second test's comment explains it was renamed, but both tests verify the same thing: pressing a wrong word shows'Wrong word.'text.Consider removing the duplicate test to reduce maintenance overhead:
♻️ Suggested consolidation
- it('shows failure modal for wrong selection', async () => { - // Renamed from 'failure modal dismiss navigates back' — this test only - // verifies the modal appears. Simulating the BackdropModal dismiss to - // assert goBack() requires reaching into modal internals; left as a - // follow-up. The dismiss → goBack wiring is currently verified by - // reading the screen source. - const { getByText, queryByText } = render( - <BackupWords navigation={mockNavigation} route={mockRoute} /> - ); - - const correctWord = TEST_WORDS_ARR[FIXED_INDEXES[0] - 1]; - const wrongWord = TEST_WORDS_ARR.find((w) => w !== correctWord); - expect(wrongWord).toBeDefined(); - - await act(async () => { - fireEvent.press(getByText(wrongWord!)); - }); - - await waitFor(() => { - expect(queryByText('Wrong word.')).toBeTruthy(); - }); - });Also applies to: 202-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/screens/BackupWords.test.tsx` around lines 162 - 181, Remove the duplicate failure-modal test: either delete the redundant it('shows failure modal when wrong word is selected') or it('shows failure modal for wrong selection') in __tests__/screens/BackupWords.test.tsx, keeping only one test that asserts pressing a wrong word displays 'Wrong word.'; ensure the retained test uses the existing selection logic (BackupWords render with mockNavigation/mockRoute, deriving correctWord from TEST_WORDS_ARR and FIXED_INDEXES, finding wrongWord, firing press, and waiting for queryByText('Wrong word.')), and update the remaining test's description to be clear and unique.
163-163: Unused destructured variablequeryByText.Line 163 destructures
queryByTextbut only usesgetByText. This is harmless but could be cleaned up.♻️ Proposed fix
- const { getByText, queryByText } = render( + const { getByText } = render(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/screens/BackupWords.test.tsx` at line 163, The test destructures queryByText from the render call but never uses it; update the destructuring in the BackupWords.test.tsx test (the line with const { getByText, queryByText } = render(...)) to remove queryByText (i.e., only destructure getByText) or else use queryByText where appropriate so the unused variable is eliminated; ensure any references to render/getByText remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/reducers/reducer.reown.test.ts`:
- Around line 13-15: The test file is missing Jest global imports, causing
describe/it/expect to be flagged by no-undef; add explicit imports from
'@jest/globals' at the top of the test file (e.g., import { describe, it, expect
} from '@jest/globals') so the existing imports (reducer,
setReownPendingRequests, types) remain unchanged and ESLint no-undef is
satisfied.
---
Outside diff comments:
In `@jestMockSetup.js`:
- Around line 95-121: The mock currently provides HathorUnleashClient as a named
export but your code imports a default UnleashClient and also expects the
FetchTogglesStatus export; update the jest.mock for '@hathor/unleash-client' so
it exports a default (UnleashClient) that maps to the existing
HathorUnleashClient mock and add a named export FetchTogglesStatus with the same
enum/values used in src/sagas/featureToggle.js (and keep EVENTS and other
methods intact) so imports like "import UnleashClient, { FetchTogglesStatus }
from '@hathor/unleash-client';" resolve at test runtime.
In `@package.json`:
- Line 1: CI fails because package-lock.json is out of sync with package.json
(lock references typescript@6.0.3 while package.json lists typescript@4.8.4);
fix by regenerating the lockfile: remove the current package-lock.json, run npm
install (or npm ci after ensuring package.json is correct) to produce a lockfile
matching package.json, then commit the updated package-lock.json so npm ci
succeeds; ensure you run this locally and push the regenerated
package-lock.json.
---
Nitpick comments:
In `@__tests__/sagas/wallet.test.ts`:
- Around line 13-26: The import list in the test imports many unused action
creators (setWallet, setServerInfo, startWalletSuccess, setUseWalletService,
setAvailablePushNotification, setUniqueDeviceId, firstAddressRequest,
walletRefreshSharedAddress, setFullNodeNetworkName); remove these unused symbols
from the import statement at the top of __tests__/sagas/wallet.test.ts so only
actually used actions (e.g., startWalletRequested, resetWalletSuccess) remain,
or reintroduce any you intend to use in new assertions—update the import to
reference only the needed action names to eliminate lint/test noise.
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 162-181: Remove the duplicate failure-modal test: either delete
the redundant it('shows failure modal when wrong word is selected') or it('shows
failure modal for wrong selection') in __tests__/screens/BackupWords.test.tsx,
keeping only one test that asserts pressing a wrong word displays 'Wrong word.';
ensure the retained test uses the existing selection logic (BackupWords render
with mockNavigation/mockRoute, deriving correctWord from TEST_WORDS_ARR and
FIXED_INDEXES, finding wrongWord, firing press, and waiting for
queryByText('Wrong word.')), and update the remaining test's description to be
clear and unique.
- Line 163: The test destructures queryByText from the render call but never
uses it; update the destructuring in the BackupWords.test.tsx test (the line
with const { getByText, queryByText } = render(...)) to remove queryByText
(i.e., only destructure getByText) or else use queryByText where appropriate so
the unused variable is eliminated; ensure any references to render/getByText
remain unchanged.
In `@docs/testing-guide.md`:
- Line 207: Replace the phrase "lags behind the code" in the sentence "keeps the
contract honest — it never lags behind the code." with a more concise
alternative such as "stays in sync with the code" (resulting in "keeps the
contract honest — it never stays in sync with the code."), or better yet
"remains current with the code" to yield "keeps the contract honest — it remains
current with the code."; update the line in docs/testing-guide.md accordingly to
use the chosen concise wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4d3c63c-4bd7-4ec5-be8e-4273986cff41
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
__tests__/helpers/mockNavigation.ts__tests__/reducers/reducer.reown.test.ts__tests__/reducers/reducer.wallet.test.ts__tests__/sagas/nanoContracts/historyNanoContract.test.js__tests__/sagas/wallet.test.ts__tests__/screens/BackupWords.test.tsxdocs/testing-guide.mdjestMockSetup.jspackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/helpers/mockNavigation.ts
The previous commit's lock was generated with Node 24 / npm 11, which resolves transitive deps differently from CI's Node 22 / npm 10 and dropped entries CI's `npm ci` expected (notably the nested @react-native-community/cli-config>typescript@6.0.3, surfaced as 'Missing: typescript@6.0.3 from lock file'). Regenerated the lock from the bdbf958 baseline using Node 22.22.2 + npm 10.9.7 (matching the .github/workflows/main.yml matrix), so `npm ci` succeeds. The intended jest-circus@29.7.0 pin and the dropped unrs-resolver subtree are preserved. Suite: 116 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The repo's .eslintrc has no `env: { jest: true }` and no test-file
override, so describe/it/expect tripped no-undef in both reducer
test files (111 errors total). Following the reviewer's suggestion,
import the jest globals explicitly at the top of each file from
'@jest/globals' (the package is already a transitive dep, used by
__tests__/helpers/mockNavigation.ts).
Also fold in two trivial auto-fixes for pre-existing dot-notation
errors at reducer.wallet.test.ts:87,99 ('tokens["token123"]' →
'tokens.token123') so `npm run lint` passes on this branch.
Suite: 38 passed (reducers); no behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- __tests__/sagas/wallet.test.ts: drop unused imports flagged by
reviewer + ESLint (setWallet, setServerInfo, startWalletSuccess,
setUseWalletService, setAvailablePushNotification, setUniqueDeviceId,
firstAddressRequest, walletRefreshSharedAddress,
setFullNodeNetworkName, throwError, select, call,
IGNORE_WS_TOGGLE_FLAG, mockServerInfo). Imports actually referenced
via function-identity comparisons inside .provide()
(isWalletServiceEnabled, isPushNotificationEnabled,
getNetworkSettings, PRE_SETTINGS_MAINNET) are kept.
- __tests__/screens/BackupWords.test.tsx: remove the duplicate
failure-modal test (the renamed 'shows failure modal for wrong
selection' was a near-clone of the canonical 'shows failure modal
when wrong word is selected'). Also remove the unused queryByText
destructure at the 'advances to next step' test. The deferred
dismiss → goBack assertion is documented as a follow-up next to
the canonical failure-modal test.
- jestMockSetup.js: annotate the @hathor/unleash-client mock with the
same pattern used for the Firebase messaging mock — the production
imports (default UnleashClient + named FetchTogglesStatus) don't
match this mock's named-only export shape, but no current test
exercises featureToggle.js so it never matters at runtime. Comment
documents what to add if a future test runs through that saga.
Findings deliberately not changed:
- testing-guide.md "lags behind the code": reviewer's suggested
replacements ("never stays in sync" / "never remains current")
invert the intended meaning. Original phrasing is correct.
Suite: 115 passed (1 fewer than before — the deleted duplicate test).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuliomir
changed the title
- __tests__/helpers/getInitialState.js (new): shared initial-state
helper. Both reducer test files now import from here instead of
redefining the same one-liner. (.js, not .ts: repo ESLint has no
TS-aware import resolver, so .ts helpers fail import/no-unresolved.)
- reducer.wallet.test.ts:
* shape contract now uses sorted-keys equality (toEqual on a sorted
list) instead of toHaveProperty, so a refactor that *moves* a key
under a sub-tree fails the test. Adds a sample-uid pin on
state.tokens to catch a hypothetical state.tokens.byId reshape.
* action-type contract now uses minimal valid payloads (real start
payload, real token, real wallet object) instead of {} / null,
so a future RTK `prepare` callback validating inputs can't mask
the type-string assertion behind a creator-level error.
- reducer.reown.test.ts: import the shared getInitialState helper.
- wallet.test.ts: tighten the wallet.stop comment to accurately
describe what it covers (null-guard short-circuit, paired with
clearAllMocks for cross-test isolation).
- BackupWords.test.tsx: pick the wrong word at FIXED_INDEXES[0] (i.e.
immediately after the correct one) instead of TEST_WORDS_ARR.find
scanning from index 0 — guarantees the wrong word is inside the
on-screen options window regardless of future FIXED_INDEXES values.
- testing-guide.md: fix two doc-accuracy issues — replace
mockNavigation() (no such export) with createMockNavigation /
createMockRoute, and replace the onSetWallet ❌ example (no such
symbol exported from reducer.js) with a real one (onResetWalletSuccess).
Suite: 115 passed, 0 failed (Node 22).
Lint: 0 errors on modified reducer files and the new helper.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the PR's "low-friction for future agents" acceptance criterion,
ship the test conventions in-repo so colleagues' AI agents (Claude
Code, Cursor, Codex, etc.) can pick them up without prior context.
- AGENTS.md (root): cross-tool entry point. Lists the repo
conventions agents must follow — pinned versions, Node-22 lock
regen, the test rules (with pointer to docs/testing-guide.md),
commit/PR conventions.
- CLAUDE.md (root): one-line pointer to AGENTS.md plus a note that
the test skill auto-loads on test work.
- .claude/skills/writing-tests/SKILL.md: detailed Claude Code skill
that auto-loads when an agent edits files under __tests__/ or
matching *.test.{ts,tsx,js}. Encodes the conventions surfaced by
repeated reviews — jest globals import, three-layer reducer safety
net, helper reuse, mock-annotation pattern, anti-patterns list, and
the verify-before-done checklist.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jestMockSetup.js (1)
95-129:⚠️ Potential issue | 🟠 MajorUnleash mock exports incompatible symbols with featureToggle.js imports.
Production code imports default
UnleashClientandFetchTogglesStatusfrom@hathor/unleash-client, but this mock exports namedHathorUnleashClientandEVENTSinstead. When the saga is tested, these imports will resolve to undefined, causing failures at the comparisonFetchTogglesStatus.Updated(line 74 of featureToggle.js).Replace this mock to export default
UnleashClient(orHathorUnleashClientas default) and addFetchTogglesStatusenum matching production values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jestMockSetup.js` around lines 95 - 129, The mock currently exports named HathorUnleashClient and EVENTS which don't match production imports; update the jest.mock to export a default UnleashClient (aliasing HathorUnleashClient as the default export) and add a named FetchTogglesStatus enum with the same members used in production (including Updated) so imports like UnleashClient (default) and FetchTogglesStatus.Updated resolve correctly; keep the existing methods (getAllToggles, isEnabled, getVariant, updateContext, getContext, setContextField, start, stop, on) and EVENTS to preserve behavior.
♻️ Duplicate comments (2)
__tests__/sagas/wallet.test.ts (1)
97-105:⚠️ Potential issue | 🟡 MinorRemove the vacuous
mockWallet.stopassertion in the null-wallet branch.This assertion does not validate saga behavior for Line 87 (
wallet: null) because the outermockWalletis unrelated to that branch’s state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/sagas/wallet.test.ts` around lines 97 - 105, Remove the vacuous assertion that checks mockWallet.stop in the null-wallet branch: delete the expect(mockWallet.stop).not.toHaveBeenCalled() assertion and keep only the meaningful assertion verifying result.storeState.walletStartState equals WALLET_STATUS.NOT_STARTED so the test reflects the null `wallet` branch behavior.jestMockSetup.js (1)
61-84:⚠️ Potential issue | 🟠 MajorMock shape for Firebase Messaging still mismatches production imports.
The mock returns a default callable only, while production paths import named symbols. This stays brittle and will break as soon as those code paths are exercised in tests.
#!/bin/bash set -euo pipefail echo "== Named imports of `@react-native-firebase/messaging` ==" rg -nP "import\\s*\\{[^}]*\\}\\s*from\\s*'@react-native-firebase/messaging'" src __tests__ echo echo "== Current jest mock block ==" sed -n '61,90p' jestMockSetup.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jestMockSetup.js` around lines 61 - 84, The current mock returns only a default callable which breaks production code that imports named symbols; update the jest.mock implementation so it exports the named symbols used in production (e.g. getMessaging, getToken, deleteToken, hasPermission, requestPermission, getInitialNotification, onMessage, onNotificationOpenedApp, setBackgroundMessageHandler, isDeviceRegisteredForRemoteMessages, registerDeviceForRemoteMessages, AuthorizationStatus) as properties on the mocked module (each as jest.fn() or the correct value) and also provide a default export if the codebase expects the factory style; ensure each named export matches the shapes/return types used in tests (e.g. getToken returns a Promise.resolve('mock-token'), AuthorizationStatus is the same enum object).
🧹 Nitpick comments (2)
__tests__/sagas/wallet.test.ts (1)
54-56: PrefergetInitialStatehelper over inline reducer init boilerplate.The repeated
reducer(undefined, { type: '@@INIT' })setup can drift from the shared test pattern. Please reuse the centralized helper.Refactor sketch
+import getInitialState from '../helpers/getInitialState.js'; .withState({ - ...reducer(undefined, { type: '@@INIT' }), + ...getInitialState(), wallet: mockWallet, walletStartState: WALLET_STATUS.READY, }) .withState({ - ...reducer(undefined, { type: '@@INIT' }), + ...getInitialState(), wallet: null, walletStartState: WALLET_STATUS.NOT_STARTED, })As per coding guidelines:
**/__tests__/**/*.{test,spec}.ts?(x): Reuse helpers from__tests__/helpers/and do not redefine setup boilerplate; note that helpers use .js, not .ts.Also applies to: 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/sagas/wallet.test.ts` around lines 54 - 56, Replace the inline reducer initialization (reducer(undefined, { type: '@@INIT' })) with the shared test helper getInitialState(); import or require the helper used by tests and change occurrences like .withState({ ...reducer(undefined, { type: '@@INIT' }), wallet: mockWallet }) to .withState({ ...getInitialState(), wallet: mockWallet }) and do the same for the other occurrence noted (the block around the second instance).__tests__/screens/BackupWords.test.tsx (1)
10-11: Use shared test helpers instead of local setup boilerplate.This test redefines navigation/render setup that should come from
__tests__/helpers/for consistency and lower maintenance.As per coding guidelines:
**/__tests__/**/*.{test,spec}.ts?(x): Reuse helpers from__tests__/helpers/and do not redefine setup boilerplate; note that helpers use .js, not .ts, due to lack of TS-aware import resolver.Also applies to: 106-114, 120-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/screens/BackupWords.test.tsx` around lines 10 - 11, Replace the local test setup/boilerplate in BackupWords.test.tsx (the imports and any custom navigation/render mocks) with the shared test helpers used across tests: import the provided helper(s) (e.g., the project’s renderWithProviders/renderWithNavigation helper) instead of directly importing render/fireEvent/act/waitFor and avoid redefining navigation mocks; remove local navigation/render setup code blocks and call the shared helper (e.g., renderWithNavigation or renderWithProviders) to render the BackupWords component and to obtain fireEvent/waitFor utilities, noting the shared helpers are JavaScript modules (use the .js helper import name) so import them accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/sagas/wallet.test.ts`:
- Around line 7-47: Add explicit Jest globals import at the top of the test file
to satisfy the linter: import the needed symbols (describe, it/test, beforeEach,
expect, jest) from '@jest/globals' so usages in this file (e.g., describe,
beforeEach, jest, expect, it/test) are defined; update the top of the file to
include a single import line that brings these globals in before any test
definitions.
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 9-20: Tests rely on implicit Jest globals causing `no-undef`; add
an explicit import from '@jest/globals' at the top of the test file (e.g.,
import { describe, it, expect, jest } from '@jest/globals') so all usages like
expect(...), jest.mock(...), and any describe/it blocks are defined; place this
import above the existing imports near TEST_WORDS / FIXED_INDEXES declarations
to satisfy linting and ensure jest is available in the file.
- Around line 135-143: The test name says "shows 5 word option buttons" but the
assertion only checks that a single expected word exists; update the test body
to actually assert five option buttons exist (or rename the test to reflect the
single-word check). Concretely, in the "shows 5 word option buttons" test for
BackupWords, replace the single-word assertion with a check that queries the
option controls (e.g., getAllByRole('button') or the component's option test id)
and asserts .length === 5, or alternatively rename the it(...) string to match
the current expectation if you prefer not to change the assertion; reference
TEST_WORDS_ARR, FIXED_INDEXES and the BackupWords test block when making the
change.
- Around line 193-197: The loop uses ++ and awaits inside the loop which
triggers lint rules `no-plusplus` and `no-await-in-loop`; instead build the list
of target words from FIXED_INDEXES and dispatch all presses concurrently using
Promise.all. Specifically, compute words = FIXED_INDEXES.slice(0, 5).map(idx =>
TEST_WORDS_ARR[idx - 1]), then create an array of promises by mapping each word
to act(async () => fireEvent.press(getByText(word))) and finally await
Promise.all(promises); this replaces the for loop and references TEST_WORDS_ARR,
FIXED_INDEXES, act, fireEvent.press and getByText.
---
Outside diff comments:
In `@jestMockSetup.js`:
- Around line 95-129: The mock currently exports named HathorUnleashClient and
EVENTS which don't match production imports; update the jest.mock to export a
default UnleashClient (aliasing HathorUnleashClient as the default export) and
add a named FetchTogglesStatus enum with the same members used in production
(including Updated) so imports like UnleashClient (default) and
FetchTogglesStatus.Updated resolve correctly; keep the existing methods
(getAllToggles, isEnabled, getVariant, updateContext, getContext,
setContextField, start, stop, on) and EVENTS to preserve behavior.
---
Duplicate comments:
In `@__tests__/sagas/wallet.test.ts`:
- Around line 97-105: Remove the vacuous assertion that checks mockWallet.stop
in the null-wallet branch: delete the
expect(mockWallet.stop).not.toHaveBeenCalled() assertion and keep only the
meaningful assertion verifying result.storeState.walletStartState equals
WALLET_STATUS.NOT_STARTED so the test reflects the null `wallet` branch
behavior.
In `@jestMockSetup.js`:
- Around line 61-84: The current mock returns only a default callable which
breaks production code that imports named symbols; update the jest.mock
implementation so it exports the named symbols used in production (e.g.
getMessaging, getToken, deleteToken, hasPermission, requestPermission,
getInitialNotification, onMessage, onNotificationOpenedApp,
setBackgroundMessageHandler, isDeviceRegisteredForRemoteMessages,
registerDeviceForRemoteMessages, AuthorizationStatus) as properties on the
mocked module (each as jest.fn() or the correct value) and also provide a
default export if the codebase expects the factory style; ensure each named
export matches the shapes/return types used in tests (e.g. getToken returns a
Promise.resolve('mock-token'), AuthorizationStatus is the same enum object).
---
Nitpick comments:
In `@__tests__/sagas/wallet.test.ts`:
- Around line 54-56: Replace the inline reducer initialization
(reducer(undefined, { type: '@@INIT' })) with the shared test helper
getInitialState(); import or require the helper used by tests and change
occurrences like .withState({ ...reducer(undefined, { type: '@@INIT' }), wallet:
mockWallet }) to .withState({ ...getInitialState(), wallet: mockWallet }) and do
the same for the other occurrence noted (the block around the second instance).
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 10-11: Replace the local test setup/boilerplate in
BackupWords.test.tsx (the imports and any custom navigation/render mocks) with
the shared test helpers used across tests: import the provided helper(s) (e.g.,
the project’s renderWithProviders/renderWithNavigation helper) instead of
directly importing render/fireEvent/act/waitFor and avoid redefining navigation
mocks; remove local navigation/render setup code blocks and call the shared
helper (e.g., renderWithNavigation or renderWithProviders) to render the
BackupWords component and to obtain fireEvent/waitFor utilities, noting the
shared helpers are JavaScript modules (use the .js helper import name) so import
them accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e662efe8-9668-4e50-ae18-ccab472a3c0f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.claude/skills/writing-tests/SKILL.mdAGENTS.mdCLAUDE.md__tests__/helpers/getInitialState.js__tests__/reducers/reducer.reown.test.ts__tests__/reducers/reducer.wallet.test.ts__tests__/sagas/wallet.test.ts__tests__/screens/BackupWords.test.tsxdocs/testing-guide.mdjestMockSetup.js
✅ Files skipped from review due to trivial changes (2)
- tests/helpers/getInitialState.js
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/reducers/reducer.reown.test.ts
- tests/reducers/reducer.wallet.test.ts
Audit by a Test/QA specialist subagent against the strategic premise "docs should cover what a frontier agent can't infer from the code, not restate what's plain in the existing tests." Net: cut ~30% filler, add seven concrete repo-specific gotchas, dedupe between the two files. SKILL.md (auto-loads on test work): - Frontmatter description tightened to a trigger condition + scope. - New "Repo-specific gotchas" section at the top with the seven concrete invariants surfaced by the audit: * jest-circus pinned at 29.7.0 (don't bump) * @hathor/wallet-lib sub-paths already mocked in jestMockSetup.js * legacy_createStore in mockStore.ts is intentional (no RTK yet) * networkSettings.test.ts is tech debt — don't pattern-match from * testPathIgnorePatterns carve-outs (helpers/ + nanoContracts fixture) * silenceTimeout idiom for sagas using delay() * 'bound start' matcher fragility in wallet.test.ts:147-164 - Section 4 (helpers) collapsed to the two non-obvious points. - Section 6 (component) cut to the snapshot-blob rule only. - Section 8 (anti-patterns) cut generic items lint already catches. - Section 10 leads with the mutation drill. - Escalation header: SKILL wins over testing-guide.md on conflict. testing-guide.md (long-form / non-Claude-Code agents): - Cut the four-layer pyramid prose; kept the policy table. - Cut "Test the public contract", "Reuse helpers", "One test one fact" (all duplicated SKILL.md word-for-word). - Cut "Running tests locally", "Coverage thresholds", "Known historical issues", and the CI-follow-ups appendix. - Kept the load-bearing principles: "Don't mock what you can run", "Don't leak persistence", "Use real action creators" with the no-creator exception. - Pointer at top: SKILL.md is canonical; this file lags it. No code changes. Suite: 115 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit's stricter ESLint pass surfaced four issues that the
repo's looser local lint config was tolerating. Local lint will be
hardened in a follow-up PR; this commit just fixes the issues.
- __tests__/sagas/wallet.test.ts: import describe/it/expect/jest/
beforeEach from '@jest/globals' to clear no-undef per repo
convention (also documented in .claude/skills/writing-tests/SKILL.md).
- __tests__/screens/BackupWords.test.tsx:
* Add the same jest globals import.
* Strengthen the 'shows 5 word option buttons' test — it previously
only asserted the correct word was present (mismatching its name).
Now asserts each of the 5 expected words in the visible window
around FIXED_INDEXES[0] = 3 (TEST_WORDS_ARR[0..4]). Fixes the
co-located max-len comment as a side effect.
* Refactor the 5-step success-modal loop from
`for (let i = 0; i < 5; i++) { await ... }` to a serial
FIXED_INDEXES.reduce chain, clearing both no-plusplus and
no-await-in-loop without parallelizing (steps must run serially).
Suite: 115 passed, 0 failed (Node 22).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
__tests__/screens/BackupWords.test.tsx (1)
174-186: Make wrong-option selection truly boundary-safe.
correctIdx + 1is not robust for future index choices; pick a rendered option that is not the correct word.Proposed refactor
const correctIdx = FIXED_INDEXES[0] - 1; const correctWord = TEST_WORDS_ARR[correctIdx]; - const wrongWord = TEST_WORDS_ARR[correctIdx + 1]; - expect(wrongWord).not.toEqual(correctWord); + const wrongWord = TEST_WORDS_ARR.find( + (word) => word !== correctWord && queryByText(word), + ); + expect(wrongWord).toBeTruthy(); await act(async () => { - fireEvent.press(getByText(wrongWord)); + fireEvent.press(getByText(wrongWord as string)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/screens/BackupWords.test.tsx` around lines 174 - 186, The test assumes wrongWord = TEST_WORDS_ARR[correctIdx + 1] which can break if FIXED_INDEXES changes; instead compute the set of rendered options around FIXED_INDEXES[0] (the same slice/indices BackupWords uses) and pick the first entry from that renderedOptions array that is not equal to correctWord. Update the test to derive wrongWord by scanning renderedOptions for w => w !== correctWord (use the existing FIXED_INDEXES, TEST_WORDS_ARR, correctIdx, correctWord identifiers) and then call fireEvent.press(getByText(wrongWord)) so the chosen wrong option is guaranteed to be on-screen and different from the correct word.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 45-55: The mock classes MemoryStore and Storage violate the
lines-between-class-members lint rule; edit the mock so each method declaration
in class MemoryStore (getItem, setItem, cleanStorage, clearItems) and each
method/constructor in class Storage are separated by a blank line (add an empty
line between members) to satisfy lines-between-class-members; update the mock
block around the MemoryStore and Storage classes referenced here so methods are
spaced accordingly.
- Around line 9-14: Remove the inline require and useless fragment in the Portal
mock and move the BackupWords import to the top of the test file: replace the
Portal mock implementation so it no longer does require('react') and simply
returns its children directly (no <>...</> fragment), and relocate the import of
BackupWords to the top-level imports section so all imports are first; ensure
the mock still exports Portal (or the same identifier used in tests) and that
BackupWords is imported before any mocks or test code runs.
In @.claude/skills/writing-tests/SKILL.md:
- Around line 134-138: Update the doc to clarify that the ".js helpers" rule is
prospective: state that new helper files should be authored as .js to avoid
import/no-unresolved failures for consumers that lack a TS-aware resolver, and
explicitly call out existing exceptions (mockNavigation.ts, mockStore.ts,
renderWithProviders.tsx) which remain TS until eslint-import-resolver-typescript
is added; alternatively note that getInitialState.js is the current JS example
and describe which consumers (ESLint import resolver) require .js and that a
follow-up task will convert or allow TS once the resolver is configured.
---
Nitpick comments:
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 174-186: The test assumes wrongWord = TEST_WORDS_ARR[correctIdx +
1] which can break if FIXED_INDEXES changes; instead compute the set of rendered
options around FIXED_INDEXES[0] (the same slice/indices BackupWords uses) and
pick the first entry from that renderedOptions array that is not equal to
correctWord. Update the test to derive wrongWord by scanning renderedOptions for
w => w !== correctWord (use the existing FIXED_INDEXES, TEST_WORDS_ARR,
correctIdx, correctWord identifiers) and then call
fireEvent.press(getByText(wrongWord)) so the chosen wrong option is guaranteed
to be on-screen and different from the correct word.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1ccd625-ee73-4b50-90fe-85facb3e55ef
📒 Files selected for processing (4)
.claude/skills/writing-tests/SKILL.md__tests__/sagas/wallet.test.ts__tests__/screens/BackupWords.test.tsxdocs/testing-guide.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/sagas/wallet.test.ts
Reviewer audit flagged that 8f9eeb2 silently shrunk nanoContract test coverage during a refactor-driven test rewrite. Restore the lost contracts and add equivalents for code paths whose original tests targeted removed production code: - restore describe('sagas/nanoContract/fetchHistory') as direct-function tests, adapted to the new fetchHistory(req) signature; assert the snake_case→camelCase tx-shape transform and authority mapping - restore expect(fetchHistoryCall.payload.fn).toBe(fetchHistory) ref-pin - restore termination + property-existence asserts on the success paths of both files - add equivalent test for the lock short-circuit (production lines 405-412), since the original 'history loading' test exercised a redux-state-based mechanism that no longer exists - add two tests for the getBlueprintInformation throw branches (404 vs generic error), since the original 'getNanoContractState error' test exercised an API call that no longer exists Mutation drill: flipped txId field, weakened lock check, swapped 404 failure message — each broke exactly one new assertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit flagged that __tests__/utils.test.ts and __tests__/screens/InitWallet.test.tsx use describe/it/expect/jest without importing them from '@jest/globals'. They lint clean under CI's `eslint .` because that defaults to .js only and silently skips .ts/.tsx — but the import is mandatory by the writing-tests skill rule §1, and any future shift to a TS-aware lint pass will trip 100+ no-undef errors otherwise. Both files still pass jest (33/33 on the affected suites). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BackupWords + InitWallet:
- add blank lines between class members in MemoryStore/Storage
mocks and InitWallet's InvalidWords (lines-between-class-members)
- BackupWords: hoist BackupWords import to module top (import/first);
jest.mock babel-hoists regardless of where the import sits
- BackupWords: drop dead `const React = require('react')` from the
Portal mock (global-require) and the redundant fragment wrapper —
ReactNode is itself a valid render value
- BackupWords: remove unused `shuffleCallCount` accumulator
- InitWallet: hoist `Switch` import out of the function body
(global-require); narrowly suppress camelCase rule for RTL's
upstream `UNSAFE_getByType` API; drop double space before comment
SKILL.md §4.2: clarify the "new helpers must be .js" rule is
prospective. Name getInitialState.js as the JS reference example
and explicitly list the three TS helper exceptions (mockNavigation.ts,
mockStore.ts, renderWithProviders.tsx) as scheduled-to-flip-or-keep
pending the typescript resolver.
Remaining lint error in both files is the pre-existing
`react/jsx-filename-extension`-vs-`.tsx` repo config tension
(JSX disallowed in .tsx). Out of scope; CodeRabbit didn't flag it.
Tests: 15/15 pass on the two screen suites.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/screens/BackupWords.test.tsx`:
- Around line 138-150: The test only checks that each expected label from
expectedOptions appears, but doesn't assert the number of selectable options;
update the 'shows 5 word option buttons' test in the BackupWords spec to first
query the rendered option controls (e.g., using getAllByRole('button') or a
dedicated testID query for the option elements produced by BackupWords) and
assert the returned array length is exactly 5, then keep the existing loop that
uses getByText(word) against expectedOptions to ensure the correct labels are
present; reference the BackupWords render call, expectedOptions, and the current
getByText assertions when making the change.
In `@__tests__/screens/InitWallet.test.tsx`:
- Around line 110-119: The test currently infers the Start control is disabled
by asserting mockNavigation.navigate wasn't called; instead locate the Start
label (getByText('Start')) and assert the actual pressable wrapper
(TouchableOpacity or Pressable used in WelcomeScreen) exposes the disabled
contract—e.g., its disabled prop is true or its accessibilityState.disabled is
true—so change the assertion in InitWallet.test.tsx to check the parent
pressable component around the 'Start' text for disabled/accessibilityState
rather than relying on navigate not being called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4812dc1e-e466-4dd8-ab18-877320fcf763
📒 Files selected for processing (7)
.claude/skills/writing-tests/SKILL.md__tests__/sagas/nanoContracts/fixtures.js__tests__/sagas/nanoContracts/historyNanoContract.test.js__tests__/sagas/nanoContracts/registerNanoContract.test.js__tests__/screens/BackupWords.test.tsx__tests__/screens/InitWallet.test.tsx__tests__/utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/utils.test.ts
- tests/sagas/nanoContracts/registerNanoContract.test.js
- tests/sagas/nanoContracts/historyNanoContract.test.js
BackupWords: 'shows 5 word option buttons' previously verified the
expected 5 words appear but didn't pin the count — a regression that
expanded the visible window to 6+ would silently pass. Add a negative
assertion that the 6th source word ('absent', TEST_WORDS_ARR[5])
does NOT render. Equivalent to the role-based count check CodeRabbit
suggested, but works with NewHathorButton (which lacks an explicit
accessibilityRole that would let getAllByRole('button') find it).
InitWallet: 'renders the Start button as disabled initially' previously
inferred disabled-ness from "navigate wasn't called" — would also pass
for unrelated reasons (mock wiring drift, etc.). Switch to a direct
contract assertion: UNSAFE_getByType(NewHathorButton) and verify
props.disabled === true. Keep the press+navigate check as
belt-and-suspenders. (toBeDisabled() matcher would be cleaner but
@testing-library/jest-native is in deps without a setupFilesAfterEach
wiring it up — out of scope.)
Mutation drill: extending the BackupWords slice end +1 pushed 'absent'
into render → queryByText catches it. Forcing disabled={false} on the
Start button → props.disabled assertion fails. Both new assertions are
non-degenerate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuliomir
added a commit
to HathorNetwork/rfcs
that referenced
this pull request
Apr 29, 2026
Promotes 0004-automated-test-suite.md into a folder following the 0003-token-swap pattern. The deep iOS Fabric/XCUITest implementation detail and the wallet-creation E2E walkthrough move into a companion phase-4-e2e-deep-dive.md annex; the main RFC keeps a Phase 4 summary with anchors into the annex. Syncs the RFC against the proposed Layers 1-3 implementation in HathorNetwork/hathor-wallet-mobile#863: - name the Layer 3 helper set (renderWithProviders, createTestStore, createMockNavigation, getInitialState) - add a Test layout table (layer to directory) and name jestMockSetup.js as the central mock registry - name the RTK-slices safety net rationale (behavior, state-shape, action-type contract triple) for reducer tests - list the additional dev deps shipped by the PR (jest-circus, @jest/globals, @types/jest) - acknowledge clearLoadingLocksForTesting as the single Layer 1-3 production-code line - correct the path of the canonical companion docs in Future Possibilities #7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuliomir
added a commit
to HathorNetwork/rfcs
that referenced
this pull request
Apr 29, 2026
Promotes 0004-automated-test-suite.md into a folder following the 0003-token-swap pattern. The deep iOS Fabric/XCUITest implementation detail and the wallet-creation E2E walkthrough move into a companion phase-4-e2e-deep-dive.md annex; the main RFC keeps a Phase 4 summary with anchors into the annex. Syncs the RFC against the proposed Layers 1-3 implementation in HathorNetwork/hathor-wallet-mobile#863: - name the Layer 3 helper set (renderWithProviders, createTestStore, createMockNavigation, getInitialState) - add a Test layout table (layer to directory) and name jestMockSetup.js as the central mock registry - name the RTK-slices safety net rationale (behavior, state-shape, action-type contract triple) for reducer tests - list the additional dev deps shipped by the PR (jest-circus, @jest/globals, @types/jest) - acknowledge clearLoadingLocksForTesting as the single Layer 1-3 production-code line - correct the path of the canonical companion docs in Future Possibilities #7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds the first three layers of automated testing (unit, saga-integration, and component/screen) for hathor-wallet-mobile, plus Jest infra/mocks and contributor/agent-facing testing documentation, to make npm test green and keep future refactors (notably the reducer→RTK slices migration) safer to review.
Changes:
- Introduces new test suites for
utils, wallet lifecycle + Reown pending-requests reducers, wallet sagas, and InitWallet/BackupWords screens. - Updates Jest tooling/config (new dev deps, updated mocks, jest ignore/transform patterns) to support RN/WalletConnect/wallet-lib test execution.
- Adds test-writing guidance docs and agent conventions, and a small production export (
clearLoadingLocksForTesting) for test isolation.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/sagas/nanoContract.js |
Exports a test-only helper to clear module-level history loading locks. |
package.json |
Adds testing devDependencies and updates Jest config ignore/transform patterns. |
package-lock.json |
Lockfile updates for newly added testing dependencies and transitive resolutions. |
jestMockSetup.js |
Expands Jest mocks for Firebase, Unleash client, WalletConnect compat, camera-kit, and wallet-lib subpaths. |
docs/testing-guide.md |
Adds a long-form testing guide describing the 4-layer pyramid and PR expectations. |
__tests__/utils.test.ts |
Unit tests for src/utils.js helpers (amount parsing, short hash/content, token label). |
__tests__/screens/InitWallet.test.tsx |
Component tests for InitWallet screens (terms toggle, navigation, word generation). |
__tests__/screens/BackupWords.test.tsx |
Component tests for BackupWords 5-step validation flow with deterministic shuffling. |
__tests__/sagas/wallet.test.ts |
Saga integration tests for wallet start/reset using redux-saga-test-plan. |
__tests__/sagas/reown.test.js |
Fixes generator-driving expectations for Reown reject flows (incl. resync). |
__tests__/sagas/nanoContracts/registerNanoContract.test.js |
Updates nano contract registration saga tests for blueprint-id/info behavior and error branches. |
__tests__/sagas/nanoContracts/historyNanoContract.test.js |
Updates nano contract history saga tests and adds lock short-circuit coverage. |
__tests__/sagas/nanoContracts/fixtures.js |
Extends nano contract fixtures to match new history payload shape. |
__tests__/reducers/reducer.wallet.test.ts |
Reducer contract tests (behavior + state shape + action type strings) for wallet lifecycle/tokens/reset. |
__tests__/reducers/reducer.reown.test.ts |
Reducer contract tests for state.reown.pendingRequests + action type literal. |
__tests__/helpers/renderWithProviders.tsx |
Adds RTL helper to render with Redux + optional navigation container. |
__tests__/helpers/mockStore.ts |
Adds helper to create a real-reducer Redux store with optional preloaded state. |
__tests__/helpers/mockNavigation.ts |
Adds navigation/route mock factories for React Navigation 7. |
__tests__/helpers/getInitialState.js |
Adds shared helper to retrieve the root reducer initial state. |
__tests__/App.test.tsx |
Fixes redux mocking to support legacy_createStore usage. |
CLAUDE.md |
Adds a pointer doc for Claude Code usage in this repo. |
AGENTS.md |
Adds repo conventions for agents, including dependency/versioning and testing rules. |
.claude/skills/writing-tests/SKILL.md |
Adds an auto-loaded “writing tests” skill documenting test conventions and pitfalls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+222
to
+233
| firstGen.next(); | ||
|
|
||
| // Start a second saga for the SAME ncId + same (initial) request type. | ||
| // The synchronous lock check at the top of requestHistoryNanoContract | ||
| // must observe the held lock and `return` immediately — no put(loading), | ||
| // no select, no anything observable. | ||
| const secondGen = requestHistoryNanoContract(nanoContractHistoryRequest({ ncId })); | ||
| const firstYield = secondGen.next(); | ||
|
|
||
| // `done: true` with `value: undefined` is a generator that returned | ||
| // without yielding. That is exactly what the lock short-circuit does. | ||
| expect(firstYield).toEqual({ value: undefined, done: true }); |
There was a problem hiding this comment.
The last test intentionally leaves firstGen paused after acquiring the module-level loading lock, and the file only clears locks in beforeEach. If this test is the last one in the file (or if this file runs before other nanoContract history tests in the same Jest worker), the held lock can leak across test files and create order-dependent failures. Prefer releasing the lock at the end of the test (e.g., drive the generator to completion / call firstGen.return() so the saga’s finally runs, or clear the locks in an afterEach).
Suggested change
| firstGen.next(); | |
| // Start a second saga for the SAME ncId + same (initial) request type. | |
| // The synchronous lock check at the top of requestHistoryNanoContract | |
| // must observe the held lock and `return` immediately — no put(loading), | |
| // no select, no anything observable. | |
| const secondGen = requestHistoryNanoContract(nanoContractHistoryRequest({ ncId })); | |
| const firstYield = secondGen.next(); | |
| // `done: true` with `value: undefined` is a generator that returned | |
| // without yielding. That is exactly what the lock short-circuit does. | |
| expect(firstYield).toEqual({ value: undefined, done: true }); | |
| try { | |
| firstGen.next(); | |
| // Start a second saga for the SAME ncId + same (initial) request type. | |
| // The synchronous lock check at the top of requestHistoryNanoContract | |
| // must observe the held lock and `return` immediately — no put(loading), | |
| // no select, no anything observable. | |
| const secondGen = requestHistoryNanoContract(nanoContractHistoryRequest({ ncId })); | |
| const firstYield = secondGen.next(); | |
| // `done: true` with `value: undefined` is a generator that returned | |
| // without yielding. That is exactly what the lock short-circuit does. | |
| expect(firstYield).toEqual({ value: undefined, done: true }); | |
| } finally { | |
| // Close the first generator so its finally block runs and releases the | |
| // module-level loading lock before other tests execute. | |
| firstGen.return(); | |
| } |
Comment on lines
+108
to
+124
| @@ -116,8 +118,10 @@ | |||
| "eslint-plugin-jsx-a11y": "6.7.1", | |||
| "eslint-plugin-react": "7.32.2", | |||
| "jest": "29.7.0", | |||
| "jest-circus": "29.7.0", | |||
| "patch-package": "8.0.0", | |||
| "react-test-renderer": "18.3.1", | |||
| "redux-saga-test-plan": "^4.0.6", | |||
There was a problem hiding this comment.
New devDependencies are using caret ranges (e.g. @testing-library/*, redux-saga-test-plan). This repo’s package.json otherwise pins exact versions; leaving ranges here defeats reproducible installs / supply-chain pinning and can cause lockfile churn. Please replace these with exact versions and regenerate package-lock.json with the project’s Node/npm versions.
| "@react-native/eslint-config": "0.72.2", | ||
| "@react-native/metro-config": "0.77.2", | ||
| "@react-native/typescript-config": "0.77.2", | ||
| "@testing-library/jest-native": "^5.4.3", |
There was a problem hiding this comment.
@testing-library/jest-native is deprecated (it’s marked DEPRECATED in the lockfile). Since @testing-library/react-native v12.4+ ships built-in Jest matchers, consider removing this dependency and enabling the matchers via the recommended @testing-library/react-native setup instead, to avoid relying on an unmaintained package.
Suggested change
| "@testing-library/jest-native": "^5.4.3", |
Three Copilot findings, all real: 1. historyNanoContract.test.js — the new lock-short-circuit test acquires the module-level loading lock via firstGen.next() and never releases it. beforeEach clears locks BEFORE the next test, so anything between leaves stale state in the worker. Wrap the inner block in try/finally and call firstGen.return() to drive the generator's finally branch (which calls cleanupLoadingLock). 2. package.json — three test devDeps were caret-ranged (@testing-library/jest-native, @testing-library/react-native, redux-saga-test-plan), violating the repo's exact-pin rule. Pin all to exact versions and regenerate the lockfile on Node 22 / npm 10 (per CI matrix). 3. @testing-library/jest-native is deprecated and was never wired up in this branch (no setupFilesAfterEach loading extend-expect). @testing-library/react-native v12.4+ ships built-in Jest matchers, so the dep is dead weight. Remove it entirely. Lockfile loses 4 packages. Promoted the exact-pinning rule from AGENTS.md into CLAUDE.md so Claude Code agents see it on cold start without needing to follow the pointer chain. Tests: 120 passed (no delta). Lint: clean. npm ci --dry-run: clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit duplicated the pin-exact-versions rule into CLAUDE.md while AGENTS.md kept its own copy and pointed *to* CLAUDE.md — circular and a maintenance hazard. Single-source it in AGENTS.md and reduce CLAUDE.md to a thin pointer. AGENTS.md is also tightened to roughly the same density as the old CLAUDE.md draft (about 25% shorter than before, same content). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuliomir
changed the title
1 task
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
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.
Summary
Introduces the first three layers of the automated test suite for
hathor-wallet-mobile, scoped from RFC #110 (HathorNetwork/rfcs#110) where the multi-layer testing pyramid is designed and motivated:utils,reducer.walletlifecycle,reducer.reownpending-requests slice.walletstart/reset sagas viaredux-saga-test-plan.InitWalletandBackupWordsscreens via@testing-library/react-native.Layer 4 (E2E) is intentionally excluded from this PR — it is the largest and most contentious layer (framework choice, native build pipeline, CI cost) and will land separately so reviewers can evaluate Layers 1-3 on their own merits. See RFC #110 for the full rationale and the Layer 4 evaluation work.
What's in
@testing-library/react-native,@testing-library/jest-native,redux-saga-test-plan,jest-circus.__tests__/helpers/{getInitialState,mockNavigation,mockStore,renderWithProviders}shared across specs.jestMockSetup.js: refreshed mocks for@react-native-firebase/{messaging,app},@hathor/unleash-client(replacing the removedunleash-proxy-client),react-native-camera-kit,@walletconnect/react-native-compat, and wallet-lib sub-path imports.package.jsonjest config:testPathIgnorePatternsforhelpers/and the legacynanoContracts/fixtures.js;@walletconnectand@testing-libraryadded totransformIgnorePatterns.App.test.tsx(legacy_createStore),historyNanoContract.test.js(saga lock ordering),registerNanoContract.test.js(blueprint fetch steps).clearLoadingLocksForTestingexported fromsrc/sagas/nanoContract.jspurely for Layer 2 test isolation.__tests__/reducers/): behavior + initial-state-shape + action-type contract layers, scoped to the wallet/tokens/reset surface plus the newstate.reown.pendingRequestshandler. Lets the upcoming RTK-slices migration ofsrc/reducers/reducer.jsbe reviewed primarily by the test diff.AGENTS.mdandCLAUDE.mdat the repo root — concise repo conventions for any AI coding agent (cross-tool format)..claude/skills/writing-tests/SKILL.md— Claude Code skill that auto-loads on test work and encodes the conventions surfaced by repeated reviews so each new test PR doesn't re-fight the same nits.docs/testing-guide.md(renamed from the oldtesting-ci-plan.md): rich human-readable reference covering the four-layer pyramid, per-PR test checklist, design principles, and future-proofing patterns.What's NOT in
e2e/or.maestro/directory.docs/testing-guide.mdshows the snippet to add to.github/workflows/main.ymlonce this lands.Out-of-scope fix bundled in
__tests__/sagas/reown.test.js(added on master by #857) was failing the moment this branch ran the full suite. Both the test andsrc/sagas/reown.jsare byte-identical to master — the test was off-by-one in countinggen.next()returns and missed the saga'sgetPendingSessionRequestsre-fetch afterput(reownReject()). The bug pre-dated this PR; it just wasn't visible because master CI doesn't currently run jest. Fixed in4f4e9d3and labelled in the commit message; happy to extract to a master-only PR if preferred.Acceptance criteria
npm teston a fresh clone reports zero failures and Layer 1-3 deliverable suites are green.clearLoadingLocksForTestingtest-only export insrc/sagas/nanoContract.js.AGENTS.md(cross-tool entry point),CLAUDE.md(Claude Code pointer),.claude/skills/writing-tests/SKILL.md(auto-loads on test work), anddocs/testing-guide.md(long-form reference). A new contributor — human or agent — can add a test that follows the repo's contracts and review conventions without prior session context.Design rationale
See RFC #110 for: layer definitions and scope boundaries, dependency choices (RTL vs Enzyme, redux-saga-test-plan vs manual
gen.next()driving for sagas), file-organization conventions, and the rationale for deferring Layer 4 (E2E) to a separate review cycle.Diff size breakdown
package-lock.json+package.jsonjestMockSetup.jsSummary by CodeRabbit
Tests
Chores
Documentation