Add comprehensive unit tests with Vitest#20
Conversation
…ponent coverage Co-authored-by: KagamiChan <3816900+KagamiChan@users.noreply.github.com>
…r and selectors Co-authored-by: KagamiChan <3816900+KagamiChan@users.noreply.github.com>
…omments Co-authored-by: KagamiChan <3816900+KagamiChan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit testing infrastructure using Vitest to improve code quality and test coverage. The goal is to achieve 100% coverage for utility functions and 70%+ coverage for React components.
Changes:
- Adds Vitest configuration with jsdom environment and coverage reporting
- Creates test mocks for external dependencies (react-i18next, react-redux, views utilities)
- Implements unit tests for utilities (timer-state, functions, fleet-utils, fleet-selectors, factor)
- Adds component tests (ship-row, fleet-list, candidates, countup-timer, index)
- Configures test scripts and coverage output
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Configures Vitest with jsdom, coverage settings, and module aliases |
| src/tests/setup.ts | Sets up global test mocks for IntersectionObserver, window.ticker, and other globals |
| src/tests/timer-state.test.ts | Tests for timer state management (100% coverage) |
| src/tests/functions.test.ts | Tests for utility functions including repair calculations and styling |
| src/tests/fleet-utils.test.ts | Tests for fleet utility functions with comprehensive edge cases |
| src/tests/fleet-selectors.test.ts | Tests for Redux selectors with memoization verification |
| src/tests/ship-row.test.tsx | Component tests for ship row display and interactions |
| src/tests/fleet-list.test.tsx | Component tests for fleet list rendering |
| src/tests/countup-timer.test.tsx | Component tests for timer behavior |
| src/tests/candidates.test.tsx | Component tests for repair queue with sorting |
| src/tests/index.test.tsx | Component tests for main plugin container |
| src/tests/factor.test.ts | Tests for ship type repair factor constants |
| src/tests/mocks/*.ts(x) | Mock implementations for external dependencies |
| package.json | Adds test scripts and testing dependencies |
| .gitignore | Excludes coverage output directory |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /** Helper to create a path alias from a relative path */ | ||
| const alias = (path: string) => new URL(path, import.meta.url).pathname |
There was a problem hiding this comment.
The alias helper function uses import.meta.url which returns a URL string, but then calls .pathname on it. On Windows systems, this can produce paths like /C:/Users/... with a leading slash before the drive letter, which may cause issues with module resolution. Consider using fileURLToPath from the url module instead for cross-platform compatibility:
import { fileURLToPath } from 'url'
const alias = (path: string) => fileURLToPath(new URL(path, import.meta.url))| /** Helper to create a path alias from a relative path */ | |
| const alias = (path: string) => new URL(path, import.meta.url).pathname | |
| import { fileURLToPath } from 'url' | |
| /** Helper to create a path alias from a relative path */ | |
| const alias = (path: string) => fileURLToPath(new URL(path, import.meta.url)) |
| }) | ||
|
|
||
| // Export the mock ticker for test manipulation | ||
| export { mockTicker } |
There was a problem hiding this comment.
The mockTicker is exported and used in tests, but the type annotation MockTicker is only defined in the test file. Consider moving the MockTicker interface to the setup.ts file and exporting it alongside the mockTicker to ensure type consistency across test files.
| expect(true).toBe(true) | ||
| }) | ||
|
|
||
| it('should handle api_req_hensei/change event', () => { | ||
| const store = createMockStore() | ||
|
|
||
| render( | ||
| <Provider store={store}> | ||
| <PluginAnchorageRepair /> | ||
| </Provider>, | ||
| ) | ||
|
|
||
| const event = new CustomEvent('game.response', { | ||
| detail: { | ||
| path: '/kcsapi/api_req_hensei/change', | ||
| postBody: { | ||
| api_id: '1', | ||
| api_ship_id: '101', | ||
| api_ship_idx: '0', | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| act(() => { | ||
| window.dispatchEvent(event) | ||
| }) | ||
|
|
||
| expect(true).toBe(true) |
There was a problem hiding this comment.
These tests only verify that the event handlers don't throw errors (by asserting expect(true).toBe(true)), which doesn't provide meaningful test coverage. Consider adding assertions that verify the actual behavior of these event handlers, such as checking if state updates correctly, if timers are reset, or if specific functions are called with expected parameters.
| it('should handle api_req_nyukyo/start event', () => { | ||
| const store = createMockStore() | ||
|
|
||
| render( | ||
| <Provider store={store}> | ||
| <PluginAnchorageRepair /> | ||
| </Provider>, | ||
| ) | ||
|
|
||
| const event = new CustomEvent('game.response', { | ||
| detail: { | ||
| path: '/kcsapi/api_req_nyukyo/start', | ||
| postBody: { | ||
| api_ship_id: '101', | ||
| api_highspeed: '1', | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| act(() => { | ||
| window.dispatchEvent(event) | ||
| }) | ||
|
|
||
| expect(true).toBe(true) | ||
| }) |
There was a problem hiding this comment.
This test only verifies that the handler doesn't throw an error. The comment "The component should handle the event without error" and the assertion expect(true).toBe(true) provide no meaningful validation. Consider asserting specific behavior like timer state changes, function calls, or component re-renders that should occur when handling these game events.
| it('should show repair estimate when ship is damaged and not in dock', () => { | ||
| const ship = createShipData({ | ||
| api_nowhp: 30, | ||
| api_maxhp: 40, | ||
| availableSRF: true, | ||
| inRepair: false, | ||
| timePerHP: 60000, | ||
| }) | ||
|
|
||
| render( | ||
| <table> | ||
| <tbody> | ||
| <ShipRow | ||
| ship={ship} | ||
| timeElapsed={1500} | ||
| lastRefresh={Date.now()} | ||
| canRepair={true} | ||
| canBoostMorale={false} | ||
| moraleTimeElapsed={0} | ||
| /> | ||
| </tbody> | ||
| </table>, | ||
| ) | ||
|
|
||
| // repairEstimate should show some value | ||
| // The function calculates HP repaired | ||
| }) |
There was a problem hiding this comment.
This test verifies that repair estimate is shown but doesn't assert any specific expected values or behavior. The comment "The function calculates HP repaired" is incomplete and doesn't clarify what should be verified. Consider adding assertions to check the actual calculated value or visible output from the repair estimate calculation.
| it('should not allow sorting by Ship column', () => { | ||
| const store = createMockStore({ | ||
| 101: { | ||
| api_id: 101, | ||
| api_ship_id: 1, | ||
| api_lv: 50, | ||
| api_nowhp: 30, | ||
| api_maxhp: 40, | ||
| api_ndock_time: 600000, | ||
| api_cond: 49, | ||
| }, | ||
| }) | ||
|
|
||
| render( | ||
| <Provider store={store}> | ||
| <RepairQueue /> | ||
| </Provider>, | ||
| ) | ||
|
|
||
| const shipHeader = screen.getByText('Ship') | ||
| fireEvent.click(shipHeader) | ||
|
|
||
| // Ship column should not show sort indicator | ||
| expect(screen.queryByText('↑')).not.toBeInTheDocument() | ||
| expect(screen.queryByText('↓')).not.toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
This test clicks the Ship header but doesn't verify the expected behavior. While the comment suggests sorting should not be allowed on this column, the test only verifies that sort indicator icons are not present. However, this doesn't confirm whether clicking actually does nothing or if it triggers any unintended behavior. Consider adding a more specific assertion, such as verifying the order of items before and after the click remains unchanged.
| it('should handle ticker callback errors gracefully', () => { | ||
| const consoleError = vi.spyOn(console, 'error').mockImplementation(() => { | ||
| // Suppress console.error during test | ||
| }) | ||
| const throwingCallback = vi.fn(() => { | ||
| throw new Error('Test error') | ||
| }) | ||
|
|
||
| render( | ||
| <CountupTimer | ||
| countdownId="test-timer-error" | ||
| startTime={Date.now()} | ||
| tickCallback={throwingCallback} | ||
| />, | ||
| ) | ||
|
|
||
| // Should not throw when tick occurs | ||
| expect(() => { | ||
| act(() => { | ||
| getMockTicker().tick(Date.now()) | ||
| }) | ||
| }).not.toThrow() | ||
|
|
||
| expect(consoleError).toHaveBeenCalled() | ||
| consoleError.mockRestore() | ||
| }) |
There was a problem hiding this comment.
This test expects an error to be thrown but uses expect(() => {...}).not.toThrow() which is contradictory. The test setup creates a callback that throws an error, but then verifies that ticking doesn't throw - this suggests the error is caught internally. However, the test also checks that console.error was called, implying errors are logged but not propagated. This test logic is confusing and should be clarified. If errors are meant to be caught and logged, consider testing that behavior explicitly rather than mixing throw and not-throw assertions.
| @@ -0,0 +1,390 @@ | |||
| import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest' | |||
There was a problem hiding this comment.
Unused import beforeEach.
| import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest' | |
| import { describe, it, expect, vi, afterEach } from 'vitest' |
| // Helper to create a repair dock | ||
| const createRepairDock = (api_ship_id: number): APIGetMemberNdockResponse => | ||
| ({ | ||
| api_ship_id, | ||
| }) as APIGetMemberNdockResponse | ||
|
|
There was a problem hiding this comment.
Unused variable createRepairDock.
| // Helper to create a repair dock | |
| const createRepairDock = (api_ship_id: number): APIGetMemberNdockResponse => | |
| ({ | |
| api_ship_id, | |
| }) as APIGetMemberNdockResponse |
| @@ -0,0 +1,180 @@ | |||
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' | |||
There was a problem hiding this comment.
Unused import afterEach.
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' | |
| import { describe, it, expect, vi, beforeEach } from 'vitest' |
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.