Improve settings test coverage for tauri-app #1730
Conversation
|
""" WalkthroughThe previous test suite for settings-related stores was removed and replaced with a new comprehensive test file. The new tests cover synchronization and reset behaviors of multiple reactive stores related to settings, including accounts, networks, orderbooks, subgraphs, and derived store outputs. Edge cases and utility functions are also tested. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant SettingsStore
participant DerivedStores
participant UtilityFunctions
Test->>SettingsStore: Update/reset settings (accounts, networks, orderbooks, subgraphs)
SettingsStore-->>DerivedStores: Propagate changes to activeAccountsItems, activeSubgraphs, activeNetworkRef, activeOrderbookRef
Test->>UtilityFunctions: Call resetActiveNetworkRef/resetActiveOrderbookRef
UtilityFunctions-->>SettingsStore: Update active references based on available data
Test->>DerivedStores: Check outputs (subgraphUrl, hasRequiredSettings, filtered orderbooks, accounts)
Test->>SettingsStore: Simulate edge cases (invalid JSON, extra accounts)
SettingsStore-->>DerivedStores: Cleanup and state consistency
Possibly related issues
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@hardingjam I think we should move the tests from settings into it's own file. This is an issue I've fixed today with this PR #1749 I was getting blank page in tauri app becase vitest dependency is used in the prod build and that was causing problems. Moving the tests to a dedicated file fixed the issue so I think we can do the same here |
Updated the PR with your separate test file |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tauri-app/src/lib/test/settings.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tauri-app/src/lib/test/settings.test.ts (1)
tauri-app/src/lib/stores/settings.ts (15)
settings(26-37)activeAccountsItems(101-112)activeSubgraphs(127-138)activeNetworkRef(43-43)activeOrderbookRef(65-65)resetActiveNetworkRef(231-239)activeNetworkOrderbooks(66-75)resetActiveOrderbookRef(219-228)hasRequiredSettings(93-97)subgraphUrl(83-87)accounts(100-100)activeAccounts(113-121)activeOrderStatus(241-253)hideZeroBalanceVaults(255-266)orderHash(268-273)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (2)
tauri-app/src/lib/test/settings.test.ts (2)
364-380:cachedWritableStoretoucheslocalStorage– use a stub in pure-node runsVitest’s default “node” environment has no
localStorage, so this test will throw unless:
- the project forces
jsdom, orcachedWritableStorefalls back internally.If condition 1 is not guaranteed, explicitly stub:
import { beforeAll } from 'vitest'; beforeAll(() => { globalThis.localStorage ??= new (require('node-localstorage').LocalStorage)('./.tmp'); });(or mock the API).
Prevents brittle CI failures when env changes.
400-423: 👍 Robust invalid-JSON parsing testNicely isolates the scenario with a dedicated key and custom (de)serializer; verifies graceful fallback.
| beforeEach(() => { | ||
| // Reset all store values | ||
| settings.set(undefined); | ||
| activeAccountsItems.set({}); | ||
| activeSubgraphs.set({}); | ||
|
|
||
| // Then set our initial test values | ||
| settings.set(mockConfigSource); | ||
| activeAccountsItems.set({ | ||
| name_one: 'address_one', | ||
| name_two: 'address_two', | ||
| }); | ||
| activeSubgraphs.set({ | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet', | ||
| }); | ||
|
|
||
| // Verify initial state | ||
| expect(get(settings)).toEqual(mockConfigSource); | ||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| name_two: 'address_two', | ||
| }); | ||
| expect(get(activeSubgraphs)).toEqual({ | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet', | ||
| }); | ||
| }); | ||
|
|
||
| test('should remove account if that account is removed', () => { | ||
| // Test removing an account | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| }, | ||
| }; | ||
|
|
||
| // Update settings - this should trigger the subscription | ||
| settings.set(newSettings); | ||
|
|
||
| // Check the expected result | ||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| }); | ||
| }); | ||
|
|
||
| test('should remove account if the value is different', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| name_two: 'new_value', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| }); | ||
| }); | ||
|
|
||
| test('should update active subgraphs when subgraph value changes', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| subgraphs: { | ||
| mainnet: 'new value', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeSubgraphs)).toEqual({}); | ||
| }); | ||
|
|
||
| test('should update active subgraphs when subgraph removed', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| subgraphs: { | ||
| testnet: 'testnet', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeSubgraphs)).toEqual({}); | ||
| }); | ||
|
|
||
| test('should reset active subgraphs when subgraphs are undefined', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| subgraphs: undefined, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeSubgraphs)).toEqual({}); | ||
| }); | ||
| }); | ||
| const { test, expect } = import.meta.vitest; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Conditional import is safe but duplicate destructuring guard can simplify
const { test, expect } = import.meta.vitest; is executed only when the block runs, so it is safe.
However, a shorter pattern is:
const { test, expect } = import.meta.vitest!;…which avoids the extra if indent while keeping type-safety.
Optional refactor only.
| // Reset store values before each test to prevent state leakage | ||
| beforeEach(() => { | ||
| settings.set(undefined); | ||
| activeAccountsItems.set({}); | ||
| activeSubgraphs.set({}); | ||
| activeNetworkRef.set(undefined); | ||
| activeOrderbookRef.set(undefined); | ||
|
|
||
| settings.set(mockConfigSource); | ||
| activeAccountsItems.set({ | ||
| name_one: 'address_one', | ||
| name_two: 'address_two' | ||
| }); | ||
| activeSubgraphs.set({ | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet' | ||
| }); | ||
|
|
||
| // Verify initial state | ||
| expect(get(settings)).toEqual(mockConfigSource); | ||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| name_two: 'address_two' | ||
| }); | ||
| expect(get(activeSubgraphs)).toEqual({ | ||
| mainnet: 'https://api.thegraph.com/subgraphs/name/mainnet' | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Repeated store-reset boilerplate – extract helper for clarity
Nearly identical beforeEach blocks reset the same set of stores across multiple describes.
Extracting a small utility avoids drift between suites and shortens tests:
function resetAllStores() {
settings.set(undefined);
activeAccountsItems.set({});
activeSubgraphs.set({});
activeNetworkRef.set(undefined);
activeOrderbookRef.set(undefined);
}Then call beforeEach(resetAllStores) everywhere.
| import { | ||
| settings, | ||
| activeAccountsItems, | ||
| activeSubgraphs, | ||
| activeNetworkRef, | ||
| activeOrderbookRef, | ||
| resetActiveNetworkRef, | ||
| resetActiveOrderbookRef, | ||
| activeNetworkOrderbooks, | ||
| hasRequiredSettings, | ||
| subgraphUrl, | ||
| accounts, | ||
| activeAccounts, | ||
| activeOrderStatus, | ||
| hideZeroBalanceVaults, | ||
| orderHash | ||
| } from '$lib/stores/settings'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛑 Tests live inside the production source tree – please move them out of src/
Placing *.test.ts files under src/lib means Vite / Tauri will still discover and transpile them for the production bundle.
A very similar arrangement already caused a blank white page in PR #1749 when the Vitest runtime crept into the app.
Even though the test body is wrapped in if (import.meta.vitest), the file itself (and therefore its dependencies) is still part of the module graph.
Action:
-tauri-app/src/lib/test/settings.test.ts
+tauri-app/tests/settings.test.ts # or e2e/, __tests__/, etc.and ensure vitest.config.ts includes the new folder.
This keeps the production bundle clean and avoids accidental leakage of test-only deps.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { | |
| settings, | |
| activeAccountsItems, | |
| activeSubgraphs, | |
| activeNetworkRef, | |
| activeOrderbookRef, | |
| resetActiveNetworkRef, | |
| resetActiveOrderbookRef, | |
| activeNetworkOrderbooks, | |
| hasRequiredSettings, | |
| subgraphUrl, | |
| accounts, | |
| activeAccounts, | |
| activeOrderStatus, | |
| hideZeroBalanceVaults, | |
| orderHash | |
| } from '$lib/stores/settings'; | |
| - tauri-app/src/lib/test/settings.test.ts | |
| + tauri-app/tests/settings.test.ts |
| test('resetActiveNetworkRef should set first available network', async () => { | ||
| settings.set(mockConfigSource); | ||
|
|
||
| await resetActiveNetworkRef(); | ||
|
|
||
| expect(get(activeNetworkRef)).toBe('mainnet'); | ||
| }); | ||
|
|
||
| test('resetActiveNetworkRef should set undefined when no networks', async () => { | ||
| const emptySettings = { ...mockConfigSource, networks: {} }; | ||
| settings.set(emptySettings); | ||
|
|
||
| await resetActiveNetworkRef(); | ||
|
|
||
| expect(get(activeNetworkRef)).toBeUndefined(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
awaiting a synchronous function needlessly slows the suite
resetActiveNetworkRef() currently performs only synchronous store.set() calls (no awaited I/O).
Removing async/await:
-test('resetActiveNetworkRef should set first available network', async () => {
- settings.set(mockConfigSource);
- await resetActiveNetworkRef();
+test('resetActiveNetworkRef should set first available network', () => {
+ settings.set(mockConfigSource);
+ resetActiveNetworkRef();…makes intent clearer and shaves a tick off each test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('resetActiveNetworkRef should set first available network', async () => { | |
| settings.set(mockConfigSource); | |
| await resetActiveNetworkRef(); | |
| expect(get(activeNetworkRef)).toBe('mainnet'); | |
| }); | |
| test('resetActiveNetworkRef should set undefined when no networks', async () => { | |
| const emptySettings = { ...mockConfigSource, networks: {} }; | |
| settings.set(emptySettings); | |
| await resetActiveNetworkRef(); | |
| expect(get(activeNetworkRef)).toBeUndefined(); | |
| test('resetActiveNetworkRef should set first available network', () => { | |
| settings.set(mockConfigSource); | |
| resetActiveNetworkRef(); | |
| expect(get(activeNetworkRef)).toBe('mainnet'); | |
| }); |
There was a problem hiding this comment.
@hardingjam Thank you for implementing the change! Removing the unnecessary async/await will help make the tests more efficient and clearer. Good work!
🐰✨
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tauri-app/src/lib/test/settings.test.ts (2)
19-26: Tests still live undersrc/– move them out of the production bundleKeeping
*.test.tsinsidetauri-app/srcmeans Vite / Tauri will include the file (and its vitest dependency) in the production graph.
PR #1749 showed this leads to a blank page at runtime.Please relocate to something like
tauri-app/tests/settings.test.tsand updatevitest.config.tsinclude paths.
29-35: Extract a shared “resetStores” helper to avoid duplicated boilerplateThe
beforeEachblocks across suites manually reset the same set of stores.
Centralising this logic will keep future additions in sync and make the tests shorter:function resetAllStores() { settings.set(undefined); activeNetworkRef.set(undefined); activeOrderbookRef.set(undefined); activeAccountsItems.set({}); activeSubgraphs.set({}); } beforeEach(resetAllStores);Also applies to: 140-147, 300-304
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tauri-app/src/lib/test/settings.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
tauri-app/src/lib/test/settings.test.ts (1)
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1749
File: tauri-app/src/lib/test/settings.test.ts:8-111
Timestamp: 2025-05-07T06:58:32.478Z
Learning: In the rain.orderbook project, stores like `activeAccountsItems` and `activeSubgraphs` are derived stores that should not be directly updated, as their values are derived from the `settings` store.
🧬 Code Graph Analysis (1)
tauri-app/src/lib/test/settings.test.ts (3)
tauri-app/src/lib/stores/settings.ts (11)
settings(26-37)activeAccountsItems(101-112)activeNetworkRef(43-43)activeOrderbookRef(65-65)resetActiveNetworkRef(231-239)activeNetworkOrderbooks(66-75)resetActiveOrderbookRef(219-228)hasRequiredSettings(93-97)subgraphUrl(83-87)accounts(100-100)activeAccounts(113-121)tauri-app/src/lib/__tests__/matchers.ts (1)
expect(5-5)packages/ui-components/src/lib/index.ts (1)
ConfigSource(71-71)
🪛 ESLint
tauri-app/src/lib/test/settings.test.ts
[error] 14-14: 'activeOrderStatus' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 15-15: 'hideZeroBalanceVaults' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 16-16: 'orderHash' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: Deploy-Preview
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: git-clean
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tauri-app/src/__tests__/settings.test.ts(1 hunks)tauri-app/src/lib/test/settings.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- tauri-app/src/lib/test/settings.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: Deploy-Preview
- GitHub Check: git-clean
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
🔇 Additional comments (12)
tauri-app/src/__tests__/settings.test.ts (12)
1-20: Well-structured imports and dependenciesThe import section is clean and properly organized, importing all necessary stores and testing utilities.
21-48: Good test setup with proper isolationThe beforeEach hook correctly resets all stores and initializes them with mock data, ensuring proper test isolation. The initial state verification is a good practice to confirm the test setup worked as expected.
50-66: Test covers account removal behaviorThis test correctly verifies that accounts are removed from activeAccountsItems when they are removed from the settings store.
84-130: Good coverage of edge cases for account and subgraph storesThese tests thoroughly cover important scenarios including undefined accounts, subgraph value changes, subgraph removal, and undefined subgraphs.
133-142: Proper store reset in beforeEachThe beforeEach hook correctly resets all stores to ensure test isolation.
200-226: Good test for network-specific orderbook filteringThis test correctly sets up multiple orderbooks on different networks and verifies that only orderbooks for the active network are included in the filtered results.
228-252: Well-structured test for complex interactionThis test effectively verifies the important behavior of resetting the orderbook when switching to a network that doesn't support the current orderbook.
278-291: Good coverage of hasRequiredSettings storeThe tests verify both positive and negative cases for the hasRequiredSettings derived store.
293-344: Comprehensive testing of derived storesThis suite thoroughly tests the behavior of derived stores (subgraphUrl, accounts, activeAccounts) under various conditions, including edge cases like empty or undefined inputs.
347-370: Excellent testing of JSON parse error handlingThis test creates a local store with intentionally invalid JSON serialization to test the error handling in the cachedWritableStore. This is a valuable test that verifies the robustness of the storage mechanism.
372-388: Good edge case testing for account synchronizationThis test verifies that accounts in activeAccountsItems that don't exist in settings.accounts are properly removed when the settings store changes. This is an important edge case to prevent stale data.
1-389: Well-organized and comprehensive test coverageThe test file provides excellent coverage of settings-related functionality, with proper test isolation through beforeEach hooks and comprehensive testing of edge cases. The organization into logical test suites makes the code easy to understand and maintain.
Moving these tests to a dedicated tests directory (as mentioned in the PR comments) is a good practice that aligns with the recommendation to separate test files from production code to prevent issues with the vitest dependency being included in production builds.
| test('resetActiveNetworkRef should set first available network', async () => { | ||
| settings.set(mockConfigSource); | ||
|
|
||
| resetActiveNetworkRef(); | ||
|
|
||
| expect(get(activeNetworkRef)).toBe('mainnet'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider combining async/await usage with expectations
These tests are marked as async but don't use await within the test body. Either remove the async keyword or ensure there's a reason for the asynchronous test.
- test('resetActiveNetworkRef should set first available network', async () => {
+ test('resetActiveNetworkRef should set first available network', () => {- test('resetActiveNetworkRef should set undefined when no networks', async () => {
+ test('resetActiveNetworkRef should set undefined when no networks', () => {Also applies to: 177-184
| test('should remove account if the value is different', () => { | ||
| const newSettings = { | ||
| ...mockConfigSource, | ||
| accounts: { | ||
| name_one: 'address_one', | ||
| name_two: 'new_value', | ||
| }, | ||
| }; | ||
|
|
||
| settings.set(newSettings); | ||
|
|
||
| expect(get(activeAccountsItems)).toEqual({ | ||
| name_one: 'address_one', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider more descriptive test name
While the test logic is correct, the test name "should remove account if the value is different" could be more specific about what "different" means in this context. Consider renaming to "should remove account if the address value changes in settings".
- test('should remove account if the value is different', () => {
+ test('should remove account if the address value changes in settings', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should remove account if the value is different', () => { | |
| const newSettings = { | |
| ...mockConfigSource, | |
| accounts: { | |
| name_one: 'address_one', | |
| name_two: 'new_value', | |
| }, | |
| }; | |
| settings.set(newSettings); | |
| expect(get(activeAccountsItems)).toEqual({ | |
| name_one: 'address_one', | |
| }); | |
| }); | |
| test('should remove account if the address value changes in settings', () => { | |
| const newSettings = { | |
| ...mockConfigSource, | |
| accounts: { | |
| name_one: 'address_one', | |
| name_two: 'new_value', | |
| }, | |
| }; | |
| settings.set(newSettings); | |
| expect(get(activeAccountsItems)).toEqual({ | |
| name_one: 'address_one', | |
| }); | |
| }); |
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit