-
Notifications
You must be signed in to change notification settings - Fork 12
fix: classify test-only-called symbols as 'test-only' instead of 'dead' #497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a196f3c
f4c8176
0f430ab
f199a68
43215be
e0afd0f
5d6d64c
25a2582
da486c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { openReadonlyOrFail } from '../../db/index.js'; | ||
| import { warn } from '../../infrastructure/logger.js'; | ||
| import { isTestFile } from '../../infrastructure/test-filter.js'; | ||
| import { normalizeSymbol } from '../../shared/normalize.js'; | ||
| import { paginateResult } from '../../shared/paginate.js'; | ||
|
|
@@ -13,9 +14,14 @@ export function rolesData(customDbPath, opts = {}) { | |
| const conditions = ['role IS NOT NULL']; | ||
| const params = []; | ||
|
|
||
| if (filterRole) { | ||
| // When noTests + filtering for 'dead', also include 'test-only' candidates | ||
| // (they are stored as non-dead roles but may need reclassification) | ||
| 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 | ||
| } | ||
| if (filterFile) { | ||
| conditions.push('file LIKE ?'); | ||
|
|
@@ -28,7 +34,27 @@ export function rolesData(customDbPath, opts = {}) { | |
| ) | ||
| .all(...params); | ||
|
|
||
| if (noTests) rows = rows.filter((r) => !isTestFile(r.file)); | ||
| if (noTests) { | ||
| rows = rows.filter((r) => !isTestFile(r.file)); | ||
| } | ||
|
|
||
| if (noTests || filterRole === 'test-only') { | ||
| // Reclassify symbols whose only callers are in test files as 'test-only'. | ||
| // A symbol that has fanIn > 0 at build time (all edges) but fanIn === 0 | ||
| // when test-file callers are excluded should be 'test-only' instead of | ||
| // whatever role it was assigned with the full graph. | ||
| const testOnlyIds = _findTestOnlyCalledIds(db); | ||
| for (const r of rows) { | ||
| if (testOnlyIds.has(`${r.name}\0${r.file}\0${r.line}`)) { | ||
| r.role = 'test-only'; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If we were asked for a specific role, filter now (after reclassification) | ||
| if (filterRole) { | ||
| rows = rows.filter((r) => r.role === filterRole); | ||
| } | ||
|
|
||
| const summary = {}; | ||
| for (const r of rows) { | ||
|
|
@@ -43,3 +69,58 @@ export function rolesData(customDbPath, opts = {}) { | |
| db.close(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Find node keys (name\0file\0line) for symbols whose callers are ALL in test files. | ||
| * These symbols have fanIn > 0 in the full graph but would have fanIn === 0 | ||
| * if test-file edges were excluded. | ||
| */ | ||
| function _findTestOnlyCalledIds(db) { | ||
| // Check whether any test-file nodes exist in the graph at all. | ||
| // If the graph was built with -T (excluding test files), there will be | ||
| // no test callers and the query below would silently return nothing. | ||
| const files = db.prepare(`SELECT DISTINCT file FROM nodes WHERE kind = 'file'`).all(); | ||
| const hasTestNodes = files.some((r) => isTestFile(r.file)); | ||
| if (!hasTestNodes) { | ||
| warn( | ||
| 'No test file nodes found in the graph — run "codegraph build" without -T to enable test-only detection', | ||
| ); | ||
| return new Set(); | ||
| } | ||
|
|
||
| // 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(); | ||
|
Comment on lines
+78
to
+102
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5d6d64c — added an early check in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — added a guard at the top of |
||
|
|
||
| // Group callers by target symbol | ||
| const callersByTarget = new Map(); | ||
| for (const r of rows) { | ||
| const key = `${r.name}\0${r.file}\0${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; | ||
|
Comment on lines
+78
to
+125
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The composite key While in practice file paths on Linux/macOS rarely contain
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in f199a68 — switched the composite key separator from |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--role test-onlysilently returns empty without-TWhen
filterRole === 'test-only'butnoTestsisfalse, the code fetches all symbols (correct), skips the reclassification block (becauseif (noTests)is false), then filters torole === '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-onlywithout-Twill getcount: 0, symbols: []and have no indication that the flag is required for this role to work. Consider either:noTestswhenfilterRole === 'test-only'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f4c8176 — the reclassification block now runs whenever \ OR \ is true, so \ without \ no longer returns an empty set silently.