test(config): add integration tests for config provider round-trip#919
test(config): add integration tests for config provider round-trip#919
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
The tests are correct and all CI checks pass. However, there is significant overlap with the existing tests/integration/db/configRepository.test.ts file and a few structural issues worth addressing.
Architecture & Design
-
[SHOULD_FIX] Substantial duplication with existing test file:
tests/integration/db/configRepository.test.tsalready coversfindProjectByBoardIdFromDb(positive + negative),findProjectByRepoFromDb(positive + negative),findProjectByIdFromDb(positive + negative),findProjectByJiraProjectKeyFromDb(positive + negative),findProjectWithConfigByBoardId(positive + negative),loadConfigFromDb(single, multi-project, trello integration), and multi-project config loading. The new file re-tests all of these. Consider extending the existing file with the genuinely new coverage (multi-project discrimination,WithConfigvariants for repo/id/jira,CascadeConfigSchema.safeParsevalidation, cross-provider isolation) rather than creating a parallel test file that duplicates ~15 of the ~30 test cases. -
[SHOULD_FIX] File placement and naming mismatch: The file is named
config-provider.test.tsand placed at the top-leveltests/integration/directory, but it exclusively testsconfigRepository.tsfunctions — not the provider layer (src/config/provider.ts). The only provider import isinvalidateConfigCache(), which is trivially used. This is misleading — a developer looking for config provider integration tests would expect to find tests for the cachedfindProjectByBoardId,loadConfig, etc. fromprovider.ts. The repository lookup tests belong alongside the existingtests/integration/db/configRepository.test.ts.
Code Issues
Should Fix
- tests/integration/config-provider.test.ts:530-548 — The cache invalidation test doesn't actually test caching behavior. It calls
findProjectByBoardIdFromDbwhich is a direct DB function that always queries PostgreSQL — it never reads from or writes to the config cache. CallinginvalidateConfigCache()between two direct DB calls has no observable effect — both calls will always hit the DB regardless. To meaningfully test cache invalidation, the test should use the cached provider functions (findProjectByBoardIdfromsrc/config/provider.ts), populate the cache, mutate the DB, then verifyinvalidateConfigCache()causes a fresh read.
Inline details
tests/integration/config-provider.test.ts:1 — This file is named config-provider.test.ts but tests configRepository.ts functions exclusively. The only provider-layer import is invalidateConfigCache(). Meanwhile, tests/integration/db/configRepository.test.ts already tests many of the same repository functions with identical scenarios (positive lookup, negative lookup, multi-project). Consider either: (1) merging the genuinely new test cases into the existing configRepository.test.ts, or (2) renaming this file and focusing it on the actual provider layer (cached lookups, cache invalidation with mutation).
tests/integration/config-provider.test.ts:530 — This test does not exercise cache behavior. findProjectByBoardIdFromDb is a direct DB function that always queries PostgreSQL — it never reads from or writes to the config cache. Calling invalidateConfigCache() between two findProjectByBoardIdFromDb calls has no observable effect. To test cache invalidation meaningfully, use the cached provider functions (e.g., findProjectByBoardId from src/config/provider.ts), verify a cached result, mutate the underlying data, call invalidateConfigCache(), and then verify the fresh result reflects the mutation.
🕵️ claude-code · claude-opus-4-6 · run details
- Rewrite config-provider.test.ts to focus on the actual provider layer (cached findProjectByBoardId, findProjectByRepo, findProjectByJiraProjectKey, loadConfig from src/config/provider.ts) rather than duplicating direct DB repository tests - Fix cache invalidation test to use cached provider functions, mutate the DB between calls, and verify invalidateConfigCache() causes a fresh read - Move genuinely new test coverage into configRepository.test.ts: WithConfig variants for repo/id/jira, CascadeConfigSchema.safeParse validation, cross-provider isolation, Trello+JIRA coexistence, and JIRA config loading Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the detailed review @nhopeatall! All three issues have been addressed: File placement and naming mismatch — Duplication with existing test file — The ~15 duplicate repository-level tests have been removed from Cache invalidation test — Fixed. The test now:
🕵️ claude-code · claude-sonnet-4-6 · run details |
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM — Well-structured integration tests with thorough coverage of the config provider caching layer and configRepository lookup functions.
The tests correctly verify:
- Cache hit/miss/invalidation semantics against the actual
ConfigCacheimplementation (null = miss, undefined = not-found) - JSONB sub-query provider filtering (boardId lookup doesn't match JIRA rows and vice versa)
WithConfigvariants return valid{ project, config }pairs that passCascadeConfigSchema.safeParse()- Multi-provider coexistence (Trello + JIRA projects in the same org)
loadConfigFromDbround-trip throughvalidateConfig()
Test isolation is properly handled via truncateAll() + invalidateConfigCache() in beforeEach, and seed helper defaults are used consistently. CI is green across all checks.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
tests/integration/config-provider.test.tswith 30 integration tests covering allconfigRepository.tslookup functionsfindProjectByBoardIdFromDb,findProjectByJiraProjectKeyFromDb) against real PostgreSQL datafindProjectByRepoFromDb,findProjectByIdFromDb)WithConfigvariants return valid{ project, config }pairs that passvalidateConfig()schemaloadConfigFromDboutput passesCascadeConfigSchema.safeParse()invalidateConfigCache()tested inbeforeEachCard
https://trello.com/c/QmSNP14Z/424-as-a-developer-i-want-integration-tests-for-config-provider-round-trip-so-that-jsonb-sub-queries-and-withconfig-lookups-are-veri
Test plan
findProjectByBoardIdFromDb— known boardId returns project; unknown returns undefined; multi-project returns correct projectfindProjectByJiraProjectKeyFromDb— known key returns project; unknown returns undefined; multi-project returns correct projectfindProjectByRepoFromDb— known repo returns project; unknown returns undefined; multi-project correctfindProjectByIdFromDb— known id returns project; unknown returns undefinedfindProjectWithConfigByBoardId— returns{ project, config }pair; project has required fields; config passesCascadeConfigSchemafindProjectWithConfigByRepo,findProjectWithConfigById,findProjectWithConfigByJiraProjectKey— round-trip verifiedloadConfigFromDb— loads all projects; Trello/JIRA configs embedded; passesvalidateConfig()🤖 Generated with Claude Code
🕵️ claude-code · claude-sonnet-4-6 · run details