feat: Add critical path testing (Phase 2) #301
Conversation
- Install @testing-library/react-native and react-test-renderer - Create jest.setup.js with comprehensive mocks for React Native and Expo modules - Update jest.config.js with coverage settings and module mapping - Add test utilities in __tests__/utils/: - test-helpers.tsx: Component rendering, API mocking, storage mocks - store-helpers.ts: Zustand store reset and setup helpers - test-helpers.test.ts: Tests for utility functions - Update eslint.config.js to include Jest globals for test files - Update AGENTS.md with lint and TypeScript checking instructions - Fix React Query v5 compatibility (cacheTime -> gcTime) All tests passing (18 tests)
- Fix crypto mock to produce deterministic but content-aware hashes (different inputs now produce different outputs for PIN tests) - Add state isolation for MMKV mock (new storage per createMMKV call) - Add state isolation for SecureStore mock (cleared between tests) - Remove commented console mutation code - Add documentation comment for node test environment choice - Return cleanup function from setupTestMerchant for proper teardown
- Fix crypto mock to produce deterministic but content-aware hashes (different inputs now produce different outputs for PIN tests) - Add state isolation for MMKV mock (new storage per createMMKV call) - Add state isolation for SecureStore mock (cleared between tests) - Remove commented console mutation code - Add documentation comment for node test environment choice - Return cleanup function from setupTestMerchant for proper teardown
Phase 2 tests include: - Utility tests: misc.ts (getDate, getDeviceIdentifier), payment-errors.ts - Store tests: useLogsStore.ts (addLog, clearLogs, MAX_LOGS_COUNT) - Store tests: useSettingsStore.ts (all methods including PIN management) - Service tests: client.ts (ApiClient class with error handling) - Service tests: payment.ts (getApiHeaders, startPayment, getPaymentStatus) - Service tests: hooks.ts (useStartPayment, usePaymentStatus React Query hooks) Also merges phase-1 testing infrastructure setup.
- Remove unused getDate import from payment-success.tsx - Fix prettier formatting for tokenDecimals ternary - Set __DEV__ = false in jest.setup.js to suppress console.log during tests
- Suppress React Query act() warnings in jest.setup.js - Add --forceExit to test script to handle React Query open handles
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review |
|
Claude finished @ignaciosantise's task in 4m 10s —— View job Code Review CompleteReviewed Phase 2 test implementation (136 tests across 7 files). Todo:
Issue 1: Missing timeout configuration for API client HTTP requestsID: client-missing-timeout-7f3a Tests verify API client functionality but don't test timeout behavior. The underlying Recommendation: Add timeout configuration to the API client and add corresponding tests: // In client.ts
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), timeout || 30000);
const response = await fetch(url, { ...options, signal: controller.signal });
clearTimeout(timeoutId);
// In test file
it("should timeout long-running requests", async () => {
jest.useFakeTimers();
(global.fetch as jest.Mock).mockImplementation(() =>
new Promise(resolve => setTimeout(resolve, 60000))
);
const promise = apiClient.get("/slow-endpoint");
jest.advanceTimersByTime(31000);
await expect(promise).rejects.toThrow(/timeout|aborted/i);
jest.useRealTimers();
});Issue 2: React Query hook tests don't verify polling/refetch behaviorID: hooks-missing-polling-test-8c4d The Recommendation: Add polling behavior tests: it("should poll when status is not final and pollInMs is set", async () => {
let callCount = 0;
(getPaymentStatus as jest.Mock).mockImplementation(() => {
callCount++;
return Promise.resolve({
status: callCount < 3 ? "processing" : "succeeded",
isFinal: callCount >= 3,
pollInMs: callCount < 3 ? 1000 : 0,
});
});
const { result } = renderHook(() => usePaymentStatus("pay_123"), {
wrapper: createWrapper(),
});
// Wait for multiple polls
await waitFor(() => {
expect(callCount).toBeGreaterThanOrEqual(3);
}, { timeout: 10000 });
expect(result.current.data?.status).toBe("succeeded");
});Issue 3: PIN verification tests don't validate timing attack resistanceID: settings-pin-timing-vuln-2a9f PIN verification tests validate correctness but don't verify constant-time comparison. The mock hash function in jest.setup.js:71 is deterministic but timing attacks could reveal PIN information through comparison timing differences. Recommendation: Ensure PIN verification uses constant-time comparison in the implementation: // In useSettingsStore.ts verifyPin method
// Instead of direct string comparison:
// if (hashedPin === storedHash) { ... }
// Use constant-time comparison:
import { timingSafeEqual } from 'crypto';
const inputBuffer = Buffer.from(hashedPin, 'utf-8');
const storedBuffer = Buffer.from(storedHash, 'utf-8');
if (inputBuffer.length === storedBuffer.length &&
timingSafeEqual(inputBuffer, storedBuffer)) {
// PIN valid
}Issue 4: Test uses hardcoded test URLs instead of environment variablesID: jest-hardcoded-test-urls-4b2c The jest.setup.js file sets test environment variables with hardcoded test URLs ( Recommendation: Use RFC 2606 reserved domains for test URLs: // Replace:
process.env.EXPO_PUBLIC_API_URL =
process.env.EXPO_PUBLIC_API_URL || "https://api.test.example.com";
// With:
process.env.EXPO_PUBLIC_API_URL =
process.env.EXPO_PUBLIC_API_URL || "https://api.example.test";
// or
process.env.EXPO_PUBLIC_API_URL =
process.env.EXPO_PUBLIC_API_URL || "https://api.test.invalid";Note: Issue 5: Store tests don't verify persistence/hydration behaviorID: store-missing-persist-test-9d1e Tests verify store state management but don't test persistence mechanisms. The stores use zustand with persistence (likely MMKV), but tests don't verify that state properly persists and hydrates across store recreation. Recommendation: Add hydration tests: it("should persist and hydrate state across store instances", async () => {
// Set some state
useSettingsStore.getState().setThemeMode("dark");
useSettingsStore.getState().setMerchantId("merchant-123");
// Simulate app restart - get current persisted state
const persistedState = { ...useSettingsStore.getState() };
// Reset store and verify hydration
useSettingsStore.getState().setHasHydrated(false);
// Trigger hydration (implementation-specific)
// Then verify state matches
expect(useSettingsStore.getState().themeMode).toBe("dark");
expect(useSettingsStore.getState().merchantId).toBe("merchant-123");
});Issue 6: Missing edge case tests for payment amount handlingID: payment-amount-edge-cases-3c7b Payment service tests verify happy paths but don't test edge cases for amount handling: zero amounts, negative amounts, very large amounts, non-numeric values, or decimal precision issues. Recommendation: Add edge case tests: describe("payment amount edge cases", () => {
beforeEach(async () => {
await setupTestMerchant("merchant-123", "api-key-456");
});
it("should reject zero amount", async () => {
await expect(startPayment({
referenceId: "ref-123",
amount: { value: "0", unit: "cents" }
})).rejects.toThrow(/amount.*zero/i);
});
it("should reject negative amount", async () => {
await expect(startPayment({
referenceId: "ref-123",
amount: { value: "-100", unit: "cents" }
})).rejects.toThrow(/amount.*negative/i);
});
it("should handle maximum safe integer amount", async () => {
const maxSafe = Number.MAX_SAFE_INTEGER.toString();
// Test implementation
});
});Automated ChecksAll external URLs detected are test URLs (api.test.example.com, gateway.test.example.com) or documentation references (reactnative.dev, sentry.io, base64.guru, solscan.io, rpc.walletconnect.org). No unapproved external domain issues. SummaryTest implementation is comprehensive with good coverage of critical paths. Main concerns:
Recommended to address HIGH severity issues before merging, MEDIUM severity issues can be tracked for follow-up. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| expect(result.current.data).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("should start fetching when paymentId is provided and enabled", async () => { |
There was a problem hiding this comment.
🤖 Auto Review Issue: React Query hook tests don't verify polling/refetch behavior
Severity: MEDIUM
Category: test_coverage
Tool: Claude Auto Review
Recommendation: Add polling behavior tests:
it("should poll when status is not final and pollInMs is set", async () => {
let callCount = 0;
(getPaymentStatus as jest.Mock).mockImplementation(() => {
callCount++;
return Promise.resolve({
status: callCount < 3 ? "processing" : "succeeded",
isFinal: callCount >= 3,
pollInMs: callCount < 3 ? 1000 : 0,
});
});
const { result } = renderHook(() => usePaymentStatus("pay_123"), {
wrapper: createWrapper(),
});
// Wait for multiple polls
await waitFor(() => {
expect(callCount).toBeGreaterThanOrEqual(3);
}, { timeout: 10000 });
expect(result.current.data?.status).toBe("succeeded");
});| expect(useSettingsStore.getState().pinLockoutUntil).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🤖 Auto Review Issue: PIN verification tests don't validate timing attack resistance
Severity: HIGH
Category: security
Tool: Claude Auto Review
Recommendation: Ensure PIN verification uses constant-time comparison in the implementation:
// In useSettingsStore.ts verifyPin method
// Instead of direct string comparison:
// if (hashedPin === storedHash) { ... }
// Use constant-time comparison:
import { timingSafeEqual } from 'crypto';
const inputBuffer = Buffer.from(hashedPin, 'utf-8');
const storedBuffer = Buffer.from(storedHash, 'utf-8');
if (inputBuffer.length === storedBuffer.length &&
timingSafeEqual(inputBuffer, storedBuffer)) {
// PIN valid
}| } | ||
| }); | ||
|
|
||
| describe("initial state", () => { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Store tests don't verify persistence/hydration behavior
Severity: MEDIUM
Category: test_coverage
Tool: Claude Auto Review
Recommendation: Add hydration tests:
it("should persist and hydrate state across store instances", async () => {
// Set some state
useSettingsStore.getState().setThemeMode("dark");
useSettingsStore.getState().setMerchantId("merchant-123");
// Simulate app restart - get current persisted state
const persistedState = { ...useSettingsStore.getState() };
// Reset store and verify hydration
useSettingsStore.getState().setHasHydrated(false);
// Trigger hydration (implementation-specific)
// Then verify state matches
expect(useSettingsStore.getState().themeMode).toBe("dark");
expect(useSettingsStore.getState().merchantId).toBe("merchant-123");
});| }); | ||
| }); | ||
|
|
||
| describe("startPayment", () => { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Missing edge case tests for payment amount handling
Severity: MEDIUM
Category: test_coverage
Tool: Claude Auto Review
Recommendation: Add edge case tests:
describe("payment amount edge cases", () => {
beforeEach(async () => {
await setupTestMerchant("merchant-123", "api-key-456");
});
it("should reject zero amount", async () => {
await expect(startPayment({
referenceId: "ref-123",
amount: { value: "0", unit: "cents" }
})).rejects.toThrow(/amount.*zero/i);
});
it("should reject negative amount", async () => {
await expect(startPayment({
referenceId: "ref-123",
amount: { value: "-100", unit: "cents" }
})).rejects.toThrow(/amount.*negative/i);
});
it("should handle maximum safe integer amount", async () => {
const maxSafe = Number.MAX_SAFE_INTEGER.toString();
// Test implementation
});
});Issue 1: Add timeout configuration to API client - Added AbortController-based timeout (default 30s) to all HTTP requests - Added timeout error handling with TIMEOUT error code - Added tests for timeout behavior Issue 2: Add polling behavior tests for usePaymentStatus hook - Added tests verifying polling stops on terminal states (succeeded/failed) - Added test for onTerminalState callback during polling Issue 3: Add timing-safe PIN comparison (security) - Implemented timingSafeEqual() function using constant-time XOR comparison - Updated verifyPin() to use timing-safe comparison - Added test for timing-safe PIN verification Issue 5: Add store persistence/hydration tests - Added hydration state tracking tests for both stores - Added tests for state preservation through hydration - Added tests for PIN and biometric state persistence
|
@claude review |
|
Claude finished @ignaciosantise's task in 2m 18s —— View job 🔄 Incremental Review: Phase 2 Test ImplementationReviewed latest commits addressing previous review feedback. Todo:
Previously Reported Issues - Status Update✅ Resolved Issues1. client-missing-timeout-7f3a (HIGH) - API timeout configuration
2. hooks-missing-polling-test-8c4d (MEDIUM) - Missing React Query polling tests
3. settings-pin-timing-vuln-2a9f (HIGH) - PIN timing attack vulnerability
|
ignaciosantise
changed the title
| })); | ||
|
|
||
| describe("Payment Hooks", () => { | ||
| let queryClient: QueryClient; |
There was a problem hiding this comment.
🤖 Auto Review Issue: React Query cache not isolated between hook tests
Severity: MEDIUM
Category: test_quality
Tool: Claude Auto Review
Recommendation: Add explicit cleanup:
afterEach(async () => {
await clearTestMerchant();
if (queryClient) {
queryClient.clear();
await queryClient.cancelQueries(); // Add this
}
});| }); | ||
| }); | ||
|
|
||
| describe("persistence and hydration", () => { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Store hydration tests don't verify MMKV persistence layer
Severity: LOW
Category: test_coverage
Tool: Claude Auto Review
Recommendation: Test store recreation scenario:
it("should persist state across store recreation", async () => {
// Set state
useSettingsStore.getState().setMerchantId("merchant-test");
// Get underlying MMKV storage
const mmkvStorage = useSettingsStore.persist.getOptions().storage;
const persistedData = mmkvStorage.getString?.("settings");
// Reset store state (simulating app restart)
useSettingsStore.setState({ merchantId: null });
// Manually rehydrate from persisted data
// Verify merchantId restored to "merchant-test"
});| }); | ||
|
|
||
| describe("POST requests", () => { | ||
| it("should make a POST request with JSON body", async () => { |
There was a problem hiding this comment.
🤖 Auto Review Issue: Mock fetch doesn't validate request body structure
Severity: LOW
Category: test_quality
Tool: Claude Auto Review
Recommendation: Add body validation:
it("should serialize request body correctly", async () => {
const requestBody = { amount: "10.00", currency: "USD" };
(global.fetch as jest.Mock).mockImplementation((url, options) => {
const body = JSON.parse(options.body);
expect(body).toEqual(requestBody);
expect(body.amount).toBe("10.00"); // Verify string type preserved
return Promise.resolve({
ok: true,
status: 200,
json: jest.fn().mockResolvedValue({ paymentId: "pay_123" })
});
});
await apiClient.post("/payments", requestBody);
});Issue 1: React Query cache isolation - Added cancelQueries() to afterEach cleanup in hooks tests Issue 2: MMKV persistence layer verification - Added tests for persist configuration (name, version, storage) - Added tests for persist API availability (rehydrate, hasHydrated) Issue 3: Test environment variable security - Changed from fallback pattern to forced test-only URLs - Prevents accidental real endpoint calls during tests Issue 4: Request body validation - Added test verifying JSON serialization preserves types - Tests string, number, boolean, and nested object types
- Configure notifyManager to use synchronous scheduler in tests - Add removeQueries() to afterEach cleanup - Add staleTime: 0 to test QueryClient config
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 2 of the testing plan, adding 136 comprehensive tests for critical paths across utilities, stores, and services. The PR also introduces timing-safe PIN comparison for enhanced security and timeout handling for API requests.
Key Changes:
- Added 136 unit tests covering utilities (date formatting, device ID, error messages), stores (logs, settings with PIN/biometric management), and services (API client, payment operations, React Query hooks)
- Implemented timing-safe string comparison function to prevent timing attacks on PIN verification
- Added configurable timeout handling with AbortController for API requests
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
utils/payment-errors.test.ts |
Tests for payment error message mapping |
utils/misc.test.ts |
Tests for date formatting and device identifier utilities |
__tests__/store/useLogsStore.test.ts |
Tests for logs store including MAX_LOGS_COUNT limit behavior |
__tests__/store/useSettingsStore.test.ts |
Comprehensive tests for settings store covering theme, variant, merchant config, PIN verification, lockout, and biometrics |
__tests__/services/client.test.ts |
Tests for ApiClient HTTP methods, error handling, timeout, and logging |
__tests__/services/payment.test.ts |
Tests for payment service functions including API headers and payment operations |
__tests__/services/hooks.test.tsx |
Tests for React Query hooks including polling behavior and terminal states |
store/useSettingsStore.ts |
Added timing-safe comparison function for PIN verification security |
services/client.ts |
Added timeout support with AbortController for API requests |
app/payment-success.tsx |
Fixed lint issues (removed unused import, formatting) |
package.json |
Added --forceExit flag to test script for React Query cleanup |
jest.setup.js |
Configured test environment (disabled DEV, suppressed React Query warnings) |
.gitignore |
Added TESTING_PLAN.md to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe("timeout handling", () => { | ||
| it("should use AbortController signal for fetch", async () => { | ||
| (global.fetch as jest.Mock).mockResolvedValueOnce({ | ||
| ok: true, | ||
| status: 200, | ||
| json: jest.fn().mockResolvedValue({ success: true }), | ||
| }); | ||
|
|
||
| await apiClient.get("/test"); | ||
|
|
||
| // Verify fetch was called with an AbortSignal | ||
| expect(global.fetch).toHaveBeenCalledWith( | ||
| expect.any(String), | ||
| expect.objectContaining({ | ||
| signal: expect.any(AbortSignal), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The comment on lines 272-276 states "Verify fetch was called with an AbortSignal" but the test doesn't actually verify that the AbortSignal is functioning correctly - it only checks that an AbortSignal object was passed. The timeout functionality (setting up the timeout, clearing it properly) is not actually tested here.
Consider adding a more comprehensive test that actually triggers the timeout by making fetch delay longer than the timeout value, or update the comment to reflect that this test only verifies the signal is passed, not that the timeout mechanism works correctly.
| // Length check - but still perform full comparison to maintain constant time | ||
| const lengthMatch = a.length === b.length; | ||
|
|
||
| // Use the longer string length to ensure we always do the same amount of work | ||
| const maxLength = Math.max(a.length, b.length); | ||
|
|
||
| let result = 0; | ||
| for (let i = 0; i < maxLength; i++) { | ||
| // Use charCodeAt with fallback to 0 for out-of-bounds | ||
| const charA = i < a.length ? a.charCodeAt(i) : 0; | ||
| const charB = i < b.length ? b.charCodeAt(i) : 0; | ||
| // XOR accumulates any differences | ||
| result |= charA ^ charB; | ||
| } | ||
|
|
||
| // Both length must match AND all characters must be equal (result === 0) | ||
| return lengthMatch && result === 0; |
There was a problem hiding this comment.
The timing-safe comparison implementation has a security vulnerability. While the function attempts to maintain constant time by always iterating through maxLength, it still leaks information through the early length check assignment. The lengthMatch variable is computed before the constant-time comparison, and modern compilers or JIT optimizations could potentially reorder these operations. Additionally, using charCodeAt with fallback to 0 for out-of-bounds access still reveals length information through the actual character comparisons performed.
A more secure approach would be to ensure both strings are padded to the same length before comparison, or use a cryptographic library's timing-safe comparison function if available.
| // Length check - but still perform full comparison to maintain constant time | |
| const lengthMatch = a.length === b.length; | |
| // Use the longer string length to ensure we always do the same amount of work | |
| const maxLength = Math.max(a.length, b.length); | |
| let result = 0; | |
| for (let i = 0; i < maxLength; i++) { | |
| // Use charCodeAt with fallback to 0 for out-of-bounds | |
| const charA = i < a.length ? a.charCodeAt(i) : 0; | |
| const charB = i < b.length ? b.charCodeAt(i) : 0; | |
| // XOR accumulates any differences | |
| result |= charA ^ charB; | |
| } | |
| // Both length must match AND all characters must be equal (result === 0) | |
| return lengthMatch && result === 0; | |
| // Normalize both strings to the same length to avoid length-based branching | |
| const maxLength = Math.max(a.length, b.length); | |
| const paddedA = a.padEnd(maxLength, "\0"); | |
| const paddedB = b.padEnd(maxLength, "\0"); | |
| let result = 0; | |
| for (let i = 0; i < maxLength; i++) { | |
| // XOR accumulates any differences | |
| result |= paddedA.charCodeAt(i) ^ paddedB.charCodeAt(i); | |
| } | |
| // All characters must be equal (result === 0) for strings to be considered equal | |
| return result === 0; |
| callback(); | ||
| }); | ||
|
|
||
| // Suppress React Query's act() warnings in tests | ||
| // These warnings come from React Query's internal state management and are expected | ||
| const originalConsoleError = console.error; |
There was a problem hiding this comment.
Suppressing React Query's act() warnings by intercepting console.error is not recommended. These warnings exist to help identify improper async handling in tests. While React Query may generate some expected warnings, blindly suppressing them could hide real issues in component tests where state updates aren't properly wrapped in act().
A better approach would be to properly structure tests to avoid these warnings, or use more targeted suppression that only filters specific React Query warnings while allowing other important console.error messages through.
| callback(); | |
| }); | |
| // Suppress React Query's act() warnings in tests | |
| // These warnings come from React Query's internal state management and are expected | |
| const originalConsoleError = console.error; | |
| const [message] = args; | |
| const isActWarning = | |
| typeof message === "string" && message.includes("not wrapped in act(...)"); | |
| const isFromReactQuery = args.some( | |
| (arg) => | |
| typeof arg === "string" && | |
| (arg.includes("react-query") || arg.includes("@tanstack/react-query")) | |
| ); | |
| if (isActWarning && isFromReactQuery) { | |
| // Ignore known React Query act() warnings | |
| return; | |
| } |
|
|
||
| it("returns default message when called without arguments", () => { | ||
| const result = getPaymentErrorMessage(); | ||
| expect(result).toBe( | ||
| "The payment couldn't be completed due to an error. Please try again or use a different payment method.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The test on line 42-47 calls getPaymentErrorMessage() without any arguments, but the function signature shows the parameter as optional (errorStatus?: string). TypeScript should allow this, but it's testing the same code path as the "undefined" test on lines 35-40. This test is redundant and should be removed to reduce duplication and improve test clarity.
| it("returns default message when called without arguments", () => { | |
| const result = getPaymentErrorMessage(); | |
| expect(result).toBe( | |
| "The payment couldn't be completed due to an error. Please try again or use a different payment method.", | |
| ); | |
| }); |
Summary
This PR implements Phase 2 of the testing plan - Critical Path Testing, building on top of the Phase 1 testing infrastructure.
New Test Files Added (136 total tests)
utils/misc.test.tsgetDate(),getDeviceIdentifier()utils/payment-errors.test.tsgetPaymentErrorMessage()__tests__/store/useLogsStore.test.tsaddLog,clearLogs, MAX_LOGS_COUNT limit__tests__/store/useSettingsStore.test.ts__tests__/services/client.test.ts__tests__/services/payment.test.tsgetApiHeaders,startPayment,getPaymentStatus__tests__/services/hooks.test.tsxuseStartPayment,usePaymentStatusReact Query hooksAdditional Changes
app/payment-success.tsx(unused import, prettier formatting)--forceExitto test script to handle React Query open handlesact()warnings in jest.setup.js__DEV__ = falsein tests to prevent console.log noiseTest plan
npm test)npm run lint)