refactor: migrate integration tests to InMemoryRepository#462
refactor: migrate integration tests to InMemoryRepository#462carlos-alm merged 7 commits intomainfrom
Conversation
Expand the deferred Phase 3 items into a dedicated phase after TypeScript Migration with detailed descriptions for each sub-item: event-driven pipeline, unified engine strategy, subgraph export filtering, transitive confidence, query caching, config profiles, pagination standardization, and plugin system. Renumber subsequent phases 5-9 → 6-10 with all cross-references updated.
Add openRepo() utility that accepts an injected Repository instance or falls back to SQLite, enabling tests to bypass the filesystem entirely. - Add openRepo(dbPath, opts) to src/db/connection.js - Make findMatchingNodes and buildDependencyGraph polymorphic (accept db or Repository via instanceof check) - Refactor triageData, sequenceData, communitiesData to use openRepo - Convert triage, sequence, communities test fixtures to createTestRepo() fluent builder (sequence dataflow tests stay on SQLite) - Mark ROADMAP 3.13 InMemoryRepository migration item complete Impact: 10 functions changed, 11 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Migrate Integration Tests to InMemoryRepositoryReview Checklist:
Root Cause AnalysisProblem Being Solved: This PR addresses ROADMAP 3.13 — migrating integration tests from manual SQLite setup to Why This Approach: The approach is well-justified. It introduces a clean Risk Assessment: Low risk. The changes are purely additive with fallback behavior. No breaking changes to existing APIs. The Configuration Change Detection ✅No high-risk configuration changes detected. No changes to biome.json, package.json, vitest config, or other build/lint configurations. This is a pure refactor without any rule relaxation or threshold lowering. Test Integrity Analysis ✅Test rigor is maintained or improved:
Code Quality Review ✅Architecture & Design:
Implementation Quality:
No Security/Performance Concerns:
Backlog Compliance AssessmentZero-dep: ✅ — No new runtime dependencies added. Pure refactoring of existing code. Foundation-aligned: ✅ — Fully compliant with all core principles:
Problem-fit: 4/5 — This directly improves the codebase's maintainability and test reliability, which reduces development friction. While not directly addressing the "lost AI" problem, it enables faster iteration on features that do address it. Strong foundation work that supports the primary mission. Breaking: No — Purely additive changes. All existing call sites continue to work identically. New Tier: 1 (Zero-dep ✅ + Foundation-aligned ✅) Critical ConcernsNone. This is a well-executed refactor that improves code quality without introducing risks. Final Recommendation
The one point deducted is for being "good foundation work" rather than "exceptional innovation" — this is solid engineering that moves the project forward systematically. |
Greptile SummaryThis PR completes a repository-pattern migration by introducing Key design decisions land cleanly:
One minor observation: the Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as CLI / MCP
participant Feature as Feature (triage/sequence/communities)
participant openRepo as openRepo()
participant Repo as Repository (InMemory or Sqlite)
participant DB as SQLite DB
Caller->>Feature: featureData(dbPath, opts)
Feature->>openRepo: openRepo(dbPath, opts)
alt opts.repo is Repository instance
openRepo-->>Feature: { repo: opts.repo, close: noop }
else opts.repo is non-null but not Repository
openRepo-->>Caller: TypeError
else no opts.repo
openRepo->>DB: openReadonlyOrFail(dbPath)
openRepo-->>Feature: { repo: new SqliteRepository(db), close: db.close }
end
Feature->>Repo: findNodes / getEdges / etc.
Repo-->>Feature: in-memory data
Feature->>Feature: close()
Feature-->>Caller: result
Last reviewed commit: ff74ffb |
| if (minConfidence != null) { | ||
| edges = db | ||
| .prepare("SELECT source_id, target_id FROM edges WHERE kind = 'calls' AND confidence >= ?") | ||
| .all(minConfidence); | ||
| if (isRepo) { | ||
| // minConfidence filtering not supported by Repository — fall back to getCallEdges | ||
| edges = dbOrRepo.getCallEdges(); | ||
| } else { | ||
| edges = dbOrRepo | ||
| .prepare("SELECT source_id, target_id FROM edges WHERE kind = 'calls' AND confidence >= ?") | ||
| .all(minConfidence); | ||
| } |
There was a problem hiding this comment.
minConfidence silently ignored for Repository
When a Repository is passed and minConfidence is set, the filter is silently dropped — getCallEdges() returns all call edges regardless of confidence. This means callers like viewer.js and export.js that pass minConfidence would get incorrect (unfiltered) graphs if they were ever migrated to use the Repository path.
At minimum, a warn() log would make this visible. Better would be to add a getCallEdgesAboveConfidence(min) method to the Repository contract, or post-filter the results:
| if (minConfidence != null) { | |
| edges = db | |
| .prepare("SELECT source_id, target_id FROM edges WHERE kind = 'calls' AND confidence >= ?") | |
| .all(minConfidence); | |
| if (isRepo) { | |
| // minConfidence filtering not supported by Repository — fall back to getCallEdges | |
| edges = dbOrRepo.getCallEdges(); | |
| } else { | |
| edges = dbOrRepo | |
| .prepare("SELECT source_id, target_id FROM edges WHERE kind = 'calls' AND confidence >= ?") | |
| .all(minConfidence); | |
| } | |
| if (minConfidence != null) { | |
| if (isRepo) { | |
| // minConfidence filtering not yet in Repository — filter in JS | |
| edges = dbOrRepo.getCallEdges().filter((e) => e.confidence == null || e.confidence >= minConfidence); | |
| } else { | |
| edges = dbOrRepo | |
| .prepare("SELECT source_id, target_id FROM edges WHERE kind = 'calls' AND confidence >= ?") | |
| .all(minConfidence); | |
| } |
Note: this suggestion requires getCallEdges() to include a confidence field in its return shape, which would need a small change to InMemoryRepository.getCallEdges() and SqliteRepository.getCallEdges().
There was a problem hiding this comment.
Fixed — when a Repository is passed and minConfidence is set, call edges are now post-filtered in JS: .filter(e => e.confidence != null && e.confidence >= minConfidence). Updated getCallEdges() in all three layers (SqliteRepository, InMemoryRepository, base JSDoc) to include the confidence field in the return shape.
src/features/sequence.js
Outdated
| if (hasTable && repo.db) { | ||
| const db = repo.db; |
There was a problem hiding this comment.
Dataflow path bypasses Repository abstraction
The dataflow annotation block relies on repo.db to access raw SQLite, falling back to skip the block for InMemoryRepository (where repo.db is undefined). While SqliteRepository intentionally exposes get db() as an escape hatch, this creates a fragile implicit contract: any future Repository subclass that happens to define a .db property for unrelated reasons would accidentally enter the raw-SQL dataflow block.
A more robust guard would be an explicit type-check or a dedicated Repository method, for example:
// Instead of:
if (hasTable && repo.db) {
// Consider:
if (hasTable && repo instanceof SqliteRepository) {Or, alternatively, expose a getDataflowStatements() method on SqliteRepository that returns the prepared statements (returning null from the base class), so no raw db access is needed at the call site. This is low priority given the existing JSDoc comment on get db(), but worth tracking as dataflow support expands to other Repository implementations.
There was a problem hiding this comment.
Fixed — replaced the fragile repo.db check with an explicit instanceof SqliteRepository guard.
| export function openRepo(customDbPath, opts = {}) { | ||
| if (opts.repo instanceof Repository) { | ||
| return { repo: opts.repo, close() {} }; | ||
| } |
There was a problem hiding this comment.
Silent fallthrough for non-Repository opts.repo
If opts.repo is provided but is not an instanceof Repository (e.g. a plain mock object, a wrong class, or an accidentally undefined value from a destructuring default), the function silently falls through to openReadonlyOrFail(customDbPath). The caller may not realize the injected repo was ignored.
Consider adding an explicit guard to surface the mismatch early:
| export function openRepo(customDbPath, opts = {}) { | |
| if (opts.repo instanceof Repository) { | |
| return { repo: opts.repo, close() {} }; | |
| } | |
| export function openRepo(customDbPath, opts = {}) { | |
| if (opts.repo != null) { | |
| if (!(opts.repo instanceof Repository)) { | |
| throw new TypeError( | |
| `openRepo: opts.repo must be a Repository instance, got ${Object.prototype.toString.call(opts.repo)}`, | |
| ); | |
| } | |
| return { repo: opts.repo, close() {} }; | |
| } | |
| const db = openReadonlyOrFail(customDbPath); | |
| return { | |
| repo: new SqliteRepository(db), | |
| close() { | |
| db.close(); | |
| }, | |
| }; | |
| } |
This makes the contract explicit and prevents hard-to-debug test failures where a repo is passed but ignored.
There was a problem hiding this comment.
Fixed — openRepo now throws a TypeError if opts.repo is provided but is not a Repository instance.
Impact: 2 functions changed, 18 affected
Add new Phase 4 covering the port of JS-only build phases to Rust: - 4.1-4.3: AST nodes, CFG, dataflow visitor ports (~587ms savings) - 4.4: Batch SQLite inserts (~143ms) - 4.5: Role classification & structure (~42ms) - 4.6: Complete complexity pre-computation - 4.7: Fix incremental rebuild data loss on native engine - 4.8: Incremental rebuild performance (target sub-100ms) Bump old Phases 4-10 to 5-11 with all cross-references updated. Benchmark evidence shows ~50% of native build time is spent in JS visitors that run identically on both engines.
Keep Phase 4 (Native Analysis Acceleration) numbering from PR branch, incorporate 'DX & onboarding' addition and improved deliverable wording from main.
Summary
openRepo(dbPath, opts)utility insrc/db/connection.jsthat accepts an injectedRepositoryinstance viaopts.repoor falls back to opening a readonly SQLite DB wrapped inSqliteRepositoryfindMatchingNodesandbuildDependencyGraphpolymorphic — accept either a rawdbor aRepositoryviainstanceofchecktriageData,sequenceData, andcommunitiesDatato useopenRepo(), enabling Repository injectioncreateTestRepo()fluent builder (sequence dataflow tests remain on SQLite sinceInMemoryRepositoryhas no dataflow table)Test plan
npx vitest run tests/integration/triage.test.js— 20 tests passnpx vitest run tests/integration/sequence.test.js— 16 tests passnpx vitest run tests/integration/communities.test.js— 13 tests passnpm test— full suite 1853 tests pass, 0 regressionsnpm run lint— clean