Send tests #218
Send tests #218
Conversation
WalkthroughThis PR adds a unit test script, refactors helper functions in the Wallet Send form, introduces comprehensive test mocks including a mock service wallet, adds Vitest tests for the Send screen, and relaxes/assertion changes in transaction screen tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant SendForm as SendForm Component
participant Helpers as Consolidated Helpers
participant State as Component State
participant Mocks as Mock Contexts / svcWallet
Runner->>Mocks: Provide mocked contexts & mockSvcWallet
Runner->>SendForm: Render component
SendForm->>Helpers: invoke smartSetError / setState
Helpers->>State: update sendInfo, setScan(false), set error
SendForm-->>Runner: Render UI (Max, Amount, Recipient, Continue)
Runner->>Runner: Assert UI elements present / absent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
Deploying wallet-signet with
|
| Latest commit: |
0771343
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://33dd7b08.wallet-23u.pages.dev |
| Branch Preview URL: | https://send-tests.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
0771343
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://209105ef.arkade-wallet.pages.dev |
| Branch Preview URL: | https://send-tests.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
0771343
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://deb9a66b.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://send-tests.wallet-bitcoin.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/screens/mocks.ts (1)
87-137: Consider using deterministic identity for test stability.The
mockSvcWalletis comprehensive and well-structured. However, usingSingleKey.fromRandomBytes()on line 88 creates a new random identity on each test run, which could lead to non-deterministic test behavior if any test logic depends on the identity value.If deterministic behavior is desired, consider creating the identity from a fixed seed:
export const mockSvcWallet = { - identity: SingleKey.fromRandomBytes(), + identity: SingleKey.fromPrivateKey(new Uint8Array(32).fill(1)), // deterministic test identity getAddress: () => '',Alternatively, if the current approach is intentional for test isolation, consider adding a comment explaining the rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(1 hunks)src/screens/Wallet/Send/Form.tsx(1 hunks)src/test/screens/mocks.ts(2 hunks)src/test/screens/wallet/send.test.tsx(1 hunks)src/test/screens/wallet/transaction.test.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/test/screens/wallet/send.test.tsxsrc/test/screens/mocks.tssrc/test/screens/wallet/transaction.test.tsx
🧬 Code graph analysis (2)
src/test/screens/wallet/send.test.tsx (7)
src/providers/navigation.tsx (1)
NavigationContext(164-168)src/test/screens/mocks.ts (6)
mockNavigationContextValue(35-39)mockAspContextValue(28-33)mockFlowContextValue(59-72)mockWalletContextValue(41-57)mockLimitsContextValue(74-85)mockSvcWallet(87-137)src/providers/asp.tsx (1)
AspContext(15-20)src/providers/flow.tsx (1)
FlowContext(82-95)src/providers/wallet.tsx (1)
WalletContext(40-54)src/providers/limits.tsx (1)
LimitsContext(28-39)src/screens/Wallet/Send/Form.tsx (1)
SendForm(43-399)
src/screens/Wallet/Send/Form.tsx (1)
src/providers/flow.tsx (1)
SendInfo(24-36)
🔇 Additional comments (6)
package.json (1)
35-35: LGTM!The new unit test script is well-structured and follows the existing pattern. The exclude pattern correctly filters out e2e tests.
src/screens/Wallet/Send/Form.tsx (1)
70-77: LGTM! Centralized helper functions improve maintainability.Moving these helper functions to a consistent location near the top of the component body improves code organization and eliminates duplication. The
smartSetErrorfunction appropriately centralizes the logic for handling Ark server unreachability, andsetStateprovides a clean wrapper for common state updates.src/test/screens/wallet/transaction.test.tsx (4)
84-85: Correct fix: UsequeryByTextfor negative assertions.This change correctly uses
queryByTextinstead offindByTextfor checking element absence. ThefindByTextmethod returns a promise and will throw an error if the element is not found, making it unsuitable for negative assertions. ThequeryByTextmethod returnsnullsynchronously when an element is not found, which is the appropriate choice for.not.toBeInTheDocument()assertions.
154-155: Correct fix: UsequeryByTextfor negative assertions.Same improvement as in the previous test case - correctly using
queryByTextfor checking element absence.
189-190: Correct fix: UsequeryByTextfor negative assertions.Same improvement as in the previous test cases - correctly using
queryByTextfor checking element absence.
226-227: Correct fix: UsequeryByTextfor negative assertions.Same improvement as in the previous test cases - correctly using
queryByTextfor checking element absence.
| <NavigationContext.Provider value={mockNavigationContextValue}> | ||
| <AspContext.Provider value={mockAspContextValue}> | ||
| <FlowContext.Provider value={mockFlowContextValue}> | ||
| <WalletContext.Provider value={{ ...mockWalletContextValue, svcWallet: mockSvcWallet as any }}> |
There was a problem hiding this comment.
Avoid type assertion with as any.
The as any cast bypasses type checking and could hide type mismatches between mockSvcWallet and the expected svcWallet type. This defeats the purpose of TypeScript's type safety.
Consider defining a proper type for mockSvcWallet or use a more specific type assertion. You can check the actual type expected by WalletContext and ensure mockSvcWallet conforms to it:
-<WalletContext.Provider value={{ ...mockWalletContextValue, svcWallet: mockSvcWallet as any }}>
+<WalletContext.Provider value={{ ...mockWalletContextValue, svcWallet: mockSvcWallet }}>If there are type mismatches, update mockSvcWallet in mocks.ts to properly implement the required interface rather than using as any.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/screens/wallet/send.test.tsx around line 41, replace the unsafe `as
any` cast on mockSvcWallet by ensuring mockSvcWallet is typed to the exact
svcWallet type expected by WalletContext: import (or reference) the
WalletContext svcWallet interface/type, update the mock implementation in
mocks.ts to include all required properties/methods so it conforms to that
interface, and use that concrete type (or a narrow type assertion only if
unavoidable) when providing value to WalletContext.Provider so TypeScript
enforces compatibility rather than bypassing checks.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/screens/mocks.ts(3 hunks)src/test/screens/wallet/send.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/screens/wallet/send.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/test/screens/mocks.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
src/test/screens/mocks.ts (2)
35-53: LGTM! Mock context values are well-structured.The new mock context exports provide appropriate default values for testing scenarios.
143-156: Verify thatundefinedproperties are intentional.Several properties are set to
undefined(sendMessage,clear,getStatus,reload). If tests attempt to call these as functions, they will fail with "undefined is not a function" errors.Please confirm whether:
- These properties are intentionally undefined because they're unused in the tests
- They should be stubbed as no-op functions:
() => {}or() => Promise.resolve()If they should be stubbed, apply this diff:
- sendMessage: undefined, + sendMessage: () => {},- clear: undefined, - getStatus: undefined, - reload: undefined, + clear: () => Promise.resolve(), + getStatus: () => Promise.resolve(undefined), + reload: () => Promise.resolve(),
| } | ||
|
|
||
| export const mockSvcWallet = { | ||
| identity: SingleKey.fromRandomBytes(), |
There was a problem hiding this comment.
Non-deterministic mock identity may cause test flakiness.
Using SingleKey.fromRandomBytes() creates a different identity on each test run, which can lead to non-deterministic test behavior if tests depend on consistent identity values.
Consider using a deterministic seed for reproducible tests:
- identity: SingleKey.fromRandomBytes(),
+ identity: SingleKey.fromSeed(new Uint8Array(32).fill(1)), // deterministic seedAlternatively, if randomness is acceptable for your test scenarios, you can ignore this suggestion.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/screens/mocks.ts around line 108, the mock identity is created with
SingleKey.fromRandomBytes() which yields a non-deterministic value; replace it
with a deterministic identity (for example use SingleKey.fromSeed() or
fromBytes() with a fixed seed/byte array or a constant test key) so tests are
reproducible, or explicitly document/feature-flag the randomness if you intend
to keep it.
Summary by CodeRabbit
Tests
Chores
Refactor