Skip to content

GO_TESTDATA selective route follow-ups: bare testdata token over-disqualifies; per-dataDir reader-scan memo #90

Open

Description

@shounakjoshi88-a11y

Follow-ups from the review of #85 (both noted as non-blocking there — this issue tracks them). Neither is a safety problem: both currently fail safe by disqualifying the selective route more often than necessary, they just limit how much of #83 the GO_TESTDATA route delivers in real repos.

1. The bare "testdata" reference token over-disqualifies the route

In goPackageTestDataOwnerFor (src/repo/impact.ts), the disqualification check builds its patterns from referenceTokens(dataDir) for p/testdata, and that token list includes the bare testdata literal. With ancestors: true, contentNamesPath matches a path-less word, so any other package containing the very common idioms

filepath.Join("testdata", name)
os.ReadDir("testdata")

disqualifies p's route — even though in package q, a bare testdata resolves to q/testdata, which has nothing to do with p/testdata.

In a repo where two or more packages each use their own testdata, the selective route would rarely be chosen (always falling back), failing safe but wasting the analysis.

Suggested direction (from the review of #85): resolve a bare testdata token against the reader's own package directory and treat it as a reference to that package's testdata only. A slash-bearing reference (p/testdata/…) should keep its current treatment, since that genuinely names another package's tree.

2. Reader scans are repeated per changed file — memoize per dataDir

Every changed file under the same testdata directory runs the same disqualifies scan over goReadersByPackage and goOutsideReaders again: same readers, same contents, same result. On a golden-file refresh that touches many files in one tree, the scan is repeated for each.

Suggested direction: cache the scan outcome keyed by the owning dataDir (valid for one analysis run, keeping the current readRepositoryFile single-read semantics), so a large refresh only pays once per tree.

Happy to take these in a separate PR if a maintainer confirms scope.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions