From 05038e87d15805797ed6793d3d505a2d23f9d25f Mon Sep 17 00:00:00 2001 From: Shounak Joshi Date: Mon, 9 Nov 2026 13:37:38 +0530 Subject: [PATCH 1/3] fix: package-owned testdata selects the owning Go package's tests instead of always falling back By Go convention `pkg/testdata/**` is read by the tests in `pkg`: the go tool runs a package's tests with the package's directory as the working directory (#83). DiffCI treated every `testdata/` edit as a full-validation fallback, so `internal/parser/testdata/case1.json` reran the whole suite instead of `go test ./internal/parser`. Changes under `/testdata/` of a package that reported `_test.go` files now select those tests (new `GO_TESTDATA` impact reason). The route is refused, and the earlier full-validation fallback stands, when: - the parent directory is not a discovered Go package, or it has no `_test.go` files (`byOwner` comes from `profile.goTestPackages`, so a legacy cached profile without that evidence fails closed), - a `_`-prefixed or `.`-prefixed directory lies on the way to `testdata` or below it (`_`/`.`-prefixed directories keep falling back, as do `_data` and `.data` siblings), - another package's `_test.go` file names the changed path or its `testdata` directory by reference - checked with the existing reference machinery against both the file path and the directory, so a sibling writing `"../parser/testdata"` (the issue's example) or `"../parser/testdata/case1.json"` blocks the selective route and keeps full validation, - the file is a modeled or reported `go:embed` input: embedding makes it a production runtime input, not only test input (`goEmbedPaths` is checked before the owner route), - a listed `_test.go` file's contents cannot be read: an unresolvable sibling reader cannot be cleared, so ownership is refused. Added to `declaredTests`: a GO profile has no `profile.tests` entries, so a declared `inputs` entry naming any Go path was always stale ("selects no tests") and forced the very fallback the declaration was meant to override. The go adapter's reported `_test.go` files now join the declared candidates, behind globs the repository names, so `inputs` keeps working as the overriding workaround the issue relies on, and its declared tests union with the package-owned selection. Validation: - npm run typecheck, npm run build, npm run audit:boundary: clean - npm test: 791 passed / 8 failed / 0 skipped; every failure is tests/action-verify.test.ts, which fails the same 8/8 on the unmodified tree under Node 24 on Windows - unrelated to this change - fixture additions in tests/repo/go-runtime-inputs.test.ts (51 cases): owned testdata selects `./pkg`; sibling reference to path or directory falls back; cross-package references resolve, `../p/data/...` never matches `p/testdata/...`; ownerless, `_`/`.`-routed and nested-`_`/`.`-prefixed testdata fall back; legacy cached profile and cached round-trip fail closed; embedded testdata stays full with repository contents wired like src/analyze.ts does; declared inputs union with the package tests Docs: release-boundary.md states the selective case and the refused cases; using-diffci.md's Go fallback table records what still lands in the discovery-excluded fallback and documents the package-testdata selection. Unsupported cases (unchanged, still full validation): testdata claimed by an unreadable sibling test; `testdata` below a package without tests; embedded testdata; dynamic path construction (`filepath.Join(dir, ...)`); testdata read via a renamed directory alias. Related to #76, closes #83. Signed-off-by: Shounak Joshi --- docs/release-boundary.md | 2 +- docs/using-diffci.md | 4 +- src/repo/impact-types.ts | 1 + src/repo/impact.ts | 102 ++++++++++++++++++++++- tests/repo/go-runtime-inputs.test.ts | 118 +++++++++++++++++++++++++-- 5 files changed, 215 insertions(+), 12 deletions(-) diff --git a/docs/release-boundary.md b/docs/release-boundary.md index 7363e49..3bc2087 100644 --- a/docs/release-boundary.md +++ b/docs/release-boundary.md @@ -26,7 +26,7 @@ When a modeled lockfile change accounts for a root `package.json` edit that only Berry, pnpm lockfile formats before 9.0, workspaces, alternate install layouts, mixed package managers, unsupported requests or fields, malformed lockfiles and incomplete attribution retain full fallback. The engine does not install packages or fetch registry metadata to fill gaps. Existing static import and file-reference limits still apply; required CI remains authoritative. -Go selection models package sources and `go:embed` inputs reported by `go list`. Every changed path outside that metadata requires full validation, regardless of extension or directory, including deleted paths and rename origins. Older cached profiles without this ownership evidence also require full validation. Modeled embeds retain dependency-based selection; discovery-excluded paths such as `testdata` retain full validation. Arbitrary runtime reads are not inferred from source text. Additional runtime reads of already modeled files by unrelated packages, and external or dynamically supplied inputs, remain outside this model. +Go selection models package sources and `go:embed` inputs reported by `go list`. Every changed path outside that metadata requires full validation, regardless of extension or directory, including deleted paths and rename origins. Older cached profiles without this ownership evidence also require full validation. Modeled embeds retain dependency-based selection; other discovery-excluded paths retain full validation, except a plain `/testdata/` path of a package reported with `_test.go` files, which selects that package's tests - unless another package's `_test.go` file names the path or its `testdata` directory by reference, a `_`-prefixed or `.`-prefixed directory lies on the way or below `testdata`, or the cached profile predates the package-test evidence. Embedded testdata is a production runtime input and retains full validation. A declared `inputs` entry still adds its tests on top. Arbitrary runtime reads are not inferred from source text. Additional runtime reads of already modeled files by unrelated packages, and external or dynamically supplied inputs, remain outside this model. Python selection reads file-level imports with Python's own `ast` module, resolved against flat and `src/` layouts, pytest's `pythonpath` and `pyproject` packages, and discovers tests with pytest's `python_files` setting. A changed `conftest.py` selects every test under its directory. Computed imports, unresolved first-party or relative imports, unparseable files, pytest configuration DiffCI cannot read, and changed project files such as `pyproject.toml` or `requirements.txt` retain full validation. Coverage-based and plugin-defined test relationships are not modeled. diff --git a/docs/using-diffci.md b/docs/using-diffci.md index 38d5e9b..815afe5 100644 --- a/docs/using-diffci.md +++ b/docs/using-diffci.md @@ -495,7 +495,9 @@ _The runtime-input check (`Go runtime inputs outside source/embed metadata …`) | `Go …: runtime plugins require full validation` | `import "plugin"` | No | | `Go …: generated or linked dependencies require full validation` | A `//go:generate` or `//go:linkname` directive | No | | `Go runtime inputs outside source/embed metadata require full validation: …` | Edit `config.yaml` beside a Go package that a test opens | No | -| `Changes in Go discovery-excluded paths require full validation, including runtime test data` | Edit `testdata/input.json` or a file under a `_`-prefixed directory | No | +| `Changes in Go discovery-excluded paths require full validation, including runtime test data` | Edit a `testdata` file of a package without `_test.go` files, a file under a `_`-prefixed directory, a `_`/`.`-prefixed directory inside the `testdata` tree, `testdata` that `go:embed` embeds, or `testdata` a sibling package's test file names | No - plain owned `testdata` selects the owning package's tests instead (see below) | + +_Package testdata (#83): a plain `testdata/` input whose parent directory is a reported Go package with `_test.go` files selects that package's tests (reason `GO_TESTDATA`) instead of falling back - the go tool runs a package's tests with the package directory as the working directory. It falls back when another package's `_test.go` file names the path or its `testdata` directory, when a `_`-prefixed or `.`-prefixed directory lies on the way or below `testdata`, when the cached profile predates the package-test evidence, or when the file is embedded. A declared `inputs` entry adds its declared tests on top of the package's._ ### Maven diff --git a/src/repo/impact-types.ts b/src/repo/impact-types.ts index d89c2b7..c8377d3 100644 --- a/src/repo/impact-types.ts +++ b/src/repo/impact-types.ts @@ -25,6 +25,7 @@ export type ImpactReason = | "TEST_FIXTURE_OWNER" | "ALWAYS_RUN_POLICY" | "DECLARED_INPUT" + | "GO_TESTDATA" | "ROUTE_VISIT" | "DELETED_FILE_LEGACY_DEPENDENTS" | "DELETED_FILE_UNKNOWABLE_GRAPH" diff --git a/src/repo/impact.ts b/src/repo/impact.ts index 564b46f..d0b0fe5 100644 --- a/src/repo/impact.ts +++ b/src/repo/impact.ts @@ -8,7 +8,7 @@ import { refineConfidenceForDelta, relevantScopedAdapterBlockers } from "./graph import { repositoryLayout, UNKNOWN_REPOSITORY_LAYOUT, type RepositoryLayout } from "./layout.js"; import { DEFAULT_TEST_FILE_MATCHER, matchesGlob as matchesTestGlob, testDiscoveryErrors, testFileMatcherForProfile } from "./test-discovery.js"; import { resolveTestFixtureOwners } from "./test-fixture-ownership.js"; -import { compilerConfigNames, directoryWalkers, DIRECTING_EXTENSIONS, infrastructureRole, isAgentConfigurationPath, listsDirectories, loadsWithoutNaming, isImplicitlyLoadedFile, isWorkerConfig, resolveFileReferences, suiteMayRunWorkers, testScriptClosure } from "./file-references.js"; +import { compilerConfigNames, contentNamesPath, directoryWalkers, DIRECTING_EXTENSIONS, infrastructureRole, isAgentConfigurationPath, listsDirectories, loadsWithoutNaming, isImplicitlyLoadedFile, isWorkerConfig, referencePattern, referenceTokens, resolveFileReferences, suiteMayRunWorkers, testScriptClosure } from "./file-references.js"; import { workflowFeedsTests } from "./workflow-test-steps.js"; import { modelTestScriptUniverse, type ScriptUniverse } from "./script-universe.js"; import { compiledProjects, type CompiledProject } from "./compiled-projects.js"; @@ -278,6 +278,13 @@ export class ImpactAnalyzer { private walkerTests: string[] | undefined; private knownTests: string[] | undefined; private declaredCovered = new Set(); + /** Go test files grouped by their package's directory value (`profile.goTestPackages` keys are test + * files; go test files always end `_test.go`). Built once per analysis. */ + private goPackageTestFilesByOwner = new Map(); + /** A changed `testdata/` path selects its owning package's tests when the parent directory is a Go + * package with tests and no other package's test file names it by reference; every other + * discovery-excluded path keeps full validation. */ + private goPackageTestDataOwner = new Map(); /** Test files known to the graph: nodes matching the profile's test globs or the test-file matcher. */ private knownTestPaths(profile: RepositoryProfile, graph: DependencyGraph): string[] { @@ -301,7 +308,80 @@ export class ImpactAnalyzer { const inputs = (profile.diffciConfig?.inputs ?? []).filter((input) => input.files.some((glob) => matchesTestGlob(path, glob))); if (inputs.length === 0) return undefined; const globs = inputs.flatMap((input) => input.tests); - return this.knownTestPaths(profile, graph).filter((test) => globs.some((glob) => matchesTestGlob(test, glob))); + // Go profiles declare no test locations (`profile.tests` is a JS-runner notion), so the go + // adapter's package-reported `_test.go` files join the candidates; without them a declaration + // naming any Go path is always stale and files the very fallback the declaration was meant to + // override. They are additional candidates behind globs the repository names - nothing is + // selected that the declaration did not ask for (2026-11-09, issue #83). + const goTestCandidates = profile.adapters?.some(adapter => adapter.id === "go") + ? Object.keys(profile.goTestPackages ?? {}).filter(test => test.endsWith("_test.go")) + : []; + return [...this.knownTestPaths(profile, graph), ...goTestCandidates].filter((test) => globs.some((glob) => matchesTestGlob(test, glob))); + } + + /** + * Groups the Go adapter's test files by their package's directory and maps each changed + * `testdata/` path to its owning package (`testdata` is excluded from package discovery, so go + * list cannot report this ownership itself; the go tool runs a package's tests with its + * directory as the working directory, which is the convention the path follows). Paths that + * qualify select that package's tests for the changed path instead of full validation; a legacy + * cached profile without test-package evidence, repository contents being unavailable, a parent + * directory that is no Go package with tests, `_`-prefixed or `.`-prefixed directories, or + * another package's test file naming the path or its `testdata` directory by literal reference + * (also, a `_`/`.`-prefixed directory inside the testdata tree) leave the fallback as before. + */ + private mapGoPackageTestData(delta: GitDelta, profile: RepositoryProfile): Map { + this.goPackageTestDataOwner = new Map(); + const byOwner = new Map(); + for (const [test, owner] of Object.entries(profile.goTestPackages ?? {})) { + if (!test.endsWith("_test.go")) continue; + const tests = byOwner.get(owner); + if (tests) tests.push(test); + else byOwner.set(owner, [test]); + } + this.goPackageTestFilesByOwner = byOwner; + if (!profile.goTestPackages || !this.readRepositoryFile) return this.goPackageTestDataOwner; + const readRepositoryFile = this.readRepositoryFile; + for (const path of new Set(delta.files.flatMap(allChangePaths))) { + const owner = this.goPackageTestDataOwnerFor(path, byOwner, readRepositoryFile); + if (owner !== undefined) this.goPackageTestDataOwner.set(path, owner); + } + return this.goPackageTestDataOwner; + } + + private goPackageTestDataOwnerFor( + path: string, + byOwner: ReadonlyMap, + readRepositoryFile: (path: string) => string | undefined, + ): string | undefined { + const segments = path.split("/"); + const at = segments.indexOf("testdata"); + if (at === -1) return undefined; + // An embedded testdata file is a production runtime input, not only test input: it keeps the + // embed fallback, and never takes the package-owned test data route. + if (this.goEmbedPaths.has(path)) return undefined; + // Only `testdata` gets the package convention: discovery-excluded directory names around it + // (both on the way to the package and inside the testdata tree) keep the fallback. + const ignored = (segment: string) => segment === "" || segment.startsWith("_") || segment.startsWith("."); + if (segments.slice(0, at).some(ignored) || segments.slice(at + 1).some(ignored)) return undefined; + const owner = segments.slice(0, at).length === 0 ? "." : `./${segments.slice(0, at).join("/")}`; + const tests = byOwner.get(owner); + if (!tests?.length) return undefined; + const dataDir = [...segments.slice(0, at), "testdata"].join("/"); + const filePattern = referencePattern(referenceTokens(path)); + const dirPattern = referencePattern(referenceTokens(dataDir)); + for (const [pkg, readers] of byOwner) { + if (pkg === owner) continue; + for (const reader of readers) { + const content = readRepositoryFile(reader); + // A listed test file that cannot be read could still name the testdata; without its + // contents the conservative answer is no selective route at all. + if (content === undefined) return undefined; + if (contentNamesPath(content, reader, path, filePattern, true) + || contentNamesPath(content, reader, dataDir, dirPattern, true)) return undefined; + } + } + return owner; } /** Dependents whose tests a change to `path` can affect. When the graph allows runtime selection @@ -366,16 +446,18 @@ export class ImpactAnalyzer { } const hasGoAdapter = profile.adapters?.some(adapter => adapter.id === "go"); this.goEmbedPaths = new Set(hasGoAdapter ? profile.goDependencyPaths?.embeds : []); + const goPackageTestDataOwner = hasGoAdapter ? this.mapGoPackageTestData(delta, profile) : new Map(); if (hasGoAdapter) { // go list models package sources and embeds, not arbitrary runtime reads. Neither a // familiar extension/directory nor membership in another adapter's graph proves ownership. // Check both identities of renames/copies, including files absent from the HEAD inventory. // Missing metadata in older cached profiles deliberately fails closed as well. const modeledPaths = new Set([...(profile.goDependencyPaths?.sources ?? []), ...this.goEmbedPaths]); - const unmodeledPaths = [...new Set(delta.files.flatMap(allChangePaths))].filter(path => !modeledPaths.has(path)); + const unmodeledPaths = [...new Set(delta.files.flatMap(allChangePaths))] + .filter(path => !modeledPaths.has(path) && !goPackageTestDataOwner.has(path)); if (unmodeledPaths.length) fallbackReasons.push(`Go runtime inputs outside source/embed metadata require full validation: ${unmodeledPaths.slice(0, 20).join(", ")}`); } - if (hasGoAdapter && delta.files.some(file => allChangePaths(file).some(isGoDiscoveryIgnoredPath))) { + if (hasGoAdapter && delta.files.some(file => allChangePaths(file).some(path => isGoDiscoveryIgnoredPath(path) && !goPackageTestDataOwner.has(path)))) { fallbackReasons.push("Changes in Go discovery-excluded paths require full validation, including runtime test data"); } const excludedGoRoots = profile.goExcludedModuleRoots ?? []; @@ -779,6 +861,18 @@ export class ImpactAnalyzer { this.processAssetChange(changedPath, graph, profile, affectedAssets, affectedEntryPoints, affectedTests, affectedSources, evidence); return; } + // A `testdata/` path whose parent directory is a Go package's is conventionally read by that + // package's tests. Ownership and cross-references were resolved above, so every path reaching + // here selects those tests; the loosened cases (no owner package, cross-package readers, + // discovery-excluded directory names) fell back and never reach this route. + const goPackageOwner = this.goPackageTestDataOwner.get(changedPath); + if (goPackageOwner !== undefined) { + evidence.push(makeEvidence("GO_TESTDATA", changedPath, `${changedPath} is testdata of Go package ${goPackageOwner}`)); + for (const test of this.goPackageTestFilesByOwner.get(goPackageOwner) ?? []) { + this.addAffectedTest(changedPath, test, graph, affectedTests, "GO_TESTDATA", evidence); + } + return; + } // Non-code files are resolved by reference below, directories included; code is not, so a test that // lists the directory a code file is in is added here (file-references.ts, directoryWalkers). if (isSourceFilePath(changedPath) && this.readRepositoryFile) { diff --git a/tests/repo/go-runtime-inputs.test.ts b/tests/repo/go-runtime-inputs.test.ts index a257cfd..3a89573 100644 --- a/tests/repo/go-runtime-inputs.test.ts +++ b/tests/repo/go-runtime-inputs.test.ts @@ -8,6 +8,7 @@ import type { ChangedFile, GitDelta } from "../../src/git/types.js"; import { DefaultCIPlanner } from "../../src/planner/planner.js"; import { createTaskRegistry } from "../../src/planner/task-registry.js"; import { analyzeGoMetadata, goAdapter } from "../../src/repo/adapters/go.js"; +import { repositoryFileReader } from "../../src/repo/file-references.js"; import { buildDependencyGraph, hydrateDependencyGraph } from "../../src/repo/graph.js"; import { ImpactAnalyzer } from "../../src/repo/impact.js"; import type { DependencyGraphResult } from "../../src/repo/types.js"; @@ -23,9 +24,14 @@ function delta(file: ChangedFile): GitDelta { }; } -function planChange(graph: DependencyGraphResult, file: ChangedFile) { +function planChange(graph: DependencyGraphResult, file: ChangedFile, analysis?: { repoPath: string; fileContents?: Record }) { const change = delta(file); - const impact = new ImpactAnalyzer().analyze(change, graph, graph.profile); + // Content checks need the same wiring the engine uses (src/analyze.ts): the repository reader and the + // file inventory. Tests that only assert fallback behavior can omit them and fail closed instead. + const analysisOptions = analysis ? { repositoryFiles: new Set(Object.keys(analysis.fileContents ?? {})), readRepositoryFile: repositoryFileReader(analysis.repoPath) } : undefined; + const impact = analysisOptions + ? new ImpactAnalyzer().analyze(change, graph, graph.profile, analysisOptions) + : new ImpactAnalyzer().analyze(change, graph, graph.profile); const plan = new DefaultCIPlanner(createTaskRegistry([])).plan({ delta: change, impact, profile: graph.profile }); return { impact, plan }; } @@ -36,6 +42,7 @@ async function fixture( extraFiles: Record, run: (graph: DependencyGraphResult, root: string) => void | Promise, embeds: { production?: string[]; test?: string[]; external?: string[] } = {}, + extraServiceTestName?: string, ) { const root = mkdtempSync(join(tmpdir(), "diffci-go-runtime-inputs-")); const declarations = (paths: string[] = []) => paths.map((path, index) => `//go:embed ${path}\nvar embedded${index} string\n`).join(""); @@ -61,7 +68,7 @@ async function fixture( } const metadata = [ { Dir: join(root, "p"), ImportPath: "example.com/project/p", Name: "p", GoFiles: ["value.go"], TestGoFiles: ["value_test.go"], Imports: ["os", ...(embeds.production?.length ? ["embed"] : [])], TestImports: ["testing", ...(embeds.test?.length ? ["embed"] : [])], EmbedFiles: embeds.production, TestEmbedFiles: embeds.test, XTestGoFiles: embeds.external?.length ? ["value_external_test.go"] : [], XTestImports: embeds.external?.length ? ["testing", "embed"] : [], XTestEmbedFiles: embeds.external }, - { Dir: join(root, "service"), ImportPath: "example.com/project/service", Name: "service", GoFiles: ["service.go"], TestGoFiles: ["service_test.go"], Imports: ["example.com/project/p"], TestImports: ["testing"] }, + { Dir: join(root, "service"), ImportPath: "example.com/project/service", Name: "service", GoFiles: ["service.go"], TestGoFiles: extraServiceTestName ? ["service_test.go", extraServiceTestName] : ["service_test.go"], Imports: ["example.com/project/p"], TestImports: ["testing"] }, { Dir: join(root, "unrelated"), ImportPath: "example.com/project/unrelated", Name: "unrelated", GoFiles: ["value.go"], TestGoFiles: ["value_test.go"], TestImports: ["testing"] }, ].map(pkg => JSON.stringify(pkg)).join("\n"); goAdapter.analyze = context => analyzeGoMetadata(context, metadata); @@ -75,12 +82,12 @@ async function fixture( } } -function assertFullRuntimeFallback(graph: DependencyGraphResult, file: ChangedFile, unmodeledPath = file.path) { - const { impact, plan } = planChange(graph, file); +function assertFullRuntimeFallback(graph: DependencyGraphResult, file: ChangedFile, unmodeledPath = file.path, allTestPaths = allTests, analysis?: { repoPath: string; fileContents?: Record }) { + const { impact, plan } = planChange(graph, file, analysis); assert.equal(impact.fallbackRequired, true); assert.ok(impact.fallbackReasons.some(reason => reason.includes("Go runtime inputs outside source/embed metadata") && reason.includes(unmodeledPath)), impact.fallbackReasons.join("\n")); assert.equal(plan.mode, "FULL"); - assert.deepEqual(plan.selectedTests, allTests); + assert.deepEqual([...plan.selectedTests].sort(), [...allTestPaths].sort()); assert.deepEqual(plan.skippedTests, []); assert.deepEqual(plan.commandSpecs, []); assert.equal(plan.commandSynthesis?.status, "NOT_APPLICABLE"); @@ -185,6 +192,23 @@ test("Go testdata, hidden and underscore paths retain full validation even when }, { production: paths }); }); +test("embedded package testdata keeps full validation also with repository contents available", async () => { + const files = { "p/testdata/settings.json": "embedded" }; + await fixture(files, (graph, root) => { + // The engine wires repository contents (src/analyze.ts); this is the production wiring, not + // the fail-closed absence of it, and the embed still keeps its discovery-excluded fallback. + const analysis = { repoPath: root, fileContents: files }; + const { impact, plan } = planChange(graph, { path: "p/testdata/settings.json", changeType: "modified" }, analysis); + assert.equal(impact.fallbackRequired, true); + assert.ok(impact.fallbackReasons.some(reason => reason.includes("Go discovery-excluded paths")), impact.fallbackReasons.join("\n")); + // the embed is modeled, so the "runtime inputs outside source/embed metadata" message never names it + assert.ok(impact.fallbackReasons.every(reason => !reason.includes("runtime inputs outside source/embed metadata")), impact.fallbackReasons.join("\n")); + assert.equal(plan.mode, "FULL"); + assert.deepEqual([...plan.selectedTests].sort(), [...allTests].sort()); + assert.ok(!impact.affectedTests.some(t => t.reasons.includes("GO_TESTDATA"))); + }, { production: ["testdata/settings.json"] }); +}); + test("Go modeled-path evidence survives graph caching", async () => { await fixture({ "p/data/embedded.json": "embedded" }, (graph, root) => { const cache = new GraphCache({ cacheDir: join(root, ".cache") }); @@ -214,3 +238,85 @@ test("Go legacy cached profiles without modeled-path evidence fail closed", asyn assertFullRuntimeFallback(loaded, { path: "p/value.go", changeType: "modified" }); }, { production: ["data/embedded.json"] }); }); + +// -- Go package testdata (issue 83): `testdata/` of a package with tests selects those tests ------------- +test("a changed package testdata input selects the owning package's tests", async () => { + const files = { "p/testdata/settings.json": "expected" }; + await fixture(files, (graph, root) => { + const { impact, plan } = planChange(graph, { path: "p/testdata/settings.json", changeType: "modified" }, { repoPath: root, fileContents: files }); + assert.equal(impact.fallbackRequired, false); + assert.equal(plan.mode, "SELECTIVE"); + assert.deepEqual(plan.selectedTests, ["p/value_test.go"]); + assert.deepEqual(plan.skippedTests, ["service/service_test.go", "unrelated/value_test.go"]); + assert.ok(impact.affectedTests.some(t => t.reasons.includes("GO_TESTDATA")), impact.affectedTests.map(t => t.reasons.join("|")).join("\n")); + assert.ok(impact.evidence.some(e => e.reason === "GO_TESTDATA" && e.message.includes("p/testdata/settings.json"))); + }); +}); + +test("a sibling package's test file naming the testdata keeps full validation", async () => { + const files = { + "p/testdata/settings.json": "expected", + // package service's sibling test names ../p/testdata, so p's testdata is not package-private + "service/service_relative_test.go": 'package service\nimport "testing"\nfunc TestRelative(t *testing.T) { _ = "../p/testdata/settings.json" }', + }; + await fixture(files, (graph, root) => { + const relative = [...allTests, "service/service_relative_test.go"]; + assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", relative, { repoPath: root, fileContents: files }); + const { impact, plan } = planChange(graph, { path: "p/testdata/settings.json", changeType: "modified" }, { repoPath: root, fileContents: files }); + assert.ok(impact.fallbackReasons.some(reason => reason.includes("Go discovery-excluded paths")), impact.fallbackReasons.join("\n")); + assert.equal(plan.mode, "FULL"); + }, {}, "service_relative_test.go"); +}); + +test("sibling test files naming only the testdata directory also fall back", async () => { + const files = { + "p/testdata/settings.json": "expected", + // written the way the issue describes it: `"../p/testdata"` without the file name + "service/service_relative_test.go": 'package service\nimport "testing"\nfunc TestRelative(t *testing.T) { _ = "../p/testdata" }', + }; + await fixture(files, (graph, root) => { + const relative = [...allTests, "service/service_relative_test.go"]; + assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", relative, { repoPath: root, fileContents: files }); + }, {}, "service_relative_test.go"); +}); + +test("testdata below an owning package without tests, and *_/.-named directories, fall back", async t => { + const paths = ["orphan/testdata/case.json", "_scratch/p/testdata/case.json", "p/testdata/_hidden/case.json"]; + await fixture(Object.fromEntries(paths.map(path => [path, "data"])), async graph => { + for (const path of paths) await t.test(path, () => { + const { impact, plan } = planChange(graph, { path, changeType: "modified" }); + assert.equal(impact.fallbackRequired, true); + assert.ok(impact.fallbackReasons.some(reason => reason.includes("Go runtime inputs outside source/embed metadata") && reason.includes(path)), impact.fallbackReasons.join("\n")); + assert.ok(impact.fallbackReasons.some(reason => reason.includes("Go discovery-excluded paths"))); + assert.equal(plan.mode, "FULL"); + assert.deepEqual(plan.selectedTests, allTests); + }); + }); +}); + +test("a legacy cached profile without test-package evidence fails closed for testdata", async () => { + await fixture({ "p/testdata/settings.json": "expected" }, (graph, root) => { + delete graph.profile.goTestPackages; + const cache = new GraphCache({ cacheDir: join(root, ".cache") }); + const key = buildGraphCacheKey({ commitSha: "head" }); + cache.save(key, graph); + const loaded = cache.load(key)!; + assert.ok(loaded); + loaded.graph = hydrateDependencyGraph(loaded.graph, root); + assertFullRuntimeFallback(loaded, { path: "p/testdata/settings.json", changeType: "modified" }); + }); +}); + +test("declared inputs add their tests to the package-owned testdata selection", async () => { + const files = { "p/testdata/settings.json": "expected" }; + await fixture(files, (graph, root) => { + graph.profile.diffciConfig = { inputs: [{ files: ["p/testdata/**"], tests: ["service/**"] }] }; + const { impact, plan } = planChange(graph, { path: "p/testdata/settings.json", changeType: "modified" }, { repoPath: root, fileContents: files }); + assert.equal(impact.fallbackRequired, false); + assert.equal(plan.mode, "SELECTIVE"); + // the declaration's tests and the owning package's tests union into one selection + assert.deepEqual([...plan.selectedTests].sort(), ["p/value_test.go", "service/service_test.go"]); + assert.ok(impact.affectedTests.some(t => t.path === "service/service_test.go" && t.reasons.includes("DECLARED_INPUT"))); + assert.ok(impact.affectedTests.some(t => t.path === "p/value_test.go" && t.reasons.includes("GO_TESTDATA"))); + }); +}); From becdd40d23116846d4544e407abaabbf5250dc5a Mon Sep 17 00:00:00 2001 From: Shounak Joshi Date: Mon, 9 Nov 2026 17:10:31 +0530 Subject: [PATCH 2/3] fix: check every go reader of package-owned testdata The package-owned testdata route judged only other packages' `_test.go` files, so a helper in an ordinary `.go` file (the `testutil/golden.go` pattern) reading a package's testdata on behalf of its callers' tests and a test outside every go-reported package (nested module, other language) naming it directly fell through the gap. Since go scopes it, the current adapter puts a package's test files into sourcePaths too, so a package's members are exactly its sourcePaths and the test-file grouping stands in for legacy cached profiles. Readers outside package discovery never take the convention (their tests run with a different working directory), and a reference there disqualifies the route like a cross-package one. Go import clauses are excluded from the scan: importing `example.com/project/p` names the package, not the directories inside it, and counting it would disqualify every sibling's route. --- src/repo/impact.ts | 75 ++++++++++++++++++++++++---- tests/repo/go-runtime-inputs.test.ts | 43 ++++++++++++++-- 2 files changed, 105 insertions(+), 13 deletions(-) diff --git a/src/repo/impact.ts b/src/repo/impact.ts index d0b0fe5..c012be9 100644 --- a/src/repo/impact.ts +++ b/src/repo/impact.ts @@ -22,6 +22,18 @@ const NEXT_ENTRY_NAMES = new Set(["page","layout","route","api","loading","error function isLockfile(filePath: string): boolean { return /(?:^|\/)(?:package-lock\.json|npm-shrinkwrap\.json|yarn\.lock|pnpm-lock\.yaml|bun\.lockb?)$/.test(filePath); } function isSourceFilePath(filePath: string): boolean { return SOURCE_EXTENSIONS.has(extname(filePath).toLowerCase()); } function isAssetFilePath(filePath: string): boolean { return ASSET_EXTENSIONS.has(extname(filePath).toLowerCase()); } +function isGoReaderFile(filePath: string): boolean { return filePath.endsWith(".go"); } +/** + * Reads a go reader's references without its import clauses: an import names the package, not any + * directory inside it, and counting `example.com/project/p` as a reference to a changed `p/...` + * path would disqualify every sibling package's package-owned testdata route (2026-11-09, review of + * #85). String literals and call arguments — where a testdata read actually appears — are kept. + */ +function goReaderReferences(content: string): string { + return content + .replace(/^[ \t]*import[ \t]+[^(){}[\]\n]*\n/gm, "") + .replace(/^[ \t]*import[ \t]*\([^)]*\)[ \t]*(?:\/\/[^\n]*)?\n/gm, ""); +} /** Vitest's default `typecheck.include`: tests whose outcome is the type checker's. */ const TYPE_TEST_FILE = /\.(?:test|spec)-d\.[cm]?[jt]sx?$/; // "Is this a test?" is answered by the profile's test patterns (src/repo/test-discovery.ts) - one shared @@ -281,6 +293,15 @@ export class ImpactAnalyzer { /** Go test files grouped by their package's directory value (`profile.goTestPackages` keys are test * files; go test files always end `_test.go`). Built once per analysis. */ private goPackageTestFilesByOwner = new Map(); + /** Each go-reported package's full file list keyed like `testPackages` (owner itself included). Profiles + * from the current adapter put a package's test files into `sourcePaths` too, so when this map is + * populated a package's members are exactly its sourcePaths; legacy cached profiles leave it empty. */ + private goSourceFilesByOwner = new Map(); + /** Repository files that no go-reported package reported — tests in nested modules or other + * languages, ordinary `.go` files excluded from package discovery. Their references to a package's + * testdata disqualify the selective route like a cross-package one (a nested module cannot take the + * package-owned convention: its tests run with a different working directory). */ + private goOutsideReaders = new Set(); /** A changed `testdata/` path selects its owning package's tests when the parent directory is a Go * package with tests and no other package's test file names it by reference; every other * discovery-excluded path keeps full validation. */ @@ -340,6 +361,29 @@ export class ImpactAnalyzer { else byOwner.set(owner, [test]); } this.goPackageTestFilesByOwner = byOwner; + // A helper in another package's ordinary `.go` file (the `testutil/golden.go` pattern) can read a + // package's testdata on behalf of its callers' tests, so the cross-package check must scan + // ordinary sources too, not only `_test.go` files (2026-11-09, review of #85). The current go + // adapter puts each package's test files into `sourcePaths`, so with source metadata a package's + // members are exactly its sourcePaths and the test-file grouping only stands in for it. + this.goSourceFilesByOwner = new Map(); + const knownSources = new Set(); + for (const source of profile.goDependencyPaths?.sources ?? []) { + knownSources.add(source); + const dir = source.slice(0, source.lastIndexOf("/")); + const pkg = dir ? `./${dir}` : "."; + const members = this.goSourceFilesByOwner.get(pkg); + if (members) members.push(source); + else this.goSourceFilesByOwner.set(pkg, [source]); + } + // And readers outside every go-reported package (tests in nested modules or other languages, + // ordinary `.go` files excluded from package discovery) can name a package's testdata directly; + // they run with a different working directory, so the package-owned convention cannot hold and + // such a reference disqualifies the selective route (2026-11-09, review of #85). + const reportedGoTests = new Set(Object.keys(profile.goTestPackages ?? {})); + this.goOutsideReaders = new Set((this.sortedRepositoryFiles ?? []).filter(file => + !knownSources.has(file) && !reportedGoTests.has(file) + && (file.endsWith(".go") || this.isTestFile(file)))); if (!profile.goTestPackages || !this.readRepositoryFile) return this.goPackageTestDataOwner; const readRepositoryFile = this.readRepositoryFile; for (const path of new Set(delta.files.flatMap(allChangePaths))) { @@ -370,17 +414,28 @@ export class ImpactAnalyzer { const dataDir = [...segments.slice(0, at), "testdata"].join("/"); const filePattern = referencePattern(referenceTokens(path)); const dirPattern = referencePattern(referenceTokens(dataDir)); - for (const [pkg, readers] of byOwner) { + const disqualifies = (reader: string): boolean => { + const content = readRepositoryFile(reader); + // A listed test file that cannot be read could still name the testdata; without its + // contents the conservative answer is no selective route at all. + if (content === undefined) return true; + const references = isGoReaderFile(reader) ? goReaderReferences(content) : content; + return contentNamesPath(references, reader, path, filePattern, true) + || contentNamesPath(references, reader, dataDir, dirPattern, true); + }; + for (const [pkg, testFiles] of byOwner) { if (pkg === owner) continue; - for (const reader of readers) { - const content = readRepositoryFile(reader); - // A listed test file that cannot be read could still name the testdata; without its - // contents the conservative answer is no selective route at all. - if (content === undefined) return undefined; - if (contentNamesPath(content, reader, path, filePattern, true) - || contentNamesPath(content, reader, dataDir, dirPattern, true)) return undefined; - } - } + // With source metadata available a package's members are its sourcePaths (which include its test + // files); legacy cached profiles only ever knew the listed tests. + const goFiles = this.goSourceFilesByOwner.get(pkg); + const readers = goFiles?.length ? [...new Set([...goFiles, ...testFiles])] : testFiles; + if (readers.some(disqualifies)) return undefined; + } + // Readers outside the go-reported packages (tests of nested modules or other languages, ordinary + // `.go` files outside package discovery) never take the package-owned convention, since their + // tests run with a different working directory; a reference there disqualifies the selective + // route exactly like a cross-package one (2026-11-09, review of #85). + if ([...this.goOutsideReaders].some(disqualifies)) return undefined; return owner; } diff --git a/tests/repo/go-runtime-inputs.test.ts b/tests/repo/go-runtime-inputs.test.ts index 3a89573..37312d0 100644 --- a/tests/repo/go-runtime-inputs.test.ts +++ b/tests/repo/go-runtime-inputs.test.ts @@ -40,7 +40,7 @@ function planChange(graph: DependencyGraphResult, file: ChangedFile, analysis?: // descriptors. This suite does not need a Go installation and does not execute Go repository code. async function fixture( extraFiles: Record, - run: (graph: DependencyGraphResult, root: string) => void | Promise, + run: (graph: DependencyGraphResult, root: string, files: Record) => void | Promise, embeds: { production?: string[]; test?: string[]; external?: string[] } = {}, extraServiceTestName?: string, ) { @@ -71,11 +71,21 @@ async function fixture( { Dir: join(root, "service"), ImportPath: "example.com/project/service", Name: "service", GoFiles: ["service.go"], TestGoFiles: extraServiceTestName ? ["service_test.go", extraServiceTestName] : ["service_test.go"], Imports: ["example.com/project/p"], TestImports: ["testing"] }, { Dir: join(root, "unrelated"), ImportPath: "example.com/project/unrelated", Name: "unrelated", GoFiles: ["value.go"], TestGoFiles: ["value_test.go"], TestImports: ["testing"] }, ].map(pkg => JSON.stringify(pkg)).join("\n"); - goAdapter.analyze = context => analyzeGoMetadata(context, metadata); + goAdapter.analyze = context => { + // Mirror the real adapter's nested-module scoping (root-module scope excludes the nested + // roots from the metadata analysis and records them on the profile). + if (context.profile.diffciConfig?.go?.scope === "root-module") { + const nestedRoots = context.files.filter((file: string) => file.endsWith("/go.mod")).map((file: string) => file.slice(0, -"/go.mod".length)); + context.profile.goExcludedModuleRoots = nestedRoots; + const scopedFiles = context.files.filter((file: string) => !nestedRoots.some(root => file.startsWith(root))); + return analyzeGoMetadata({ ...context, files: scopedFiles }, metadata); + } + return analyzeGoMetadata(context, metadata); + }; const graph = await buildDependencyGraph({ repoPath: root }); assert.deepEqual(graph.adapterBlockers, []); assert.equal(graph.confidence, "COMPLETE"); - await run(graph, root); + await run(graph, root, files); } finally { goAdapter.analyze = originalAnalyze; rmSync(root, { recursive: true, force: true }); @@ -320,3 +330,30 @@ test("declared inputs add their tests to the package-owned testdata selection", assert.ok(impact.affectedTests.some(t => t.path === "p/value_test.go" && t.reasons.includes("GO_TESTDATA"))); }); }); + +test("non-test Go helper naming testdata", async () => { + // A shared helper in an ordinary `.go` file (the testutil/golden.go pattern) reads another + // package's testdata on behalf of its callers' tests; the cross-package check must scan + // ordinary sources, not only `_test.go` files (2026-11-09 review of #85). + const files = { + "p/testdata/settings.json": "expected", + "service/service.go": 'package service\nimport "os"\nfunc Read(name string) ([]byte, error) { return os.ReadFile("../p/testdata/settings.json") }\n', + }; + await fixture(files, (graph, root, files) => { + assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", allTests, { repoPath: root, fileContents: files }); + }); +}); + +test("a test outside every go-reported package naming the testdata keeps full validation", async () => { + // A test in a nested module (excluded from go list package discovery) reads the testdata + // directly; it runs with its own working directory, so the package-owned convention cannot + // hold and the reference must unlock full validation (2026-11-09 review of #85). + const files = { + "diffci.json": '{\n "go": { "scope": "root-module" }\n}\n', + "legacy/go.mod": "module example.com/legacy\n\ngo 1.24\n", + "legacy/tools/manual_test.go": 'package tools\nimport "os"\nimport "testing"\nfunc TestManual(t *testing.T) { if data, err := os.ReadFile("../../p/testdata/settings.json"); err != nil || string(data) != "expected" { t.Fatal("bad runtime input") } }\n', + }; + await fixture(files, (graph, root, files) => { + assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", allTests, { repoPath: root, fileContents: files }); + }); +}); From 1408f0341e8f062226d8830c5b30b5c4543f0ffd Mon Sep 17 00:00:00 2001 From: Shounak Joshi Date: Mon, 9 Nov 2026 20:49:19 +0530 Subject: [PATCH 3/3] fix: check every reported go package and modeled source module for testdata readers The cross-package check looped only over packages reporting \_test.go\ files, so a helper package without tests of its own (the usual \ estutil/golden.go\ shape) was never scanned: its ordinary source sits in goSourceFilesByOwner but not in the test-file grouping, and its files are knownSources, so the outside-readers pool skipped it too (2026-11-09, review of #85, point 1). The scan now covers the union of every reported package's members and listed tests, so a testless helper package's reference to another package's testdata disqualifies the selective route; legacy cached profiles without source metadata degrade to the listed tests exactly as before. On the pre-fix tree the new fixture reproduces the predicted miss (SELECTIVE [p/value_test.go] while service's test, which calls the helper, is skipped); with the fix the route is refused. The outside-readers pool also only took .go files and test-matched files, so a non-test source module in a modeled mixed repository went unchecked (2026-11-09, review of #85, point 2). The pool now takes every modeled source module (isSourceFilePath) and the python adapter's modeled sources; their references fall back like any other reader outside package discovery, which keeps the GO_TESTDATA reason set clean instead of reintroducing general reference resolution under the route's early return. A Go adapter plus any JavaScript file is itself a mixed-repo global blocker, so no healthy graph holds a JS helper; go + python repositories model cleanly, and the fixture stubs the python adapter's parse results the way the go metadata is stubbed. --- src/repo/impact.ts | 68 ++++++++++++++++++---------- tests/repo/go-runtime-inputs.test.ts | 49 ++++++++++++++++++++ 2 files changed, 94 insertions(+), 23 deletions(-) diff --git a/src/repo/impact.ts b/src/repo/impact.ts index c012be9..5cf1a05 100644 --- a/src/repo/impact.ts +++ b/src/repo/impact.ts @@ -297,10 +297,17 @@ export class ImpactAnalyzer { * from the current adapter put a package's test files into `sourcePaths` too, so when this map is * populated a package's members are exactly its sourcePaths; legacy cached profiles leave it empty. */ private goSourceFilesByOwner = new Map(); - /** Repository files that no go-reported package reported — tests in nested modules or other - * languages, ordinary `.go` files excluded from package discovery. Their references to a package's - * testdata disqualify the selective route like a cross-package one (a nested module cannot take the - * package-owned convention: its tests run with a different working directory). */ + /** Every go-reported package's testdata-reading candidates keyed by its directory: with source + * metadata the package's members (ordinary sources and test files, a package without tests of its + * own included — helpers there read other packages' testdata for their callers' tests); legacy + * cached profiles only ever knew the listed tests. The cross-package scan checks every package's + * entry except the owner (2026-11-09, review of #85). */ + private goReadersByPackage = new Map(); + /** Repository files that no go-reported package reported — code outside package discovery whose + * references to a package's testdata disqualify the selective route like a cross-package one: + * tests in nested modules or other languages, ordinary `.go` files excluded from package + * discovery, and non-test source modules in a mixed repository. Their readers run with a + * different working directory, so the package-owned convention cannot hold for them. */ private goOutsideReaders = new Set(); /** A changed `testdata/` path selects its owning package's tests when the parent directory is a Go * package with tests and no other package's test file names it by reference; every other @@ -348,8 +355,10 @@ export class ImpactAnalyzer { * qualify select that package's tests for the changed path instead of full validation; a legacy * cached profile without test-package evidence, repository contents being unavailable, a parent * directory that is no Go package with tests, `_`-prefixed or `.`-prefixed directories, or - * another package's test file naming the path or its `testdata` directory by literal reference - * (also, a `_`/`.`-prefixed directory inside the testdata tree) leave the fallback as before. + * another package naming the path or its `testdata` directory by literal reference (its test + * files or ordinary sources, a helper package with no tests of its own included, or a source + * module outside the go-reported packages; also, a `_`/`.`-prefixed directory inside the + * testdata tree) leave the fallback as before. */ private mapGoPackageTestData(delta: GitDelta, profile: RepositoryProfile): Map { this.goPackageTestDataOwner = new Map(); @@ -361,11 +370,9 @@ export class ImpactAnalyzer { else byOwner.set(owner, [test]); } this.goPackageTestFilesByOwner = byOwner; - // A helper in another package's ordinary `.go` file (the `testutil/golden.go` pattern) can read a - // package's testdata on behalf of its callers' tests, so the cross-package check must scan - // ordinary sources too, not only `_test.go` files (2026-11-09, review of #85). The current go - // adapter puts each package's test files into `sourcePaths`, so with source metadata a package's - // members are exactly its sourcePaths and the test-file grouping only stands in for it. + // The current go adapter puts each package's test files into `sourcePaths`, so with source + // metadata a package's members are exactly its sourcePaths and the test-file grouping only + // stands in for it. this.goSourceFilesByOwner = new Map(); const knownSources = new Set(); for (const source of profile.goDependencyPaths?.sources ?? []) { @@ -376,14 +383,32 @@ export class ImpactAnalyzer { if (members) members.push(source); else this.goSourceFilesByOwner.set(pkg, [source]); } + // The cross-package scan checks every go-reported package's readers, not only packages with + // tests: a helper in an ordinary `.go` file of a package with no tests of its own (the + // `testutil/golden.go` pattern) can read a package's testdata on behalf of its callers' tests, + // and such a package never enters the test-file grouping (2026-11-09, review of #85). With + // source metadata a package's readers are its sourcePaths (which include its test files); + // legacy cached profiles only ever knew the listed tests, so their entries keep just those. + this.goReadersByPackage = new Map(); + for (const pkg of new Set([...this.goSourceFilesByOwner.keys(), ...byOwner.keys()])) { + const sourceFiles = this.goSourceFilesByOwner.get(pkg); + const testFiles = byOwner.get(pkg); + if (sourceFiles?.length) { + this.goReadersByPackage.set(pkg, testFiles?.length ? [...new Set([...sourceFiles, ...testFiles])] : sourceFiles); + } else { + this.goReadersByPackage.set(pkg, testFiles ?? []); + } + } // And readers outside every go-reported package (tests in nested modules or other languages, - // ordinary `.go` files excluded from package discovery) can name a package's testdata directly; - // they run with a different working directory, so the package-owned convention cannot hold and - // such a reference disqualifies the selective route (2026-11-09, review of #85). + // ordinary `.go` files excluded from package discovery, and non-test source modules in a mixed + // repository) can name a package's testdata directly; they run with a different working + // directory, so the package-owned convention cannot hold and such a reference disqualifies + // the selective route (2026-11-09, review of #85). const reportedGoTests = new Set(Object.keys(profile.goTestPackages ?? {})); + const pythonSources = new Set(profile.pythonSources ?? []); this.goOutsideReaders = new Set((this.sortedRepositoryFiles ?? []).filter(file => !knownSources.has(file) && !reportedGoTests.has(file) - && (file.endsWith(".go") || this.isTestFile(file)))); + && (isSourceFilePath(file) || pythonSources.has(file) || this.isTestFile(file)))); if (!profile.goTestPackages || !this.readRepositoryFile) return this.goPackageTestDataOwner; const readRepositoryFile = this.readRepositoryFile; for (const path of new Set(delta.files.flatMap(allChangePaths))) { @@ -423,18 +448,15 @@ export class ImpactAnalyzer { return contentNamesPath(references, reader, path, filePattern, true) || contentNamesPath(references, reader, dataDir, dirPattern, true); }; - for (const [pkg, testFiles] of byOwner) { + for (const [pkg, readers] of this.goReadersByPackage) { if (pkg === owner) continue; - // With source metadata available a package's members are its sourcePaths (which include its test - // files); legacy cached profiles only ever knew the listed tests. - const goFiles = this.goSourceFilesByOwner.get(pkg); - const readers = goFiles?.length ? [...new Set([...goFiles, ...testFiles])] : testFiles; if (readers.some(disqualifies)) return undefined; } // Readers outside the go-reported packages (tests of nested modules or other languages, ordinary - // `.go` files outside package discovery) never take the package-owned convention, since their - // tests run with a different working directory; a reference there disqualifies the selective - // route exactly like a cross-package one (2026-11-09, review of #85). + // `.go` files outside package discovery, non-test source modules in a mixed repository) never + // take the package-owned convention, since their tests run with a different working directory; + // a reference there disqualifies the selective route exactly like a cross-package one + // (2026-11-09, review of #85). if ([...this.goOutsideReaders].some(disqualifies)) return undefined; return owner; } diff --git a/tests/repo/go-runtime-inputs.test.ts b/tests/repo/go-runtime-inputs.test.ts index 37312d0..7b305d8 100644 --- a/tests/repo/go-runtime-inputs.test.ts +++ b/tests/repo/go-runtime-inputs.test.ts @@ -8,6 +8,7 @@ import type { ChangedFile, GitDelta } from "../../src/git/types.js"; import { DefaultCIPlanner } from "../../src/planner/planner.js"; import { createTaskRegistry } from "../../src/planner/task-registry.js"; import { analyzeGoMetadata, goAdapter } from "../../src/repo/adapters/go.js"; +import { analyzePythonRepository, pythonAdapter, type PythonFileImports } from "../../src/repo/adapters/python.js"; import { repositoryFileReader } from "../../src/repo/file-references.js"; import { buildDependencyGraph, hydrateDependencyGraph } from "../../src/repo/graph.js"; import { ImpactAnalyzer } from "../../src/repo/impact.js"; @@ -43,6 +44,7 @@ async function fixture( run: (graph: DependencyGraphResult, root: string, files: Record) => void | Promise, embeds: { production?: string[]; test?: string[]; external?: string[] } = {}, extraServiceTestName?: string, + pythonImports?: Record, ) { const root = mkdtempSync(join(tmpdir(), "diffci-go-runtime-inputs-")); const declarations = (paths: string[] = []) => paths.map((path, index) => `//go:embed ${path}\nvar embedded${index} string\n`).join(""); @@ -61,6 +63,7 @@ async function fixture( ...extraFiles, }; const originalAnalyze = goAdapter.analyze; + let originalPythonAnalyze: typeof pythonAdapter.analyze | undefined; try { for (const [path, content] of Object.entries(files)) { mkdirSync(dirname(join(root, path)), { recursive: true }); @@ -70,6 +73,11 @@ async function fixture( { Dir: join(root, "p"), ImportPath: "example.com/project/p", Name: "p", GoFiles: ["value.go"], TestGoFiles: ["value_test.go"], Imports: ["os", ...(embeds.production?.length ? ["embed"] : [])], TestImports: ["testing", ...(embeds.test?.length ? ["embed"] : [])], EmbedFiles: embeds.production, TestEmbedFiles: embeds.test, XTestGoFiles: embeds.external?.length ? ["value_external_test.go"] : [], XTestImports: embeds.external?.length ? ["testing", "embed"] : [], XTestEmbedFiles: embeds.external }, { Dir: join(root, "service"), ImportPath: "example.com/project/service", Name: "service", GoFiles: ["service.go"], TestGoFiles: extraServiceTestName ? ["service_test.go", extraServiceTestName] : ["service_test.go"], Imports: ["example.com/project/p"], TestImports: ["testing"] }, { Dir: join(root, "unrelated"), ImportPath: "example.com/project/unrelated", Name: "unrelated", GoFiles: ["value.go"], TestGoFiles: ["value_test.go"], TestImports: ["testing"] }, + // A helper package reported without test files is still a reported package: mirrors whatever + // testutil sources the fixture carries (currently golden.go importing "os"). + ...(Object.keys(files).some(path => /^testutil\/.+\.go$/.test(path) && !path.endsWith("_test.go")) ? [{ + Dir: join(root, "testutil"), ImportPath: "example.com/project/testutil", Name: "testutil", GoFiles: Object.keys(files).filter(path => /^testutil\/.+\.go$/.test(path) && !path.endsWith("_test.go")).map(path => path.slice("testutil/".length)), TestGoFiles: [], Imports: ["os"], TestImports: [], + }] : []), ].map(pkg => JSON.stringify(pkg)).join("\n"); goAdapter.analyze = context => { // Mirror the real adapter's nested-module scoping (root-module scope excludes the nested @@ -82,12 +90,19 @@ async function fixture( } return analyzeGoMetadata(context, metadata); }; + // The python adapter is stubbed the same way: hand the resolver the file parses directly, so + // the mixed-repository fixtures need no interpreter and still exercise real adapter logic. + if (pythonImports) { + originalPythonAnalyze = pythonAdapter.analyze; + pythonAdapter.analyze = context => analyzePythonRepository(context, listed => Object.fromEntries(listed.map(file => [file, pythonImports[file] ?? { imports: [] }]))); + } const graph = await buildDependencyGraph({ repoPath: root }); assert.deepEqual(graph.adapterBlockers, []); assert.equal(graph.confidence, "COMPLETE"); await run(graph, root, files); } finally { goAdapter.analyze = originalAnalyze; + if (originalPythonAnalyze) pythonAdapter.analyze = originalPythonAnalyze; rmSync(root, { recursive: true, force: true }); } } @@ -357,3 +372,37 @@ test("a test outside every go-reported package naming the testdata keeps full va assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", allTests, { repoPath: root, fileContents: files }); }); }); + +test("a helper package without tests of its own naming testdata keeps full validation", async () => { + // The usual testutil/golden.go shape: a helper package with no tests of its own reads another + // package's testdata for the callers' tests (service imports testutil below and its test calls + // Golden). A cross-package scan over packages with tests alone never reaches that package, + // since it contributes no `_test.go` file of its own (2026-11-09 review of #85). + const files = { + "p/testdata/settings.json": "expected", + "testutil/golden.go": 'package testutil\nimport "os"\nfunc Golden() ([]byte, error) { return os.ReadFile("../p/testdata/settings.json") }\n', + "service/service_test.go": 'package service\nimport "testing"\nimport "example.com/project/testutil"\nfunc TestGolden(t *testing.T) { if _, err := testutil.Golden(); err != nil { t.Fatal(err) } }\n', + }; + await fixture(files, (graph, root, files) => { + assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", allTests, { repoPath: root, fileContents: files }); + }); +}); + +test("a python helper in a modeled mixed repository naming testdata keeps full validation", async () => { + // In a mixed (go + python) repository a non-test helper reads a package's testdata for the test + // that imports it; the outside-readers pool only took `.go` files and test-matched files, so the + // helper's reference was never checked (2026-11-09 review of #85). A Go adapter plus any + // JavaScript file is itself a mixed-repo blocker, so the JS variant cannot reach a healthy graph. + const files = { + "p/testdata/settings.json": "expected", + "pyproject.toml": '[project]\nname = "mixed"\n', + "tools/snapshot.py": 'from pathlib import Path\n\ndef snapshot() -> str:\n return Path("../p/testdata/settings.json").read_text()\n', + "tools/test_snapshot.py": 'from .snapshot import snapshot\n\ndef test_snapshot():\n assert snapshot() == "expected"\n', + }; + await fixture(files, (graph, root, files) => { + assertFullRuntimeFallback(graph, { path: "p/testdata/settings.json", changeType: "modified" }, "p/testdata/settings.json", [...allTests, "tools/test_snapshot.py"], { repoPath: root, fileContents: files }); + }, {}, undefined, { + "tools/snapshot.py": { imports: [] }, + "tools/test_snapshot.py": { imports: [[".snapshot", 1, ["snapshot"]]] }, + }); +});