test(engine): tighten test-suite hermeticity and parent-aware subtest references#149
Conversation
… references
Two follow-ups deferred from the apply-path regression PR:
1. Scope the workflow-leakage scan to git-tracked files. The previous
filepath.WalkDir-based walk failed go test ./... on any working-tree
artefact (notes, build scratch, generated YAML) that happened to
contain a banned phrase, even when no committed source violated the
rule. The new committedTextFiles helper shells out to git ls-files
so the guard is hermetic with respect to working-tree state.
2. Make TestNoDanglingSubtestReferencesInSource parent-aware. The
previous flat map[slug]struct{} collector accepted a citation
TestX/wrong as long as ANY test in the package had a subtest named
"wrong" — the wrong-parent drift the guard claims to catch slipped
through silently. The new findDanglingSubtestReferences helper
parses test files via go/parser, collects t.Run literals scoped to
each parent test function, and rejects citations whose slug does
not match a subtest under that specific parent (exact or prefix
match within the same parent).
Both helpers get focused unit tests with synthetic fixtures so the
contracts survive future refactors:
- TestCommittedTextFilesIgnoresUntrackedArtefacts builds a tiny
temporary git repo with one tracked + one untracked file and
asserts the helper returns only the tracked one.
- TestFindDanglingSubtestReferencesIsParentAware constructs a
synthetic test source with two parent tests, asserts the same-parent
exact match passes, the same-parent prefix match passes, and the
cross-parent slug match is flagged as dangling.
The synthetic test source in (2) is assembled via string concatenation
so engine_test.go itself does not embed the literal Test<Name>/slug
patterns the production scanner would otherwise flag against this
package's real test list.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the reliability of the test suite by ensuring scans only target committed files and by implementing scope-aware detection of dangling subtest references. Key additions include the committedTextFiles helper, which utilizes git ls-files, and findDanglingSubtestReferences, which uses AST parsing to verify subtest citations. Review feedback suggests refining the t.Run regex to account for whitespace and optimizing the file parsing loop to avoid redundant disk I/O and FileSet allocations.
| // citation, lift the slug into a literal at the t.Run call site or | ||
| // extend the collector to walk tt.name resolutions. | ||
| func findDanglingSubtestReferences(testFiles []string) ([]danglingSubtestRef, error) { | ||
| subtestRe := regexp.MustCompile(`t\.Run\("([^"\\]+)"`) |
There was a problem hiding this comment.
The current regex for capturing t.Run calls is quite restrictive. It will miss calls that include whitespace or newlines between the function name, the parenthesis, and the string literal (e.g., t.Run( "name", ...) or t.Run(\n"name", ...)). This can lead to false positive "dangling reference" errors when the scanner fails to identify valid subtests. Consider allowing optional whitespace in the pattern.
| subtestRe := regexp.MustCompile(`t\.Run\("([^"\\]+)"`) | |
| subtestRe := regexp.MustCompile(`t\.Run\s*\(\s*"([^"\\]+)"`) |
There was a problem hiding this comment.
Applied in a8cb031 — subtestRe is now t\.Run\s*\(\s*"([^"\\]+)". gofmt-conformant Go has no whitespace there, but the helper now also handles hand-formatted or alternate-formatter sources without false-positive dangling-ref errors.
| for _, path := range testFiles { | ||
| fset := token.NewFileSet() | ||
| file, err := parser.ParseFile(fset, path, nil, parser.ParseComments) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "parse %s", path) | ||
| } | ||
| body, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "read %s", path) | ||
| } | ||
| bodies[path] = body |
There was a problem hiding this comment.
This loop currently reads each test file twice: once implicitly via parser.ParseFile (when the source argument is nil) and once explicitly via os.ReadFile. Additionally, it recreates the token.FileSet for every file. You can optimize this by reading the file once and passing the content to the parser, and by reusing a single FileSet for the entire scan. This reduces syscalls and memory allocations during the repo-wide scan.
| for _, path := range testFiles { | |
| fset := token.NewFileSet() | |
| file, err := parser.ParseFile(fset, path, nil, parser.ParseComments) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "parse %s", path) | |
| } | |
| body, err := os.ReadFile(path) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "read %s", path) | |
| } | |
| bodies[path] = body | |
| fset := token.NewFileSet() | |
| for _, path := range testFiles { | |
| body, err := os.ReadFile(path) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "read %s", path) | |
| } | |
| bodies[path] = body | |
| file, err := parser.ParseFile(fset, path, body, parser.ParseComments) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "parse %s", path) | |
| } |
There was a problem hiding this comment.
Applied in 24f31f6 — os.ReadFile(path) runs once per file and the bytes are passed to parser.ParseFile(fset, path, body, parser.ParseComments) so the parser does not reopen the file. The token.FileSet is hoisted out of the loop, so a single fset covers the whole walk.
Address review feedback from gemini-code-assist on pkg/engine/engine_test.go:95: tighten subtestRe to allow optional whitespace between t.Run, the parenthesis, and the string literal (t\.Run\s*\(\s*"...). gofmt-conformant Go has no such whitespace, but the helper now also handles hand-formatted or alternate-formatter sources that another consumer might feed it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from gemini-code-assist on pkg/engine/engine_test.go:116: pass the file bytes to parser.ParseFile so the file is read once per scan instead of twice (the previous form let parser.ParseFile reopen the file when src=nil and then os.ReadFile read it again). Hoist the token.FileSet out of the loop so a single fset covers the whole walk instead of allocating one per file. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
2 participants
What changed
Two follow-ups deferred from the apply-path regression PR:
TestNoWorkflowLeakageInRepoSourceis now hermetic. The previous walk usedfilepath.WalkDirover the module root, so any working-tree artefact (notes, build scratch, generated YAML, plan files) that happened to contain a banned phrase failedgo test ./...even when committed source was clean. The newcommittedTextFileshelper shells out togit ls-files -zso the guard is a function of the index, not the working tree. The testt.Skips when.gitis absent (release tarballs, vendored copies) instead of fataling.TestNoDanglingSubtestReferencesInSourceis now parent-aware. The previous flatmap[slug]struct{}collector accepted a citationTestX/wrongas long as ANY test in the package had a subtest namedwrong— the wrong-parent drift the guard claims to catch slipped through. The newfindDanglingSubtestReferenceshelper parses test files viago/parser, collectst.Runliterals scoped to each parent test function, and returns dangling refs with the file path so a future failure points at the exact source location.Tests
Two focused unit tests with synthetic fixtures pin the contracts:
TestCommittedTextFilesIgnoresUntrackedArtefactsbuilds a tiny temporary git repo with one tracked + one untracked file and asserts the helper returns only the tracked one.TestFindDanglingSubtestReferencesIsParentAwareconstructs a synthetic test source with two parent tests and asserts the same-parent exact match passes, the same-parent prefix match passes, and the cross-parent slug match is flagged as dangling. The synthetic source is assembled via string concatenation (tag := "Tes" + "t",runFn := "t.Ru" + "n") so this test file does not embed literals the production scanners would otherwise flag against this package's real test list.Both helpers are documented with the literal-only
t.Runregex caveat (table-drivent.Run(tt.name, ...)subtests are invisible to the collector), so a future maintainer who hits a false positive on a runtime-slug citation knows where to look.Closes #137.