test: add 121 unit tests for highest-impact coverage gaps#514
test: add 121 unit tests for highest-impact coverage gaps#514zbigniewsobiecki merged 2 commits intodevfrom
Conversation
|
👍 Just a sec, looking into the unit tests for highest-impact coverage gaps |
|
Hi! I'm ready to address review feedback on this PR. However, I don't see any review comments or requested changes yet. The PR appears to be freshly opened with CI checks still running. Once reviewers provide feedback (review comments, requested changes, or suggestions), I'll be happy to:
For now, let's wait for the CI checks to complete and for reviewers to provide their feedback. 🚀 |
|
🔍 Reviewing code — Examining the PR changes for quality and correctness... |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — well-structured test suite covering high-impact code paths across the agent pipeline, PM webhooks, triggers, and utilities. CI passes cleanly.
Minor Observations
Should Fix (non-blocking)
- tests/unit/agents/utils/setup.test.ts:8-10 — Dead mock at
../../src/utils/repo.jsdoes nothing. The comment on line 12 explains the correct mock is on line 13 (../../../../src/utils/repo.js), which matches how vitest resolves the relative import../../utils/repo.jsfrom the source module. The dead mock on line 8 should be removed to avoid confusion — it looks like a leftover from initial trial-and-error.
The tests are well-scoped, use appropriate mock granularity, correctly verify both happy and error paths, and the assertions match the actual source implementations I checked (llmEnv.ts, setup.ts, webhook-handler.ts). The helper factory functions (createBaseOptions, createMockAgent, createMockIntegration, etc.) keep the tests DRY without sacrificing readability.
|
|
||
| vi.mock('../../src/utils/repo.js', () => ({ | ||
| runCommand: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
Nit: This mock at ../../src/utils/repo.js is dead code — it doesn't match any actual module resolution path from the test file. The real mock on line 13 (../../../../src/utils/repo.js) is the one that works. Consider removing lines 8-10 to avoid confusion.
There was a problem hiding this comment.
Fixed — removed the dead mock at ../../src/utils/repo.js (lines 8-10) and the now-unnecessary comment on line 12. The correct mock at ../../../../src/utils/repo.js remains. All 25 tests in the file still pass. Pushed in fbc309d.
|
@aaight address code review concerns |
Request AddressedWhat was askedAddress code review concerns from @nhopeatall's review. Changes Made
Verification
|
Remove unused vi.mock at incorrect path '../../src/utils/repo.js' that doesn't match any actual module resolution path. The correct mock at '../../../../src/utils/repo.js' on the next line is the one that works. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🔍 Reviewing code — Examining the PR changes for quality and correctness... |
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM — well-targeted test coverage addition.
All 8 test files accurately reflect their source implementations. I verified the test assertions against the actual source code for agentLoop.ts, logging.ts, setup.ts, syntheticCalls.ts, builderFactory.ts, webhook-handler.ts, builtins.ts, and llmEnv.ts. Highlights:
- Mocking strategy is sound: Each file mocks only its direct dependencies, not transitive ones. The
as nevercasts are a pragmatic approach for avoiding full type construction of complex internal types. - Edge cases covered well:
truncateContentboundary at exactmaxLen,warmTypeScriptCachetreating tsc errors as success (matching the intentional design),injectLlmApiKeysrestore function preserving original vs undefined values,processPMWebhookerror recovery clearing the processing flag. - Assertions match actual behavior: Verified
registerBuiltInTriggerscount (16) and ordering against the source, budgetwithBudgetconditional (> 0, not just truthy) matches the source's!== undefined && > 0check, andwebhook-handlertests correctly model thehandleMatchedTrigger → resolveTriggerResultflow including pre-resolved result bypass.
CI passes (lint, typecheck, all 3,006 tests). No issues found.
Summary
Implements 8 new test files targeting the highest-impact coverage gaps in the codebase, adding 121 new unit tests across core agent pipeline, gadgets, PM webhooks, and utilities.
tests/unit/agents/utils/agentLoop.test.ts(NEW) — 28 tests forrunAgentLoopandtruncateContent: event processing (text, gadget_call, gadget_result, stream_complete), loop detection/hard-stop, interactive mode, session completion injection, gadget-specific log contexttests/unit/agents/utils/logging.test.ts(NEW) — 7 tests forcreateAgentLogger: dual-write to console + file logger, null file logger support, all log levelstests/unit/agents/utils/setup.test.ts(NEW) — 25 tests forgetLogLevel,readContextFiles,installDependencies,warmTypeScriptCache: env var precedence, package manager detection, missing files, TypeScript error tolerancetests/unit/agents/shared/syntheticCalls.test.ts(NEW) — 15 tests forinjectSyntheticCall,injectDirectoryListing,injectContextFiles,injectSquintContext: invocation ID tracking, sequential IDs, squint fallbacktests/unit/agents/shared/builderFactory.test.ts(NEW) — 18 tests forisSquintEnabledandcreateConfiguredBuilder: budget enforcement, BudgetPricingUnavailableError handling, skipSessionState, postConfigure callbacktests/unit/pm/webhook-handler.test.ts(NEW) — 14 tests forprocessPMWebhook: invalid payload, queuing when busy, no project found, duplicate card skipping, pre-resolved triggers, ackCommentId passthrough, error recoverytests/unit/triggers/builtins.test.ts(NEW) — 8 tests forregisterBuiltInTriggers: total count, registration order (comment-mention before card-moved, JIRA comment before transition), all trigger categories presenttests/unit/utils/llmEnv.test.ts(NEW) — 6 tests forinjectLlmApiKeys: env injection from DB, restore function removes injected key, restores original value, null credential handlingTest plan
npm run lintpasses cleanly (zero errors)npm run typecheckpasses cleanly (zero errors)Card: https://trello.com/c/699c9f643c77d7c420a98233
🤖 Generated with Claude Code