test: create tests for stores #1338
Conversation
WalkthroughThe changes reorganize and streamline import statements and test setups across components, stores, and mocks. In components such as DownloadApiLogs and KeyActions, new import statements (like Vue’s computed) have been added and reorganized. Various test files for Auth, DownloadApiLogs, KeyActions, and the server store have been enhanced with clearer logic and additional interfaces. Outdated mocks and utility files have been removed or updated, and the test configuration has been consolidated to use JavaScript files instead of TypeScript mocks. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/test/mocks/ui-components.ts (1)
3-39: Enhanced UI component mocks, but type definitions need improvementThe mock implementation for UI components is much more comprehensive now, which should improve test accuracy. The
MockBrandButtonwith complete props support and themockCnutility function are excellent additions.However, there are two instances of
anytype that should be addressed to align with TypeScript best practices.-const mockCn = (...args: any[]) => args.filter(Boolean).join(' '); +const mockCn = (...args: (string | boolean | undefined | null)[]) => args.filter(Boolean).join(' '); // ... - setup(props: any) { + setup(props: Record<string, unknown>) {🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 4-4:
Unexpected any. Specify a different type
[failure] 30-30:
Unexpected any. Specify a different typeweb/test/components/Auth.test.ts (1)
44-46: Add proper typing to the mock implementationThe
storeToRefsmock usesanytype for the store parameter, which triggered a static analysis warning.vi.mock('pinia', () => ({ - storeToRefs: (store: any) => store, + storeToRefs: (store: Record<string, unknown>) => store, }));🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 45-45:
Unexpected any. Specify a different type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
web/components/DownloadApiLogs.ce.vue(1 hunks)web/components/KeyActions.vue(1 hunks)web/test/components/Auth.test.ts(1 hunks)web/test/components/DownloadApiLogs.test.ts(1 hunks)web/test/components/KeyActions.test.ts(1 hunks)web/test/mocks/pinia.ts(0 hunks)web/test/mocks/ui-components.ts(1 hunks)web/test/mocks/ui-libraries.ts(0 hunks)web/test/mocks/utils/README.md(0 hunks)web/test/mocks/utils/component-mock.ts(0 hunks)web/test/mocks/utils/examples/KeyActions.test.example.ts(0 hunks)web/test/mocks/utils/examples/key-actions-mock.ts(0 hunks)web/test/mocks/vue-i18n.ts(0 hunks)web/test/mocks/vue.ts(0 hunks)web/test/setup.ts(0 hunks)
💤 Files with no reviewable changes (9)
- web/test/setup.ts
- web/test/mocks/pinia.ts
- web/test/mocks/utils/README.md
- web/test/mocks/vue-i18n.ts
- web/test/mocks/ui-libraries.ts
- web/test/mocks/utils/examples/key-actions-mock.ts
- web/test/mocks/vue.ts
- web/test/mocks/utils/component-mock.ts
- web/test/mocks/utils/examples/KeyActions.test.example.ts
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/test/mocks/ui-components.ts
[failure] 4-4:
Unexpected any. Specify a different type
[failure] 30-30:
Unexpected any. Specify a different type
web/test/components/Auth.test.ts
[failure] 45-45:
Unexpected any. Specify a different type
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (29)
web/components/KeyActions.vue (1)
2-2: LGTM - Good import addition!Adding the
computedimport is necessary as it's being used in the component (lines 31 and 35). This change properly aligns with Vue 3's Composition API practices.web/components/DownloadApiLogs.ce.vue (1)
2-4: LGTM - Import organization improved!The added imports are correctly organized and necessary for the component:
computedis used on line 11 for creating thedownloadUrlcomputed propertyuseI18nis used on line 9 to get the translation functionThis follows good practices of importing Vue core utilities first, then third-party libraries.
web/test/components/Auth.test.ts (7)
11-24: LGTM - Well-defined interfaces for mocksAdding explicit interfaces for
AuthActionandStateDatamakes the test more maintainable and type-safe. This is a great practice that helps document the expected structure of these objects.
34-37: LGTM - Clean mock store implementationThe mock store using Vue refs properly simulates reactive store properties. This approach allows for better testing of component reactivity when store values change.
39-41: LGTM - Simplified store mockReturning the predefined mockServerStore object is cleaner than the previous implementation.
49-67: LGTM - Improved test clarity and assertionThe test now properly verifies both the existence of the button and its props, making it more comprehensive than before.
69-90: LGTM - Enhanced error state testingGood improvement on testing the error state, including verification of both heading and message content.
92-112: LGTM - More explicit click handler testThe click handler test is now more explicit about what's being tested and has clear stages (setup, action, verification).
114-126: LGTM - Added coverage for undefined authActionThis new test case improves coverage by testing the component behavior when authAction is undefined.
web/test/components/DownloadApiLogs.test.ts (8)
9-10: Appropriate imports for testingGood job adding these specific imports from @unraid/ui and vitest. This makes it clear what exact components and testing utilities are being used.
12-12: Direct component import improves test clarityUsing the actual component instead of a mock implementation creates more reliable tests that validate the real component behavior.
14-20: Well-structured URL mockingGood approach using predictable mock URLs that are consistent throughout the test. This makes the tests more deterministic and easier to maintain.
22-27: Clean i18n mockingSimple and effective approach to mocking the translation function. This isolates the component behavior from the translation implementation details.
29-34: Good test setupThe beforeEach setup properly clears mocks and sets up the CSRF token, ensuring tests start with a clean state.
36-59: Comprehensive download button testThis test thoroughly validates the download button's functionality, including URL construction, attributes, and content.
61-90: Complete support links validationGood test coverage checking all support links have the correct URLs, text content, and proper attributes for external links.
92-110: Verify instructional text presenceThis test ensures the important guidance text is present, which is critical for user understanding.
web/test/components/KeyActions.test.ts (12)
1-13: Clean imports and component usageGood job streamlining the imports and directly using the actual KeyActions component instead of mocks. This creates more accurate and maintainable tests.
14-18: Well-defined mock store actionsCreating these mock actions with spy functions for click handlers makes it easy to test interactions across all test cases.
20-29: Appropriate store mockingGood approach mocking both Pinia and the server store to isolate component testing from actual store implementations.
39-49: Verify store action renderingThis test correctly verifies that the component renders buttons from the store when no actions are provided via props.
51-67: Verify prop action renderingGood test ensuring that actions provided as props take precedence over store actions.
69-79: Empty actions handling testImportant edge case to test - ensures the component handles empty action arrays gracefully by rendering an empty list container.
81-97: Action click handler testGreat test verifying that click handlers are properly triggered when buttons are clicked.
99-120: Disabled button behavior testExcellent test ensuring disabled buttons don't trigger their click handlers, which prevents unintended actions.
122-142: Action filtering testGood verification of the filterBy prop functionality, ensuring only the specified actions are displayed.
144-164: Action exclusion filtering testGreat complement to the filterBy test, validating that the filterOut prop correctly excludes specified actions.
166-182: Styling prop verificationGood test ensuring that the maxWidth prop applies the correct styling class to buttons.
184-213: Comprehensive props passing testExcellent test verifying that all props are correctly passed to the BrandButton component, including action properties and translation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/test/mocks/ui-components.ts (2)
4-4: Specify a more precise type instead of 'any'.Replace
any[]with a more specific type to address the static analysis warning.-const mockCn = (...args: any[]) => args.filter(Boolean).join(' '); +const mockCn = (...args: (string | number | boolean | null | undefined)[]) => args.filter(Boolean).join(' ');🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 4-4:
Unexpected any. Specify a different type
20-28: Fix HTML attributes in button template.The current template applies HTML attributes that aren't standard for button elements.
template: ` - <button - :class="props.class" - :disabled="props.disabled" - :href="props.href" - :external="props.external" - :title="props.title" - > - {{ props.text || '' }} <slot /> - </button> + <div> + <button + v-if="!props.href" + :class="props.class" + :disabled="props.disabled" + :title="props.title" + > + {{ props.text || '' }} <slot /> + </button> + <a + v-else + :class="props.class" + :href="props.href" + :target="props.external ? '_blank' : undefined" + :rel="props.external ? 'noopener noreferrer' : undefined" + :title="props.title" + > + {{ props.text || '' }} <slot /> + </a> + </div> + `,web/test/components/DownloadApiLogs.test.ts (1)
34-42: Reduce test setup duplication.The same component setup is repeated in multiple tests.
Consider refactoring to use a factory function or beforeEach:
let wrapper; beforeEach(() => { wrapper = mount(DownloadApiLogs, { global: { stubs: { ArrowDownTrayIcon: true, ArrowTopRightOnSquareIcon: true, }, }, }); });Also applies to: 59-67, 90-98
web/test/components/Auth.test.ts (1)
109-109: Consider using trigger('click') instead of vm.$emit.For more accurate simulation of user interaction:
-await wrapper.findComponent({ name: 'BrandButton' }).vm.$emit('click'); +await wrapper.findComponent({ name: 'BrandButton' }).trigger('click');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
web/test/components/Auth.test.ts(1 hunks)web/test/components/DownloadApiLogs.test.ts(1 hunks)web/test/components/KeyActions.test.ts(1 hunks)web/test/mocks/ui-components.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/test/mocks/ui-components.ts
[failure] 4-4:
Unexpected any. Specify a different type
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
web/test/components/DownloadApiLogs.test.ts (2)
44-56: Good use of URL verification.The test effectively checks that the download button has the correct URL with CSRF token.
84-87: Good accessibility check for external links.Checking for both target="_blank" and proper security attributes is a best practice.
web/test/components/Auth.test.ts (2)
13-25: Good type definitions for mocks.Well-defined interfaces improve code readability and type safety.
115-127: Good defensive testing for undefined state.Testing that UI elements don't render when data is undefined prevents unexpected UI behaviors.
web/test/components/KeyActions.test.ts (3)
85-101: Good click handler test.Proper verification that click handlers are being called when buttons are clicked.
103-124: Good disabled button test.Testing that disabled buttons don't trigger click handlers prevents unexpected behaviors.
188-217: Comprehensive props testing.The test thoroughly verifies all props are correctly passed to the BrandButton component.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.cursor/rules/web-testing-rules.mdc (1)
119-120:⚠️ Potential issueContent appears to be cut off.
The section on testing state and getters seems incomplete, ending abruptly at line 120 without proper closure.
Consider completing this section with additional guidance and examples similar to the actions section.
🧹 Nitpick comments (7)
.cursor/rules/web-testing-rules.mdc (2)
2-2: Consider adding a descriptive summary.The description field is empty. Adding a brief overview of the document's purpose would help developers understand the document's intent at a glance.
-description: +description: Best practices for testing Vue components and Pinia stores
9-9: Fix typo in terminal command guidance.There's a spelling error in "termical" which should be "terminal".
-Use pnpm when running termical commands and stay within the web directory. +Use pnpm when running terminal commands and stay within the web directory.web/test/store/server.test.ts (5)
21-21: Address theanyusage in the type definition.The
MockServerStoretype broadens the type signature toRecord<string, any>, introducingany. Consider defining more precise fields or an index signature to ensure type safety:-type MockServerStore = ReturnType<typeof useServerStore> & Record<string, any>; +type MockServerStore = ReturnType<typeof useServerStore> & { + [key: string]: unknown; +};🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 21-21:
Unexpected any. Specify a different type
24-24: Replace theanycast with a stronger type.Currently
createTestDatareturnsTafter casting toany, which negates the utility of generic types. Consider removing the cast:-const createTestData = <T extends Record<string, any>>(data: T): T => data as any; +const createTestData = <T extends Record<string, unknown>>(data: T): T => data;🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 24-24:
Unexpected any. Specify a different type
[failure] 24-24:
Unexpected any. Specify a different type
110-110: Use optional chaining for safer property access.To avoid potential runtime errors when
siteis undefined or null, consider optional chaining:- (store.site && store.site.includes('www.') && store.site.includes('unraid.net')) + (store.site?.includes('www.') && store.site?.includes('unraid.net'))🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
163-163: Define a more specific type for the parameter instead ofany.The
setServermethod uses(data: any). For clarity, define a proper interface or use an existing type from the codebase to strongly typedata:-store.setServer = vi.fn((data: any) => { +interface ServerData { + apiVersion?: string; + // ... add more fields as needed +} + +store.setServer = vi.fn((data: ServerData) => {🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 163-163:
Unexpected any. Specify a different type
300-300: Replaceanywith a more definite type in the loop.To strengthen type safety, specify the type for each property rather than using a generic
any:-toRefs: vi.fn((obj) => { +toRefs: vi.fn((obj: Record<string, unknown>) => {🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 300-300:
Unexpected any. Specify a different type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
.cursor/rules/web-testing-rules.mdc(1 hunks)web/store/server.ts(3 hunks)web/test/store/server.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/store/server.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/test/store/server.test.ts
[error] 110-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: Build Web App
web/test/store/server.test.ts
[failure] 21-21:
Unexpected any. Specify a different type
[failure] 24-24:
Unexpected any. Specify a different type
[failure] 24-24:
Unexpected any. Specify a different type
[failure] 163-163:
Unexpected any. Specify a different type
[failure] 300-300:
Unexpected any. Specify a different type
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
.cursor/rules/web-testing-rules.mdc (3)
55-55: Good approach to element selection with helpful preference.The guidance to prefer semantic selectors over data test IDs aligns with best practices as it makes tests more resilient to implementation changes.
75-75: Helpful reference to existing mocks.Pointing developers to the existing mocks directory is valuable for maintaining consistency and reducing duplication in test code.
1-120: Well-structured testing documentation with practical examples.Overall, this is a comprehensive guide that covers the essential aspects of testing Vue components and Pinia stores. The inclusion of TypeScript examples and specific techniques makes it particularly useful for developers.
Some additional suggestions to enhance the document:
- Consider adding a section on handling component props in tests
- Include guidance on testing components with router or Vuex dependencies
- Add examples of common testing patterns for form validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/test/store/server.test.ts (2)
21-21: Consider removing 'any' usage for stricter typing.These lines declare or accept generic records using
any, which may contradict stricter TypeScript configurations or best practices. Consider replacinganywithunknownor using a well-defined interface to make the code more robust and type-safe.Below is a possible change:
- type MockServerStore = ReturnType<typeof useServerStore> & Record<string, any>; + type MockServerStore = ReturnType<typeof useServerStore> & Record<string, unknown>; - const createTestData = <T extends Record<string, any>>(data: T): T => data as T; + const createTestData = <T extends Record<string, unknown>>(data: T): T => data as T; - store.setServer = vi.fn((data: Record<string, any>) => { + store.setServer = vi.fn((data: Record<string, unknown>) => {Also applies to: 24-24, 163-163
🧰 Tools
🪛 GitHub Check: Build Web App
[failure] 21-21:
Unexpected any. Specify a different type
110-110: Consider optional chaining for readability.Using optional chaining can be clearer and obviates the need for explicit null/undefined checks:
- (store.site && store.site.includes('www.') && store.site.includes('unraid.net')) + (store.site?.includes('www.') && store.site?.includes('unraid.net'))🧰 Tools
🪛 Biome (1.9.4)
[error] 110-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/test/store/server.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/test/store/server.test.ts (3)
web/store/server.ts (1)
useServerStore(61-1399)web/types/server.ts (3)
ServerStateDataAction(182-184)ServerconnectPluginInstalled(36-36)ServerUpdateOsResponse(54-63)web/composables/gql/graphql.ts (1)
PartialCloudFragment(2183-2183)
🪛 Biome (1.9.4)
web/test/store/server.test.ts
[error] 110-110: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: Build Web App
web/test/store/server.test.ts
[failure] 21-21:
Unexpected any. Specify a different type
[failure] 24-24:
Unexpected any. Specify a different type
[failure] 163-163:
Unexpected any. Specify a different type
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/test/mocks/ui-components.ts (1)
6-33: Good mock implementation with some minor issues.The
MockBrandButtoncomponent is well-structured with a comprehensive list of props and a template that binds most of them. However, there are a few issues to consider:
- Some props (
icon,iconRight,iconRightHoverDisplay) are defined but not used in the template.- The
:hrefand:externalattributes are not standard for HTML button elements.Consider addressing these issues:
template: ` <button :class="props.class" :disabled="props.disabled" - :href="props.href" - :external="props.external" :title="props.title" > + <span v-if="props.icon" class="icon-left">{{ props.icon }}</span> {{ props.text || '' }} <slot /> + <span v-if="props.iconRight" :class="{'hover-only': props.iconRightHoverDisplay}" class="icon-right">{{ props.iconRight }}</span> </button> `,Or if this is supposed to be a link button:
template: ` - <button + <a :class="props.class" :disabled="props.disabled" :href="props.href" :target="props.external ? '_blank' : undefined" :rel="props.external ? 'noopener noreferrer' : undefined" :title="props.title" > {{ props.text || '' }} <slot /> - </button> + </a> `,web/test/store/server.test.ts (3)
107-113: Use optional chaining to prevent potential errorsThe current implementation could throw an error if
store.siteis undefined or null. Using optional chaining would make this code more robust.isRemoteAccess: { get: () => !!( store.wanFQDN || - (store.site && store.site.includes('www.') && store.site.includes('unraid.net')) + (store.site?.includes('www.') && store.site?.includes('unraid.net')) ), },🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
320-342: Consider adding negative test cases for the setServer methodThe current test only covers the happy path. Consider adding tests for:
- Handling of partial data
- Handling of null or undefined values
- Edge cases for cloud error handling
This would ensure the method is robust against all possible inputs.
702-723: Consider expanding cloud error test assertionsWhile the test verifies basic error creation, consider adding more assertions to verify all properties of the created error object, including the error type.
expect(store.cloudError).toBeDefined(); expect((store.cloudError as { message: string })?.message).toBe('Test error'); +expect((store.cloudError as { type: string })?.type).toBe('unraidApiState');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
web/test/mocks/ui-components.ts(1 hunks)web/test/store/server.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/test/store/server.test.ts (3)
web/store/server.ts (1)
useServerStore(61-1399)web/types/server.ts (3)
ServerStateDataAction(182-184)ServerconnectPluginInstalled(36-36)ServerUpdateOsResponse(54-63)web/composables/gql/graphql.ts (1)
PartialCloudFragment(2183-2183)
🪛 Biome (1.9.4)
web/test/store/server.test.ts
[error] 111-111: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
web/test/mocks/ui-components.ts (2)
3-4: Nice utility function addition.The
mockCnfunction correctly mimics the behavior of class name concatenation utilities by filtering falsy values and joining the remaining ones with spaces. This is a clean, concise implementation that will be useful for tests.
35-38: Clean mock export implementation.Good job updating the mock for
@unraid/uito include both themockCnfunction and theMockBrandButtoncomponent. This approach makes the mocks available for tests in a clean and maintainable way.web/test/store/server.test.ts (5)
25-25: Good use of type assertionsThe helper function for creating test data with type assertions is a smart approach that provides type safety while testing.
593-609: Great test coverage for OS version ignore functionalityThe tests thoroughly verify adding, removing, and clearing ignored OS versions - covering all key functionality.
1-20: Well-structured imports and clear file organizationThe file has well-organized imports and proper type definitions, making it easy to understand what external dependencies are required.
217-296: Complete and thorough mock setupAll dependent stores and services are properly mocked, ensuring that the tests focus solely on the server store's behavior without interference.
297-724: Comprehensive test coverage of store functionalityThe test suite provides excellent coverage of various aspects of the server store, including:
- Initial state
- State updates
- Computed properties
- License expiration
- Device limits
- Remote access detection
- Payload generation
- OS version management
This comprehensive coverage ensures the store behaves as expected in all key scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
web/test/store/server.test.ts (5)
23-25: Consider ifcreateTestDatais truly necessary.
This helper function essentially performs a type assertion. If there's no additional logic, you could inline type assertions at call sites.
113-114: Use optional chaining to simplify checks.
You can replacestore.site && store.site.includes('www.') && store.site.includes('unraid.net')with optional chaining:- (store.site && store.site.includes('www.') && store.site.includes('unraid.net')) + (store.site?.includes('www.') && store.site?.includes('unraid.net'))🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
601-637: Add additional filter scenarios if needed.
The logic forfilteredKeyActionsis validated well here. Consider testing edge cases (e.g., empty arrays) or mixing multiple filters.
653-659: Consider negative scenarios forrefreshServerState.
The test covers a happy path where the method resolves totrue. Verify whether it handles rejected Promises or false responses.
694-718: Expand cloud error test coverage.
You're testing the presence of errors, but you might also confirm behavior when no error key is present or a different error type is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/test/store/server.test.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
web/test/store/server.test.ts
[error] 114-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
web/test/store/server.test.ts (3)
1-4: Good use of test documentation.
This opening doc block provides clear context for the tests. Nice job keeping it concise and informative.
190-200: Solid coverage of OS ignore-release methods.
These methods appear well tested, thoroughly addressing multiple add/remove scenarios.
292-304: Effective use of lifecycle hooks.
UsingbeforeEachandafterEachhelps maintain test isolation and ensures each run starts fresh.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
web/__test__/mocks/ui-components.ts (1)
35-39: Good mock implementation with room for extension.The mock setup using
vi.mockis well-structured and provides a clear path for adding more UI components in the future.Consider defining a type interface for the props to improve type safety and documentation.
// Define interfaces for better type safety interface BrandButtonProps { class?: string; disabled?: boolean; external?: boolean; href?: string; icon?: string; iconRight?: string; iconRightHoverDisplay?: boolean; text?: string; title?: string; } // Then use it in setup setup(props: BrandButtonProps) { return { props }; }web/__test__/store/server.test.ts (2)
111-115: Use optional chaining for safer property accessThe current implementation doesn't handle the case where
store.sitemight be undefined before callingincludes().get: () => !!( store.wanFQDN || - (store.site && store.site.includes('www.') && store.site.includes('unraid.net')) + (store.site?.includes('www.') && store.site?.includes('unraid.net')) ),🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
691-718: Direct assignment of cloudError propertyThe test for cloud error handling directly assigns the
cloudErrorproperty instead of testing how it's actually set in the implementation.If there's a method that should set this property based on the cloud error, consider testing that method instead:
// Error when registered store.setServer({ registered: true, cloud: createTestData({ error: 'Test error', }) as PartialCloudFragment, }); - store.cloudError = { - message: 'Test error', - type: 'unraidApiState', - }; + // Let the actual computed getter run or call the method that sets cloudError expect(store.cloudError).toBeDefined(); expect((store.cloudError as { message: string })?.message).toBe('Test error');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
web/__test__/helpers/__snapshots__/markdown.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
.cursor/rules/web-testing-rules.mdc(1 hunks)web/__test__/components/Auth.test.ts(1 hunks)web/__test__/components/DownloadApiLogs.test.ts(1 hunks)web/__test__/components/KeyActions.test.ts(1 hunks)web/__test__/helpers/apollo-cache/merge.test.ts(11 hunks)web/__test__/helpers/markdown.test.ts(1 hunks)web/__test__/mocks/ui-components.ts(1 hunks)web/__test__/setup.ts(1 hunks)web/__test__/store/server.test.ts(1 hunks)web/vitest.config.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/test/helpers/apollo-cache/merge.test.ts
- web/test/helpers/markdown.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .cursor/rules/web-testing-rules.mdc
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/__test__/components/KeyActions.test.ts (1)
web/types/server.ts (2)
ServerStateDataActionType(180-180)ServerStateDataAction(182-184)
🪛 Biome (1.9.4)
web/__test__/store/server.test.ts
[error] 114-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (37)
web/__test__/mocks/ui-components.ts (1)
4-4: Nice utility function for class name handling.The
mockCnfunction correctly mimics the behavior of combining CSS class names by filtering out falsy values and joining with spaces.web/vitest.config.mjs (2)
17-17: Well-organized test setup path.The path update for setupFiles reflects the modern convention of using
__test__directory for test files, improving organization and making it clearer where test setup code resides.
22-22: Consolidated test pattern.Consolidating the test file patterns into a single directory structure simplifies the test discovery process and creates a more organized test hierarchy. This change aligns well with modern testing practices.
web/__test__/components/Auth.test.ts (5)
1-48: Well-structured test setup with proper mocking.The test file has a clear organization with:
- Proper TypeScript interfaces for mock data types
- Clean mock implementations for vue-i18n and store
- Good separation of concerns
This setup provides a solid foundation for behavior-driven tests.
49-68: Good verification of button rendering.This test properly verifies that the Auth component renders a button when authAction is available, checking both existence and props. The test follows the Vue Testing Library philosophy by focusing on component behavior rather than implementation details.
70-91: Thorough error state verification.The test correctly verifies error state rendering, checking both the heading and message content. This ensures the component properly handles and displays error conditions to users.
93-113: Effective click handler verification.The test properly verifies that the click handler is called when the button is clicked. Using
$emit('click')is appropriate for testing the component's event handling.
115-127: Complete behavior verification with negative test case.This test properly verifies that no button is rendered when authAction is undefined, ensuring the component behaves correctly in all possible states. Including this negative test case improves test coverage.
web/__test__/components/KeyActions.test.ts (9)
5-33: Appropriate imports and mocks for component testing.The test file includes all necessary imports and properly mocks external dependencies like the server store and Pinia. The direct import of the KeyActions component from the relative path is clear and appropriate.
43-53: Effective store-based rendering test.This test verifies that buttons render correctly from the store when no actions prop is provided. It checks both the number of buttons and their text content, which is a good practice for ensuring the component behaves as expected.
55-71: Good verification of props-based rendering.This test confirms that buttons render correctly from the actions prop when provided, overriding the store values. This validation ensures the component correctly prioritizes prop values over store values.
73-83: Appropriate empty state handling test.The test verifies that the component handles empty actions appropriately, rendering an empty list container. This is important for ensuring the component degrades gracefully when no actions are available.
85-101: Proper click handler verification.This test ensures the click handler is called when a button is clicked, which is critical for interactive components. The test properly isolates the behavior being tested.
103-124: Good disabled state verification.This test verifies that click handlers are not called for disabled buttons, which is an important accessibility and user experience consideration. This test helps prevent unexpected behavior with disabled UI elements.
126-168: Comprehensive filtering tests.These tests verify both the filterBy and filterOut props work correctly, ensuring the component can properly filter actions based on different criteria. The tests are thorough and check both the number and content of the filtered results.
170-186: Good styling prop verification.This test confirms that the maxWidth styling is applied when the corresponding prop is set to true. Testing styling props is important for ensuring the component's visual presentation behaves as expected.
188-217: Thorough props validation.This test ensures all required props are correctly passed to the BrandButton component, including text, title, href, external state, disabled state, and icon. This comprehensive verification helps prevent regression issues in the component interface.
web/__test__/components/DownloadApiLogs.test.ts (4)
13-25: Well-structured mocks for external dependencies.The test properly mocks both the URLs helper and the vue-i18n library, ensuring predictable behavior during testing. Using URL objects for mocking is a good practice for testing components that interact with external resources.
34-57: Thorough download button verification.This test effectively verifies that the download button has the correct URL (including CSRF token), attributes, and text. The test creates and validates against an expected URL, which is a good practice for testing URL construction.
59-88: Complete support links verification.This test thoroughly verifies all support links, checking their href attributes, text content, and security attributes (target, rel). The test also confirms the correct number of links, which helps prevent regressions if links are added or removed.
90-108: Good instructional text verification.This test confirms that key instructional messages about log usage and privacy are displayed, ensuring users receive important information about handling sensitive data. Testing instructional text is important for ensuring a good user experience.
web/__test__/store/server.test.ts (15)
1-24: Well-structured test setup with clear documentation and type safetyThe file starts with clear documentation and imports. The
MockServerStoretype definition andcreateTestDatahelper function provide good type safety for the tests.
26-165: Well-implemented mock store setup with thorough computed property definitionsThe store setup is comprehensive and includes mocking of all necessary properties and computed values needed for tests. This thorough approach ensures that tests can run independently of the actual implementation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 114-114: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
166-203: Complete mock implementation of store methodsAll store methods are properly mocked with necessary implementation details. The use of
vi.fn()for creating spy functions is a good practice for verifying method calls in tests.
207-290: Comprehensive mocking of dependent stores and servicesAll dependencies are properly mocked, which is essential for isolating the server store behavior in tests. The imports are mocked using
vi.mock()and provide minimal implementations needed for the tests.
292-304: Standard test setup and teardownThe test suite uses proper setup with
beforeEachand cleanup withafterEach, ensuring that tests don't interfere with each other.
305-337: Effective store initialization and state update testsThese tests verify the basic functionality of store creation and the
setServermethod, focusing on the correct updating of state properties.
339-374: Thorough testing of computed propertiesThe tests for
computedRegDevsandregUpdatesExpiredverify that these computed properties return the expected values based on different input states.
376-444: Comprehensive state data tests for different server statesThe tests for
stateDatacomprehensively verify that the correct data is returned for each server state (ENOKEYFILE, TRIAL, EEXPIRED, PRO), including human-readable messages and error flags.
446-485: Effective testing of the tooManyDevices computed propertyThe tests verify that
tooManyDevicescorrectly identifies situations where the device count exceeds limitations or when there are configuration errors.
487-513: Good test cases for remote access detectionThe tests for
isRemoteAccessverify the property works correctly for different combinations ofwanFQDNandsitevalues.
515-581: Thorough testing of payload generationThe tests for
serverPurchasePayloadandserverAccountPayloadverify that these objects are correctly populated with the required data from the store.
583-599: Complete testing of OS version ignore functionalityThese tests verify that the methods for adding, removing, and clearing ignored OS releases work as expected.
601-637: Effective testing of key action filteringThe test verifies that the
filteredKeyActionsmethod correctly filters actions based on provided criteria. The use ofvi.spyOnto mock the computed property is a good approach.
639-649: Simple but effective test for isOsVersionStableThe test verifies that the property correctly identifies stable and non-stable OS versions.
661-689: Good tests for setter methodsThe tests for
setUpdateOsResponseandsetRebootVersionverify that these methods correctly update the store state.web/__test__/setup.ts (1)
17-20:❓ Verification inconclusive
Verify i18n mock configuration.
The simple i18n mock is still present in the config, but the dedicated
vue-i18n.tsimport has been removed. Ensure that this simplified mock is sufficient for all tests that depend on i18n functionality.
🏁 Script executed:
#!/bin/bash # Check if any tests are using i18n features that might rely on the removed mock echo "Checking for i18n usage in tests..." rg "\$t\(|useI18n|i18n\." --type ts --type vue --type js --glob "web/__test__/**"Length of output: 191
Update: Verify i18n Test Coverage
The simplified i18n mock in
web/__test__/setup.ts(lines 17–20) remains active now that the dedicatedvue-i18n.tsimport has been removed. Please confirm that none of the tests inweb/__test__rely on i18n functionality requiring a more detailed configuration. The initial verification script produced an error with vue files, so please re-run the following checks:#!/bin/bash echo "Checking for i18n usage in TS/JS test files..." rg "\$t\(|useI18n|i18n\." --type ts --type js web/__test__/ echo "Checking for i18n usage in Vue test files..." rg "\$t\(|useI18n|i18n\." --glob "*.vue" web/__test__/If these commands return no i18n usage, the simplified mock should be sufficient; otherwise, please refine the mock or update the tests accordingly.
| import './mocks/shared-callbacks.js'; | ||
| import './mocks/ui-components.js'; | ||
| import './mocks/stores/index.js'; | ||
| import './mocks/services/index.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Import extensions changed and some imports removed.
The mock imports have been changed from TypeScript (.ts) to JavaScript (.js) extensions. Additionally, several imports that were previously included in the file have been removed:
'./mocks/vue-i18n.ts''./mocks/vue.ts''./mocks/pinia.ts''./mocks/ui-libraries.ts'
Please verify that these mocks are no longer needed or have been consolidated into the remaining imports to ensure all required test functionality is preserved.
🏁 Script executed:
#!/bin/bash
# Check if any tests might be importing the removed mock files directly
echo "Checking for direct imports of removed mocks..."
rg "import.*(vue-i18n|vue\.ts|pinia\.ts|ui-libraries)" --type ts --type vue --type js --glob "!web/__test__/setup.ts"
# Check if the remaining JS mock files exist and contain the necessary functionality
echo "Checking if JS mock files exist..."
ls -la web/__test__/mocks/shared-callbacks.js web/__test__/mocks/ui-components.js web/__test__/mocks/stores/index.js web/__test__/mocks/services/index.js 2>/dev/null || echo "Some mock files are missing"Length of output: 552
Mock Imports Modification Verification
- The imports in
web/__test__/setup.tshave been updated to use JavaScript extensions, and several TS mocks (e.g.,vue-i18n.ts,vue.ts,pinia.ts,ui-libraries.ts) have been removed. - A verification script did not conclusively detect direct imports of the removed mocks (note: an error was reported due to an unrecognized file type for Vue files).
- The script output indicates that one or more of the remaining JS mock files (
shared-callbacks.js,ui-components.js,stores/index.js,services/index.js) are missing. - Action: Please verify that the missing mock file(s) are intentionally removed or consolidated into other mocks, and ensure that all necessary test functionality is preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/__test__/store/server.test.ts (1)
204-205: Mocking the same method in the store and override in the test
As noted in previous reviews, redefiningrefreshServerStatein both places can undermine test coverage for the actual method. Consider removing the mock inside the store or unify your approach so the test exercises the real logic.Also applies to: 668-675
🧹 Nitpick comments (2)
web/__test__/store/server.test.ts (2)
25-26: Consider simplifying or removingcreateTestData
It's a helpful function but may be unnecessary if you're already ensuring test object shapes elsewhere. TypeScript often infers the types without requiring an extra utility function.
112-116: Use optional chaining for site checks
Instead ofstore.site && store.site.includes('www.') && store.site.includes('unraid.net'), you can simplify the logic as follows:- !!(store.site && store.site.includes('www.') && store.site.includes('unraid.net')) + !!(store.site?.includes('www.') && store.site?.includes('unraid.net'))🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
web/__test__/components/DownloadApiLogs.test.ts(1 hunks)web/__test__/mocks/ui-components.ts(1 hunks)web/__test__/store/server.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/test/components/DownloadApiLogs.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
web/__test__/store/server.test.ts
[error] 116-116: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build API
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
web/__test__/mocks/ui-components.ts (2)
1-4: Appropriate utility function implementation for class name handling.The
mockCnfunction correctly mimics utility functions likeclsxorcnby filtering falsy values and joining with spaces.
40-44: Clear module mocking implementation.The mock implementation correctly provides the necessary components for testing. As the comment suggests, you can extend this pattern to include other UI components as they become needed in tests.
web/__test__/store/server.test.ts (2)
307-315: Good coverage for store initialization
The test validates important default properties and ensures the store is set up correctly.
490-515: Thorough checks for remote access
These test scenarios accurately cover typical usage patterns. Great job ensuring all relevant paths are tested.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
This gets the original 3 component tests refactored to better follow the Vue Testing Library philosophy and test behavior. This also adds a new test file for the server store. Additional batches of tests will be added in proceeding PR's.
Summary by CodeRabbit
Chores
Tests
Auth,DownloadApiLogs, andKeyActionsto ensure robust behavior.These updates optimize performance and stability behind the scenes without altering the end-user experience.