Refactor tests#60
Conversation
|
The coverage is around 80%, I think it's high enough to cover most operations, going for higher coverage will demand quite a bit of effort |
claudiovb
left a comment
There was a problem hiding this comment.
Good work on the test migration and new coverage.
Left some inline comments on specific files. A few additional notes:
Pre-existing (not introduced by this PR, but worth fixing since we're refactoring tests):
accountService.test.ts: The 'should throw error if HRPC not available' test (line 329) passes but for the wrong reason: the mock state is missing isWorkletStartedPromise and isWorkletInitializedPromise, so it hits a TypeError before reaching the hrpc check.
Fix:
mockWorkletStore.getState = jest.fn(() => ({
isInitialized: true,
hrpc: null,
isWorkletStartedPromise: { promise: Promise.resolve() },
isWorkletInitializedPromise: { promise: Promise.resolve() },
wdkConfigs: {},
}))
await expect(
AccountService.callAccountMethod('ethereum', 0, 'getBalance')
).rejects.toThrow('WDK not initialized or wdkConfigs not available')The try/catch in storeHelpers.ts masks it.
Same file: duplicate should throw error if result is null' test block (lines 351-369).
| const createWallet = useCallback( | ||
| async (walletId: string) => { | ||
| clearTemporaryWallet() | ||
| await WorkletLifecycleService.ensureWorkletStarted() |
There was a problem hiding this comment.
This ensureWorkletStarted() call appears redundant: WalletSetupService.createNewWallet already calls ensureWorkletStarted() as its first action, and checkWallet only touches secure storage so it doesn't need the worklet.
The subtle side effect is that the loading state transition is now delayed until the worklet is confirmed started, whereas before the UI would see loading immediately. This seems like a tiny UX downgrade, unless you have some other reason for adding this line?
Note that if you agree and we revert this, the "Worklet Lifecycle Integration" test (line 614 of the test file) asserts ensureWorkletStarted is called at the hook level, which would only pass with this new line, so that test would need to change also.
If you want to verify that createNewWallet gates on ensureWorkletStarted before calling initializeWDK, that test belongs in walletSetupService.test.ts where the real service code runs, I was thinking something like:
it('should ensure worklet is started before initializing WDK', async () => {
const callOrder: string[] = [];
(WorkletLifecycleService.ensureWorkletStarted as jest.Mock).mockImplementation(() => {
callOrder.push('ensureWorkletStarted');
return Promise.resolve();
});
(WorkletLifecycleService.initializeWDK as jest.Mock).mockImplementation(() => {
callOrder.push('initializeWDK');
return Promise.resolve();
});
await WalletSetupService.createNewWallet();
expect(callOrder).toEqual(['ensureWorkletStarted', 'initializeWDK']);
});Not blocking since double-awaiting is a no-op, but wanted to flag it.
| await store.getState().isWorkletInitializedPromise.promise | ||
| } catch (e) { | ||
| logWarn(e) | ||
| throw new Error('WDK not initialized') |
There was a problem hiding this comment.
The try/catch wrapping here makes sense for giving consumers a clean error message. One small suggestion: consider also passing the original error as cause so error monitoring tools can still access the root failure:
| throw new Error('WDK not initialized') | |
| throw new Error('WDK not initialized', { cause: e }) |
The logWarn covers developer visibility in console, but cause ensures consumer apps with error monitoring or programmatic error handling can still access the root failure.
| "sourceMap": true | ||
| }, | ||
| "include": ["src/**/*"], | ||
| "include": ["src/**/*", "tests/store", "tests/*", "tests/provider/WdkAppProvider.test.tsx", "tests/hooks/useWorklet.test.tsx", "tests/hooks/useWdkApp.test.tsx", "tests/hooks/useWalletManager.test.tsx", "tests/hooks/useBalance.test.tsx"], |
There was a problem hiding this comment.
The include in tsconfig.json enumerates specific test files, but many are missing (e.g., all of tests/utils/, tests/services/, tests/hooks/useAddressLoader.test.tsx, etc.). This means tsc, noEmit won't type-check most tests.
Switching to "tests//*" surfaces just one type error (in useAppLifecycle.test.tsx:90). Could we either fix the remaining type issues and simplify to "tests//*", or if there's any other reason I'm missing?
tsconfig.json:
| "include": ["src/**/*", "tests/store", "tests/*", "tests/provider/WdkAppProvider.test.tsx", "tests/hooks/useWorklet.test.tsx", "tests/hooks/useWdkApp.test.tsx", "tests/hooks/useWalletManager.test.tsx", "tests/hooks/useBalance.test.tsx"], | |
| "include": ["src/**/*", "tests/**/*"], |
Then you'll need to adapt useAppLifecycle.test.tsx:
javascript const removeFn = (AppState.addEventListener as jest.Mock).mock.results[0]?.value?.remove;
| @@ -79,10 +81,20 @@ | |||
| "globals": "^17.4.0", | |||
| "jest": "^29.7.0", | |||
| "jest-environment-node": "^29.7.0", | |||
There was a problem hiding this comment.
The new jest.config.cjs file uses react preset this could be removed
| "react": "18.3.1", | ||
| "react-native": "0.75.4", | ||
| "react-test-renderer": "18.3.1", | ||
| "ts-jest": "^29.1.0", |
|
|
||
| it('should handle address loading and errors correctly', async () => { | ||
| const addressErr = new Error('Failed to load addresses'); | ||
| mockUseMultiAddressLoader.mockReturnValue({ addresses: {}, isLoading: false, error: addressErr }); |
There was a problem hiding this comment.
| mockUseMultiAddressLoader.mockReturnValue({ addresses: {}, isLoading: false, error: addressErr }); | |
| mockUseMultiAddressLoader.mockReturnValue({ addresses: [], isLoading: false, error: addressErr }); | |
| }); | ||
|
|
||
| it('should call fetchBalances correctly on success', async () => { | ||
| mockUseMultiAddressLoader.mockReturnValue({ addresses: { 'ethereum-0': 'mock-eth-addr', 'polygon-0': 'mock-poly-addr' }, isLoading: false, error: null }); |
There was a problem hiding this comment.
| mockUseMultiAddressLoader.mockReturnValue({ addresses: { 'ethereum-0': 'mock-eth-addr', 'polygon-0': 'mock-poly-addr' }, isLoading: false, error: null }); | |
| mockUseMultiAddressLoader.mockReturnValue({ addresses: [{ network: 'ethereum', address: 'mock-eth-addr' }, { network: 'polygon', address: 'mock-poly-addr' }], isLoading: false, error: null }); | |
| const newState = updateWalletLoadingState(currentState, { type: 'ready', identifier: 'test' }) // Invalid: not_loaded -> ready | ||
|
|
||
| expect(newState).not.toBe(currentState) // Immer produce creates a new object | ||
| expect(newState.walletLoadingState).toEqual({ type: 'ready', identifier: 'test' }) // But the walletLoadingState within is unchanged |
There was a problem hiding this comment.
Super minor but this comment got me really confused when reading the test as it says "unchanged" but the state did change from not_loaded to ready, which seems the point of this test (invalid transitions are allowed in production). Maybe update the comment to clarify that.
| moduleNameMapper: { | ||
| '^@tetherto/wdk-react-native-secure-storage$': '<rootDir>/src/__mocks__/secureStorage.ts', | ||
| '^react$': '<rootDir>/src/__mocks__/react.ts', | ||
| '^@src/(.*)$': '<rootDir>/src/$1', |
There was a problem hiding this comment.
The @src path alias (line 20) isn't used anywhere in tests or source. Can be removed to keep config clean.
|
|
||
| describe('walletStore', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks() |
There was a problem hiding this comment.
createWalletStore() is a singleton but beforeEach only clears mocks, not store state. Tests that mutate state could leak into subsequent tests. Adding a state reset would prevent that:
| jest.clearAllMocks() | |
| jest.clearAllMocks() | |
| createWalletStore().setState(mockInitState, true) |
Longer term, a resetWalletStore() export similar to workletStore's resetWorkletStore() would be cleaner.
Description