Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Solid, well-structured integration test suite. The 33 tests are thorough and well-organized, covering CRUD, upsert semantics, trigger deep-merge, credential role mapping, JSONB round-trips, and unique constraint enforcement. No blocking issues.
Should Fix
- tests/integration/db/integrationsRepository.test.ts:10 —
deleteProjectCredentialis imported fromcredentialsRepositorybut never used in any test. Should be removed to keep imports clean.
Notes
- There is intentional overlap with
repositories-edge-cases.test.ts(upsert insert/update, trigger merge, unique constraint). Since the edge-cases file imports via the barrelsettingsRepository.tswhich re-exportsintegrationsRepository.ts, these test the same code paths. The new file adds genuinely new coverage (deleteProjectIntegration,removeIntegrationCredentialrole mapping for all providers, JSONB config round-trip, CRUD lifecycle), so the overlap is acceptable — but worth noting that ~8 tests duplicate scenarios already covered. - Test quality is high: proper isolation via
truncateAll()+beforeEach, good negative cases (no-op on missing data, project isolation), and assertions verify both the changed and unchanged state (e.g., checking other credentials remain after removing one).
🕵️ claude-code · claude-opus-4-6 · run details
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — well-structured, comprehensive integration test suite for integrationsRepository that fills genuine coverage gaps (especially deleteProjectIntegration, removeIntegrationCredential role mappings, and getIntegrationByProjectAndCategory isolation). All CI checks pass.
Minor Observations
Test overlap with repositories-edge-cases.test.ts
The upsertProjectIntegration (insert/update/preserve triggers), updateProjectIntegrationTriggers (flat merge, nested merge, throws-when-missing), and unique constraint tests are already covered in repositories-edge-cases.test.ts. This isn't blocking — having a dedicated, focused test file for the module is the right direction — but worth being aware of for future cleanup. If the intent is to eventually make integrationsRepository.test.ts the canonical test location for these functions, consider removing the duplicated scenarios from repositories-edge-cases.test.ts in a follow-up to reduce integration test runtime.
upsertProjectIntegration type signature vs nested triggers
The triggers parameter of upsertProjectIntegration is typed as Record<string, boolean>, but the nested trigger tests (e.g., line ~290: { issueTransitioned: { splitting: true, planning: true, implementation: true } }) pass objects where values are Record<string, boolean> rather than boolean. This works at runtime because JSONB accepts any JSON, and it isn't flagged by CI because tsconfig.json scopes typecheck to src/ only. Not a problem introduced by this PR (the same pattern exists in repositories-edge-cases.test.ts), but the source type could be widened to Record<string, unknown> to match reality.
Neither observation blocks merge.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
tests/integration/db/integrationsRepository.test.ts— 33 integration tests for theintegrationsRepositorymodulerepositories-edge-cases.test.ts:listProjectIntegrations,getIntegrationByProjectAndCategory,deleteProjectIntegration, andremoveIntegrationCredentialremoveIntegrationCredentialfor all providers (Trello, JIRA, GitHub)Card: https://trello.com/c/69b8004910366990f4ee4c95
Test plan
listProjectIntegrations— empty list, multi-integration list, project isolationgetIntegrationByProjectAndCategory— null when missing, correct lookup, category isolation, project isolationupsertProjectIntegration— insert new, update on conflict, preserve triggers when omitted, explicit trigger set, returns rowdeleteProjectIntegration— removes correct integration, no-op when absent, doesn't affect other projectsupdateProjectIntegrationTriggers— flat key merge, nested object merge, add new keys, throws when integration absentremoveIntegrationCredential— mapsapi_key→TRELLO_API_KEY,token→TRELLO_TOKEN,email→JIRA_EMAIL,api_token→JIRA_API_TOKEN,implementer_token→GITHUB_TOKEN_IMPLEMENTER; no-op for unknown role; no-op for missing integration id🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details