Improve test coverage: mapState functions + app/jumpLinks/systemsList #46
Conversation
…ata validation - Introduced new test files for app module, jump links, map state, systems list, and utility functions. - Implemented coverage reporting for tests using Vitest. - Created mocks for dependencies to isolate tests and ensure predictable behavior. - Validated data structures and ensured integrity of jump links and systems. - Enhanced DOM manipulation tests to verify rendering logic and visibility toggling. - Established utility tests for type validation and system ID management.
There was a problem hiding this comment.
Pull Request Overview
This PR significantly improves test coverage by adding comprehensive unit and integration tests for core application functionality. The addition targets MapStateImpl core functions, app module wiring, and data validation while achieving near 99% statement coverage.
Key changes:
- Add unit tests for MapStateImpl core functions (toggles, window resize, controls, animation, cleanup)
- Add integration tests for app module bootstrap with DOM event handling and UI controls
- Add data shape validation tests for jumpLinks and systemsList modules
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/utils/dom.test.ts | Tests DOM builder functions for star and link elements with proper fallback behavior |
| scripts/types.test.ts | Tests type utilities including system ID set building, data validation, and jump type mappings |
| scripts/systemsList.test.ts | Validates system data structure integrity and unique ID constraints |
| scripts/mapState.unit.test.ts | Unit tests for MapStateImpl with mocked Three.js controls and core functionality |
| scripts/mapState.int.test.ts | Integration tests for MapStateImpl including label visibility based on camera distance |
| scripts/jumpLinks.test.ts | Validates jump link data structure, year bounds, and prevents self-referential bridges |
| scripts/app.module.test.ts | Tests app bootstrap process with mocked dependencies and UI event handling |
| package.json | Adds coverage script and vitest coverage dependency |
| declare global { | ||
| var __testMapState: MapStateImplMock | undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Using global variables for test state can lead to test pollution and coupling issues. Consider using a more controlled approach like returning the mock instance from the module or using a test context pattern.
| const map = new MapStateImpl(systems, jumps); | ||
| const renderSpy = vi.spyOn(map, "render"); | ||
| map.init(); | ||
| // @ts-expect-error using mock helper |
There was a problem hiding this comment.
[nitpick] The @ts-expect-error directive suppresses type checking for accessing the mock's trigger method. Consider typing the mock properly or using a more type-safe approach to avoid bypassing TypeScript's type safety.
| // @ts-expect-error using mock helper |
| type LabelRefs = WeakMap<object, LabelEntry>; | ||
| const labelRefs = (map as unknown as { labelRefs: LabelRefs }).labelRefs; | ||
| const firstLabels = labelRefs.get(map.systems[0])!; | ||
| const secondLabels = labelRefs.get(map.systems[1])!; |
There was a problem hiding this comment.
Using type assertion to access private properties breaks encapsulation and makes tests brittle to implementation changes. Consider exposing a test-friendly method or using public APIs to verify the behavior.
| const secondLabels = labelRefs.get(map.systems[1])!; | |
| const firstLabels = map.getLabelEntry(map.systems[0])!; | |
| const secondLabels = map.getLabelEntry(map.systems[1])!; |
This PR increases test coverage significantly.\n\nHighlights:\n- Add unit tests for MapStateImpl core functions (toggles, onWindowResize, controls change, animate, cleanup)\n- Add tests for app module wiring (DOMContentLoaded, slider + toggles)\n- Add data shape tests for jumpLinks and systemsList\n- Minor fixes to tests to satisfy ESLint and jsdom quirks\n\nResults:\n- 23 tests, all passing\n- High overall coverage (near 99% statements).\n\nNotes:\n- Optional follow-ups: raise branch coverage for mapState.ts and utils/dom.ts to 100% by adding a couple of focused tests.