chore(coverage): 🔧 improve coverage tooling and test suite#57
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR substantially expands test coverage across main/renderer cloud/auth/update flows and adds a merged coverage reporting pipeline wired into CI. The direction is good and mostly aligns with the project’s architecture, but there is one CI-level correctness issue that will likely break the pipeline as currently ordered. I would not merge until that is fixed.
Critical
.github/workflows/ci.yml:84— Coverage step likely fails due to missing test setup dependencies/build artifacts: Runningnpm run test:rendererandnpm run test:coverage:allbeforenpm run buildcan fail if renderer/main tests rely on generated artifacts or type-transpiled ESM paths (*.jsimports in TS tests/modules are common in this repo). Since the workflow previously only ran unit tests and then build, introducing these steps without ensuring prerequisite generation/transpile can cause deterministic CI failures.
Suggestion: MoveGenerate merged coverage reportafter build (or add explicit prerequisite steps before tests, e.g.,npm run generateand any required compile/prep step), and ensure the same ordering used locally fortest:coverage:all.
Important
package.json:43— Coverage merge script assumes JSON reporter output path stability:test:coverage:mergehardcodescoverage/main/coverage-final.jsonandcoverage/renderer/coverage-final.json. If Vitest/coverage provider output naming changes or reporter config drifts, the merge step will fail hard.
Suggestion: Either (a) set explicitcoverage.reportOnFailure/stable output config in Vitest files and document this contract, or (b) makecoverage-merge.mjsdiscovercoverage-final.jsondynamically within each directory and provide clearer remediation in error output.
Praise
scripts/coverage-merge.mjs:1— Pragmatic merged coverage utility: Nice use ofistanbul-lib-coveragewith scoped summaries (srcvs business files) and actionable “top uncovered” output; this is useful for prioritizing real coverage work.tests/setup.renderer.ts:1— Much more complete renderer API mocking: Expandingwindow.apimocks for cloud/auth/updater/events reduces brittle test setup and improves reliability for component/context tests.src/main/ipc/handlers/__tests__/cloud.handler.test.ts:1— Strong IPC contract validation: Good breadth on input validation, delegated call behavior, and error wrapping across many channels—this directly protects main↔renderer API stability.
AI Code Review SummaryPR: #57 (chore(coverage): 🔧 improve coverage tooling and test suite) Overall AssessmentDetected 12 actionable findings, prioritize CRITICAL/HIGH before merge. Major Findings by Severity
Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
|
| expect(mockWindowManager.sendToRenderer).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('readStream parses CRLF chunks and reconnects when stream ends naturally', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| })) | ||
|
|
||
| describe('FileWaitUtil', () => { | ||
| const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| expect(warnSpy).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('retries until file becomes available', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| authManager: mockAuthManager, | ||
| })) | ||
|
|
||
| const makeJsonResponse = (status: number, body: any) => ({ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| vi.useRealTimers() | ||
|
|
||
| const { cloudSSEManager } = await import('../CloudSSEManager.js') | ||
| const manager = cloudSSEManager as any |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| expect(mockSendToRenderer).toHaveBeenCalledWith('updater:status', { status: 'downloaded', version: '1.2.3' }) | ||
| }) | ||
|
|
||
| it('truncates error message before sending to renderer', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| vi.clearAllMocks() | ||
| handlers.clear() | ||
|
|
||
| const mod = await import('../auth.handler.js') |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| success: true, | ||
| data: { buffer, fileName: 'demo.pdf' }, | ||
| }) | ||
| mockDialog.showSaveDialog.mockResolvedValueOnce({ canceled: false, filePath: '/downloads/demo.pdf' }) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| await second | ||
| }) | ||
|
|
||
| it('forwards updater events to renderer', async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| describe('cloudTaskMapper', () => { | ||
| it('maps cloud task to local task shape', () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }) | ||
|
|
||
| describe('CloudService', () => { | ||
| beforeEach(() => { |
There was a problem hiding this comment.
[MEDIUM] Shared singleton state is not fully reset between CloudService tests
Tests rely on a shared imported singleton without explicit state reset, which can cause inter-test coupling/regressions if CloudService gains or already has mutable internal fields.
Suggestion: Use module isolation per test (vi.resetModules() before import) or add/reset a dedicated CloudService test hook/state reset in beforeEach. Prefer constructing a fresh service instance where possible instead of importing a shared default singleton.
Risk: Order-dependent failures and hard-to-debug flakes as implementation evolves (especially around cached tokens/options/retry flags).
Confidence: 0.91
| describe('CloudService', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockAuthManager.getAccessToken.mockResolvedValue('token-1') |
There was a problem hiding this comment.
[MEDIUM] Missing explicit coverage for token acquisition rejection path
Tests cover empty token handling but not the case where getAccessToken rejects with an exception, leaving an unverified error-normalization path.
Suggestion: Add tests where mockAuthManager.getAccessToken.mockRejectedValueOnce(new Error('auth down')) for key methods (at least convert, getTasks, and one mutating method) and assert consistent { success:false, error:'auth down' } behavior.
Risk: Unhandled auth exceptions could surface as unnormalized errors or crash paths in production without being caught by current tests.
Confidence: 0.90
| } | ||
| mockAuthManager.fetchWithAuth.mockResolvedValueOnce(makeJsonResponse(200, response)) | ||
|
|
||
| const result = await cloudService.getTasks(1, 10) |
There was a problem hiding this comment.
[MEDIUM] No assertion of request payload/query construction for pagination/filter methods
Many tests assert outputs but not whether input params are serialized correctly into request URL/body/options, which can hide regressions in API contract formation.
Suggestion: For getTasks/getTaskPages/getCreditHistory/getPaymentHistory/getCheckoutStatus, assert fetchWithAuth.mock.calls[n] contains expected endpoint, query params (page, page_size, filters, wait_seconds), and method.
Risk: A regression in request construction could pass tests while breaking real API behavior.
Confidence: 0.85
| expect(mockWindowManager.sendToRenderer).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('readStream parses CRLF chunks and reconnects when stream ends naturally', async () => { |
There was a problem hiding this comment.
[MEDIUM] No coverage for SSE stream read exceptions and abort/cancel path
Current SSE tests focus on parse correctness and reconnect on normal end/error status, but not failures during read or explicit cancellation flow.
Suggestion: Add tests simulating read() rejection and disconnect() during active stream; assert graceful handling, no renderer spam, and reconnect policy consistency.
Risk: Runtime stream errors may leave manager in inconsistent connected state or trigger unexpected reconnect behavior.
Confidence: 0.82
| expect(reconnectSpy).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('reconnect applies exponential backoff and caps delay', async () => { |
There was a problem hiding this comment.
[MEDIUM] SSE reconnect/backoff tests do not verify cleanup or duplicate timer prevention
Timer-heavy tests validate scheduling but not that reconnect timer lifecycle avoids stacking or stale timers, leaving potential flaky/redundant reconnect behavior untested.
Suggestion: Add assertions that reconnect clears/overwrites previous timers and that cleanup()/disconnect() nulls timers/aborts streams. Verify only one active reconnect timer after repeated failures.
Risk: Duplicate reconnect timers can cause intermittent reconnect storms and nondeterministic behavior that unit tests may currently miss.
Confidence: 0.78
| on: vi.fn((event: string, cb: (...args: any[]) => void) => { | ||
| handlers[event] = cb | ||
| }), | ||
| checkForUpdates: vi.fn().mockResolvedValue(undefined), |
There was a problem hiding this comment.
[MEDIUM] Missing error-path coverage for updater check failures
The suite does not verify behavior when autoUpdater.checkForUpdates() fails, leaving resilience and observability of that path unvalidated.
Suggestion: Add a test where mockAutoUpdater.checkForUpdates.mockRejectedValueOnce(new Error('network')) and assert expected renderer notification/logging plus reset of in-flight guard state for subsequent calls.
Risk: Unhandled promise rejection or stuck internal state could break update checks in production without test detection.
Confidence: 0.91
| }) | ||
|
|
||
| it('uses current timestamp fallback when no created_at and no started_at', () => { | ||
| const nowMin = Date.now() - 1000 |
There was a problem hiding this comment.
[MEDIUM] Time-based fallback test can be flaky due to real clock usage
The test relies on wall-clock time and a 1s window, which can intermittently fail under slow CI, timer skew, or execution pauses.
Suggestion: Use vi.useFakeTimers() + vi.setSystemTime(...) and assert exact equality for sortTimestamp, then restore real timers in cleanup.
Risk: Intermittent failures reduce CI reliability and can mask real regressions.
Confidence: 0.93
| }, | ||
| }) | ||
|
|
||
| if (!window.URL.createObjectURL) { |
There was a problem hiding this comment.
[MEDIUM] Conditional URL API mocking can cause test-order dependent behavior
Global URL methods are mocked only when absent, which can lead to non-deterministic test behavior depending on runtime environment and library versions.
Suggestion: Always stub these globals in setup (or vi.spyOn + mockImplementation) and restore/reset in hooks for deterministic behavior. Example: assign window.URL.createObjectURL = vi.fn(() => 'blob:mock-url') unconditionally in setup and clear in beforeEach.
Risk: Flaky tests and hidden regressions due to differing global behavior across environments or after jsdom upgrades.
Confidence: 0.82
| expect(global.fetch).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should return error when browser cannot be opened', async () => { |
There was a problem hiding this comment.
[LOW] No assertion that browser-open failure avoids entering polling state
The test checks error state but does not assert that polling/timers were not started after browser open failure.
Suggestion: Also assert no subsequent polling attempt (e.g., no token-endpoint fetch calls or timer scheduling, depending on implementation hooks) after shell.openExternal rejection.
Risk: A hidden side effect could keep background polling alive despite immediate failure, causing flaky state transitions or resource leaks.
Confidence: 0.86
| coverage: { | ||
| provider: "v8", | ||
| reporter: ["text", "json", "html"], | ||
| include: [ |
There was a problem hiding this comment.
[LOW] Coverage config change lacks guard test for include/exclude correctness
Coverage scope has materially changed, but there is no validation that key source areas are still measured and that test/config files remain excluded as intended.
Suggestion: Add a lightweight CI assertion (script or snapshot of coverage summary paths) to catch accidental glob regressions, especially across monorepo-like src segments.
Risk: Coverage metrics may drift silently, creating false confidence in test completeness.
Confidence: 0.79
1 participant
Summary
Test Plan
npm run test:unitnpm run test:renderernpm run test:coverage:all🤖 Generated with Codex Cli