diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca5b9ff1..eaa773d40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixes + +- Calls from your code no longer resolve into your tests. Tests depend on the code they exercise, never the other way round, but a name with no real definition to bind to — a value destructured from a hook, a helper that lives in a package — would land on any same-named helper inside a test file. On one project every `t(...)` call across 137 page components pointed at a translation stub defined in a single test, making it the second most-called symbol in the codebase and its callers and impact almost entirely fictional. Test files are recognised by the names their test runner enforces (`test_*.py`, `*.test.ts`, `*_test.go`, `*Test.java`), never by which folder they sit in, so a `spec/` directory holding an API document or a `testing/` utility library is left alone. Set `CODEGRAPH_TEST_TREE_GATE=0` to restore the old behaviour. + ## [1.6.0] - 2026-08-26 diff --git a/__tests__/resolution.test.ts b/__tests__/resolution.test.ts index decaadee5..37637b1e6 100644 --- a/__tests__/resolution.test.ts +++ b/__tests__/resolution.test.ts @@ -11,7 +11,7 @@ import * as os from 'os'; import { CodeGraph } from '../src'; import { Node, UnresolvedReference } from '../src/types'; import { ReferenceResolver, createResolver, ResolutionContext } from '../src/resolution'; -import { matchReference, resolveMethodOnType, matchByQualifiedName, preferCallSiteFile, matchMethodCall } from '../src/resolution/name-matcher'; +import { matchReference, resolveMethodOnType, matchByQualifiedName, preferCallSiteFile, matchMethodCall, isRunnerNamedTestFile } from '../src/resolution/name-matcher'; import { resolveImportPath, extractImportMappings, resolveJvmImport, loadCppIncludeDirs, clearCppIncludeDirCache, isPhpIncludePathRef } from '../src/resolution/import-resolver'; import type { UnresolvedRef } from '../src/resolution/types'; import { detectFrameworks, getAllFrameworkResolvers } from '../src/resolution/frameworks'; @@ -36,6 +36,174 @@ describe('Resolution Module', () => { } }); + + describe('Production code never resolves into a test file', () => { + // Tests depend on the code they exercise; the code never depends on its + // tests. A production symbol resolving INTO a test file is therefore always + // wrong — and it happens constantly, because the names that reach the + // name-matching fallback are precisely the ones with no real definition to + // bind to. + const node = (over: Partial): Node => ({ + id: 'n', kind: 'function', name: 'x', qualifiedName: 'x', + filePath: 'a.ts', language: 'typescript', + startLine: 1, endLine: 2, startColumn: 0, endColumn: 0, + updatedAt: Date.now(), ...over, + }); + const contextOf = (nodes: Node[]): ResolutionContext => ({ + getNodesInFile: () => nodes, + getNodesByName: (name: string) => nodes.filter((n) => n.name === name), + getNodesByQualifiedName: () => [], + getNodesByKind: () => [], + getNodesByLowerName: (lower: string) => + nodes.filter((n) => n.name.toLowerCase() === lower), + getImportMappings: () => [], + fileExists: () => true, + readFile: () => null, + getProjectRoot: () => '/test', + getAllFiles: () => nodes.map((n) => n.filePath), + } as any); + const callFrom = (filePath: string, name: string, language = 'tsx') => ({ + fromNodeId: `caller:${filePath}:c:1`, + referenceName: name, + referenceKind: 'calls' as const, + line: 5, column: 0, filePath, language: language as any, + }); + + it('declines a component call whose only candidate is a test stub', () => { + // `const { t } = useTranslation()` binds `t` from a package, so there is + // no local definition and no exported one — the only same-named symbol + // anywhere is a stub inside a test. + const nodes = [node({ + id: 'stub:t', name: 't', qualifiedName: 'merge-dialog.test.tsx::t', + filePath: 'src/page-tests/merge-dialog.test.tsx', language: 'tsx', + })]; + expect(matchReference(callFrom('src/pages/Planner.tsx', 't'), contextOf(nodes))) + .toBeNull(); + }); + + it('prefers the production definition when both exist', () => { + const nodes = [ + node({ id: 'test:format', name: 'format', qualifiedName: 'x.test.ts::format', + filePath: 'src/lib/x.test.ts' }), + node({ id: 'prod:format', name: 'format', qualifiedName: 'fmt.ts::format', + filePath: 'src/lib/fmt.ts' }), + ]; + const result = matchReference(callFrom('src/pages/Row.tsx', 'format'), contextOf(nodes)); + expect(result?.targetNodeId).toBe('prod:format'); + }); + + it('leaves a call made BY a test alone', () => { + // Tests calling shared fixtures is the normal direction. + const nodes = [node({ + id: 'stub:renderPage', name: 'renderPage', + qualifiedName: 'helpers.test.ts::renderPage', + filePath: 'src/page-tests/helpers.test.ts', + })]; + const result = matchReference( + callFrom('src/page-tests/planner.test.tsx', 'renderPage'), contextOf(nodes)); + expect(result?.targetNodeId).toBe('stub:renderPage'); + }); + + it('identifies a test by the name its runner enforces, not by its directory', () => { + // Which directories hold tests is a project's own decision — `spec/` is an + // OpenAPI document in one repo and RSpec in another, and `testing/` is + // often a shipped utility library. Deleting edges on that guess would be + // silently wrong, so only runner-enforced filenames count. + expect(isRunnerNamedTestFile('src/lib/panel.test.ts')).toBe(true); + expect(isRunnerNamedTestFile('tests/test_collect.py')).toBe(true); + expect(isRunnerNamedTestFile('internal/server_test.go')).toBe(true); + expect(isRunnerNamedTestFile('src/main/java/app/OrderTest.java')).toBe(true); + expect(isRunnerNamedTestFile('spec/models/user_spec.rb')).toBe(true); + + // Directory alone is never enough. + expect(isRunnerNamedTestFile('spec/openapi.yaml')).toBe(false); + expect(isRunnerNamedTestFile('src/testing/harness.ts')).toBe(false); + expect(isRunnerNamedTestFile('tests/helpers.ts')).toBe(false); + // And a lowercase "test" inside an ordinary word is not a match. + expect(isRunnerNamedTestFile('src/latest.ts')).toBe(false); + expect(isRunnerNamedTestFile('src/manifest.ts')).toBe(false); + expect(isRunnerNamedTestFile('src/protest.py')).toBe(false); + + // The bare CamelCase suffix belongs to the JVM/.NET/Swift runners that + // collect on it. vitest and jest do not — they want `.test.`/`.spec.` — + // so `useTests.ts` is an ordinary hook and must stay resolvable. + expect(isRunnerNamedTestFile('src/hooks/useTests.ts')).toBe(false); + expect(isRunnerNamedTestFile('src/RequestSpec.tsx')).toBe(false); + expect(isRunnerNamedTestFile('app/OrderTests.java')).toBe(true); + expect(isRunnerNamedTestFile('app/OrderTest.kt')).toBe(true); + }); + + it('applies to a method resolved through an inferred receiver type', () => { + // This path reaches a candidate through a receiver type rather than a + // bare name, but fails the same way: an unpinned receiver settles for a + // same-named method, and a test class's method is as good a match as any. + const nodes = [node({ + id: 'test:say', kind: 'method', name: 'say', + qualifiedName: 'Reporter::say', filePath: 'src/tests/test_report.py', + language: 'python', + })]; + const ctx = contextOf(nodes); + const ref = { ...callFrom('src/tools/collect.py', 'say', 'python') }; + expect(resolveMethodOnType('Reporter', 'say', ref as any, ctx, 0.9, 'instance-method')) + .toBeNull(); + // A test calling the same method still resolves. + const fromTest = { ...callFrom('src/tests/test_collect.py', 'say', 'python') }; + expect(resolveMethodOnType('Reporter', 'say', fromTest as any, ctx, 0.9, 'instance-method') + ?.targetNodeId).toBe('test:say'); + }); + + it('applies to the receiver-name overlap fallback, which needs it most', () => { + // That strategy scores a receiver NAME against a class name, so a short + // receiver (`rep.say()`) matches a test's stand-in class (`_Rep`) on one + // shared word — and stand-ins are precisely the classes that share a name + // with the real collaborator they replace. + const nodes = [node({ + id: 'test:_Rep.say', kind: 'method', name: 'say', + qualifiedName: 'test_collect::_Rep::say', + filePath: 'src/tests/test_collect.py', language: 'python', + })]; + const ref = { ...callFrom('src/tools/answer.py', 'rep.say', 'python') }; + expect(matchMethodCall(ref as any, contextOf(nodes))).toBeNull(); + }); + + it('can be switched off for a project that relies on the old behaviour', async () => { + // The gate removes edges, so it carries the same escape hatch the + // ambiguity ceiling does. Read at module load, hence the reimport. + const prev = process.env.CODEGRAPH_TEST_TREE_GATE; + process.env.CODEGRAPH_TEST_TREE_GATE = '0'; + try { + vi.resetModules(); + const fresh = await import('../src/resolution/name-matcher'); + const nodes = [node({ + id: 'stub:t', name: 't', qualifiedName: 'merge.test.tsx::t', + filePath: 'src/page-tests/merge.test.tsx', language: 'tsx', + })]; + expect(fresh.matchReference( + callFrom('src/pages/Planner.tsx', 't') as any, contextOf(nodes)) + ).not.toBeNull(); + } finally { + if (prev === undefined) delete process.env.CODEGRAPH_TEST_TREE_GATE; + else process.env.CODEGRAPH_TEST_TREE_GATE = prev; + vi.resetModules(); + } + }); + + it('declines rather than falling back when the only candidate is a test', () => { + // The fabricated edges are precisely the ones with no production + // alternative, so a "keep it if nothing else matches" fallback would + // leave the entire defect in place. Production cannot depend on a test + // file — it would not build — so no match is the truthful answer. + const nodes = [node({ + id: 'only:assertGolden', name: 'assertGolden', + qualifiedName: 'golden.test.ts::assertGolden', + filePath: 'src/lib/golden.test.ts', + })]; + expect(matchReference( + callFrom('src/lib/report.ts', 'assertGolden', 'typescript'), contextOf(nodes))) + .toBeNull(); + }); + }); + describe('Name Matcher', () => { it('should match exact name references', () => { // Create a mock context diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index c74d8f272..ab57bfb20 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -388,6 +388,81 @@ function isLexicallyReachable( ); } +/** + * Whether to withhold test-file definitions from production references. + * On by default; `CODEGRAPH_TEST_TREE_GATE=0` restores the old behaviour, the + * same escape hatch `CODEGRAPH_AMBIGUOUS_NAME_CEILING` gives its own gate. + */ +const TEST_TREE_GATE = process.env.CODEGRAPH_TEST_TREE_GATE !== '0'; + +/** + * Files a test RUNNER will collect, identified by the naming its own toolchain + * enforces — not by where they sit. + * + * The distinction matters because this gate DELETES edges, and a wrong deletion + * is silent. Which directories hold a project's tests is a project's own + * decision (`spec/` is an OpenAPI document in one repo and RSpec in another; + * `testing/` is often a shipped utility library), so guessing at it would + * remove real edges from projects that merely name a folder unluckily. A + * FILENAME, by contrast, is not a preference: pytest collects `test_*.py` and + * `*_test.py` and nothing else, `go test` requires `_test.go`, vitest and jest + * default to `*.test.*` / `*.spec.*`, surefire to `*Test.java`. A file named + * this way IS a test, in every project, by the runner's own rule. + * + * Measured on a mixed Elixir/TypeScript/Python repository: of the impossible + * production→test call edges, the runner-enforced names accounted for all but + * one. Directory guessing would have added the risk without the reward. + */ +export function isRunnerNamedTestFile(filePath: string): boolean { + const fileName = filePath.slice(filePath.lastIndexOf('/') + 1); + const lower = fileName.toLowerCase(); + if ( + // pytest: test_foo.py + lower.startsWith('test_') || + // vitest/jest/go/pytest/rspec: foo.test.ts, foo_test.go, foo-spec.rb, bar_spec.py + /[._-](test|tests|spec|specs)\.[a-z0-9]+$/.test(lower) + ) { + return true; + } + + // The bare CamelCase suffix — `OrderTest.java` — is a JVM/.NET/Swift + // convention (Maven surefire collects `**/*Test.java`, `**/*Tests.java`), + // NOT a JavaScript one: vitest and jest collect `*.test.ts` and would never + // pick up `useTests.ts`, which is an ordinary hook. Applying the suffix + // everywhere misreads any identifier that happens to end in "Test" or + // "Tests", so it is scoped to the languages whose runners actually use it. + return /\.(java|kt|kts|scala|cs|swift)$/i.test(fileName) + && /(?:Test|Tests|TestCase|Spec|Specs)\.[A-Za-z0-9]+$/.test(fileName); +} + +/** + * Drop test-file definitions when the reference comes from production code. + * + * Tests depend on the code they exercise; the code never depends on its tests. + * So a production symbol resolving INTO a test file is always wrong — and it + * happens constantly, because the names that reach this fallback are the ones + * with no real definition to bind to. A React component calling `t(...)` from a + * `const { t } = useTranslation()` destructure has no local `t` node and no + * exported one either (the hook comes from a package), so the only candidates + * anywhere are same-named helpers inside test files, and proximity picks one. + * Measured on one repository: 2,820 calls across 137 production page components + * all resolved to a single i18n stub defined in one test file. + * + * A reference from a test is left alone — tests calling test helpers is the + * normal direction, and shared fixtures legitimately live in the test tree. + */ +function applyTestTreeGate(candidates: Node[], ref: UnresolvedRef): Node[] { + if (!TEST_TREE_GATE) return candidates; + if (isRunnerNamedTestFile(ref.filePath)) return candidates; + // Unconditional, including when it empties the set. A production symbol that + // matches ONLY inside tests is the exact shape of the defect: production code + // cannot depend on a test file — it would not build or run — so a lone + // test-file candidate is a coincidence of naming, never the real target. + // Keeping it as a fallback would leave the whole defect in place, since the + // fabricated edges are precisely the ones with no production alternative. + return candidates.filter((c) => !isRunnerNamedTestFile(c.filePath)); +} + /** * Try to resolve a reference by exact name match */ @@ -404,7 +479,8 @@ export function matchByExactName( // unresolved import refs each scored K same-named import candidates through // findBestMatch — O(K²) per package, the dominant cost of "Resolving refs" on // large import-heavy (front-end + back-end) repos (#915). - const candidates = applyLanguageGate(context.getNodesByName(ref.referenceName), ref) + const candidates = applyTestTreeGate( + applyLanguageGate(context.getNodesByName(ref.referenceName), ref), ref) .filter((n) => n.kind !== 'import') // Nested locals are only reachable from inside their container (#1230). .filter((n) => isLexicallyReachable(n, ref, context)); @@ -706,6 +782,13 @@ export function resolveMethodOnType( } } } + // Same rule as the name-based strategies: production code cannot depend on a + // test file, so a method that only matches inside one is a naming coincidence. + // This path reaches it through an inferred receiver type rather than a bare + // name, but the failure is identical — an unpinned receiver settles for a + // same-named method, and a test's class method is as good a match as any. + matches = applyTestTreeGate(matches, ref); + if (matches.length === 0) { // Conformance fallback: the method may be defined on a supertype `typeName` // extends, or on a protocol / trait it conforms to (e.g. a Swift protocol- @@ -1998,8 +2081,14 @@ export function matchMethodCall( if (methodCandidates.length > AMBIGUOUS_NAME_CEILING) { return null; } - const methods = methodCandidates.filter( - (n) => n.kind === 'method' && n.name === methodName + // Same rule as every other strategy: production code cannot depend on a + // test file. This one needs it most — it scores a receiver NAME against a + // class name, so a short receiver (`rep.say()`) matches any test's stand-in + // class (`_Rep`) on one shared word, and the stand-ins that tests define + // are exactly the classes that share a name with the real collaborator. + const methods = applyTestTreeGate( + methodCandidates.filter((n) => n.kind === 'method' && n.name === methodName), + ref ); // Filter to same-language candidates first @@ -2412,7 +2501,8 @@ export function matchFuzzy( // Filter to callable kinds only (function, method, class) const callableKinds = new Set(['function', 'method', 'class']); - const callableCandidates = applyLanguageGate(candidates.filter((n) => callableKinds.has(n.kind)), ref); + const callableCandidates = applyTestTreeGate( + applyLanguageGate(candidates.filter((n) => callableKinds.has(n.kind)), ref), ref); // Prefer same-language matches const sameLanguageCandidates = callableCandidates.filter(n => n.language === ref.language);