feat(tests): add integration test suites for all DB repositories#549
Conversation
|
✨ On it — checking the PR for feature/integration-test-suites-db-repositories Progress: [███░░░░░░░] 34% (iteration 24/70) 🔍 Code Review Update (1 min) I've completed a thorough review of the database migrations, confirming that the Last updated: iteration 24 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM — Well-structured integration test suite that follows the established patterns from credentialsRepository.test.ts.
Notes
Migration approach is sound: The 0000_base_schema.sql correctly uses CREATE TABLE IF NOT EXISTS / CREATE ... INDEX IF NOT EXISTS, making it idempotent — safe both for fresh databases (integration tests) and existing databases where these tables already exist. The timestamp ordering (1735000000000 before 1736000000000) ensures the bootstrap-journal script skips it on already-migrated databases.
Test quality: Tests cover CRUD, edge cases (null/missing data, wrong project isolation), upsert semantics, pagination, and cross-cutting concerns like encryption round-trips and credential resolution priority. The beforeEach(truncateAll → seed) pattern matches the existing credentialsRepository.test.ts convention.
Minor observations (not blocking):
seedRunis imported but never used inrunsRepository.test.ts(tests usecreateRundirectly) — unused importseedPrWorkItemandseedRunLogsare defined inseed.tsbut not used by any test file yet — dead code for now, but reasonable to include as helpers for future tests
|
@aaight address code review concerns |
Request AddressedWhat was askedAddress code review concerns raised by @nhopeatall. Changes Made
Verification
|
Remove unused `seedRun` import from runsRepository.test.ts, and remove unused `seedPrWorkItem` and `seedRunLogs` helpers from seed.ts along with their now-unnecessary schema imports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the code review concerns from @nhopeatall:
All checks pass (3198 unit tests, lint, typecheck). |
|
🤖 Just a sec, looking into PR #549 now Progress: [████░░░░░░] 37% (iteration 26/70) 🔍 Code Review Update (1 min) I've completed reviewing the migration files and confirmed that the Last updated: iteration 26 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Well-structured integration test suite that follows existing patterns precisely, with a sound approach to the base schema migration.
Migration Safety Verified
The 0000_base_schema.sql insertion into the journal is safe for existing databases. I traced through drizzle-orm's migrator (pg-core/dialect.cjs:64): it applies migrations by checking Number(lastDbMigration.created_at) < migration.folderMillis. For production databases with migrations up to 0015 (timestamp 1750000000000), the base schema's 1735000000000 is below the watermark and will be skipped. For fresh databases, CREATE TABLE IF NOT EXISTS provides additional idempotency. The re-indexing of idx values is cosmetic from drizzle's perspective — the migrator uses when/created_at for ordering, not idx.
Test Quality
- Consistent structure: All 8 new test files follow the established pattern from
credentialsRepository.test.ts(truncateAll+seedOrg/seedProjectinbeforeEach, section headers with=====, testing both happy paths and edge cases). - Good coverage of domain invariants: Tests verify cross-project isolation (
prWorkItemsRepository), config inheritance hierarchy (configRepository), credential override precedence (credentialResolution), and session expiry behavior (usersRepository). - Seed helpers are well-factored: Each new seed function follows the
overridespattern established by the existing helpers, with sensible defaults. vi.stubEnvusage is correct: Vitest'sunstubEnvs: truehandles cleanup, and the credential encryption round-trip tests properly validate the encrypt/decrypt path.
Minor note (not blocking)
The PR description lists seedPrWorkItem and seedRunLogs as new seed functions, but these don't exist in the code — the tests use the repository functions directly for those operations, which is fine.
Summary
configRepository,runsRepository,settingsRepository,webhookLogsRepository,prWorkItemsRepository,partialsRepository,usersRepositorycredentialResolution.test.ts) that testsgetAllProjectCredentials()end-to-end with real DB, integration credential overrides, and encryption round-tripstests/integration/helpers/seed.ts) with 7 new functions:seedDefaults,seedAgentConfig,seedRun,seedUser,seedWebhookLog,seedPromptPartial,seedPrWorkItem,seedSession,seedRunLogs0000_base_schema.sqlmigration to enable running the full migration chain from a fresh database (required forrunMigrations()in integration test setup)Total: 155 integration tests (8 previously from
credentialsRepository.test.ts, plus 147 new)Key decisions
vi.stubEnv()for environment variable manipulation in tests (vitest'sunstubEnvs: truecleans up automatically), avoiding the biomenoDeletelint rule while ensuring proper cleanup0000_base_schema.sqlas the initial schema since the existing migrations (starting at 0001) assume a pre-existingprojectstable from when the DB was originally created withdrizzle-kit push1735000000000(before 0001's1736000000000) so existing databases (which have already had migrations applied) won't re-run itTest plan
runMigrations()Card
https://trello.com/c/3KLGEu9q/118-add-key-missing-integration-test-suites-for-db-repositories
🤖 Generated with Claude Code