test: add golden files and RunWithSuggestedFixes for 3 autofix linters#43376
Conversation
…r 3 linters Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…contains, sprintfint, stringreplaceminusone Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage — Run §28706384347
Score breakdown: impact 22 + urgency 12 + quality 14 Adds golden files and
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens linter autofix testing by switching three analyzers’ tests to validate their SuggestedFix text edits via golden files, rather than only validating diagnostic locations/messages.
Changes:
- Added
.goldenfixtures forstringsindexcontains,sprintfint, andstringreplaceminusoneto assert exact post-fix output. - Updated the three linter tests to use
analysistest.RunWithSuggestedFixesso suggested fixes are exercised and verified. - Kept analyzer implementations unchanged; changes are limited to test harness + fixtures.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go.golden | Golden output validating the strings.Index(...) containment comparisons are rewritten to strings.Contains forms. |
| pkg/linters/stringsindexcontains/stringsindexcontains_test.go | Switches test runner to RunWithSuggestedFixes to verify fixes against the golden file. |
| pkg/linters/stringreplaceminusone/testdata/src/stringreplaceminusone/stringreplaceminusone.go.golden | Golden output validating strings.Replace(..., -1) is rewritten to strings.ReplaceAll(...). |
| pkg/linters/stringreplaceminusone/stringreplaceminusone_test.go | Switches test runner to RunWithSuggestedFixes to verify fixes against the golden file. |
| pkg/linters/sprintfint/testdata/src/sprintfint/sprintfint.go.golden | Golden output validating fmt.Sprintf("%d", x) is rewritten to strconv.Itoa(x) for int. |
| pkg/linters/sprintfint/sprintfint_test.go | Switches test runner to RunWithSuggestedFixes to verify fixes against the golden file. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Low
|
@copilot please run the
|
Ran the
No unresolved review threads. Branch is up to date with the pushed commits. PR is ready for a human to re-trigger CI and merge. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 139.9 AIC · ⌖ 9.21 AIC · ⊞ 6.6K
Comment /matt to run again
| // bad demonstrates the flagged pattern: fmt.Sprintf with a single "%d" verb | ||
| // and an int argument, which should be replaced by strconv.Itoa. | ||
| func bad(n int) string { | ||
| return strconv.Itoa(n) // want `use strconv\.Itoa\(x\) instead of fmt\.Sprintf\("%d", x\)` |
There was a problem hiding this comment.
[/tdd] The // want comments on fixed lines in the golden file are never evaluated by RunWithSuggestedFixes — they are dead annotations that could mislead future readers.
💡 Why this matters and what to do
In .go.golden files, analysistest.RunWithSuggestedFixes compares the post-fix source text verbatim. // want patterns are only processed in the original .go source files. Retaining them in the golden file creates confusion: readers may think a diagnostic is still expected on the fixed line.
Established golden files in this repo (lenstringsplit.go.golden, fprintlnsprintf.go.golden, lenstringzero.go.golden) do not include // want comments — they show clean, expected output. Follow that convention:
// bad demonstrates the flagged pattern...
func bad(n int) string {
return strconv.Itoa(n)
}Remove the // want ... suffixes from lines 14 and 19 in this file, and the corresponding lines in stringsindexcontains.go.golden and stringreplaceminusone.go.golden.
@copilot please address this.
|
|
||
| func badContains(s, sub string) bool { | ||
| return strings.Contains(s, sub) // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison` | ||
| } |
There was a problem hiding this comment.
[/tdd] The golden file retains // want comments on fixed lines — same issue as in sprintfint.go.golden. These comments are not evaluated by RunWithSuggestedFixes and diverge from the pattern used by other golden files in this repo.
💡 Suggestion
All 10 badXxx functions carry a // want suffix in this golden file (lines 6, 10, 14, 18, 22, 26, 30, 34, 38, 42). Since the golden file represents the expected fixed source, these comments serve no test-assertion purpose. Removing them keeps the golden files consistent with the rest of the repo and eliminates any ambiguity about whether a diagnostic is still expected post-fix.
@copilot please address this.
| const replaceAllConst = -1 | ||
|
|
||
| func badLiteral(s string) string { | ||
| return strings.ReplaceAll(s, "o", "0") // want `use strings\.ReplaceAll instead of strings\.Replace with n=-1` |
There was a problem hiding this comment.
[/tdd] Same // want comment pattern issue — golden files should represent clean, fixed source without want directives.
💡 Suggestion
Lines 9 and 13 carry // want annotations. As noted in the sibling files, these are not asserted by RunWithSuggestedFixes. The established golden-file convention in this repo is to show purely the expected fixed output without any want markers. Stripping these keeps all three new golden files consistent with each other and with the existing ones.
@copilot please address this.
Ran
No unresolved review threads. PR is ready for a human to re-trigger CI and merge. |
🤖 PR Triage — Run §28715668077
Carry-over from prior run. Previously draft, now undrafted. Design Decision Gate requires ADR. Batch with other low-risk test PRs.
|
|
@copilot please run the
|
…erification-gap Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…erification-gap Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch refreshed with latest main ( |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/28718358099
|
Completion summary for maintainers
What this PR does: Switches 3 linter tests ( Hand-off: Re-trigger CI (close/reopen, |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
Completion summary for maintainers
Hand-off: Re-trigger CI (close/reopen, |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
The ADR file is already committed on the branch at A maintainer needs to add the following line to the PR body (e.g. after the "No production analyzer code changed." line): Once the PR body is updated the ADR gate can be re-triggered. Local validation remains fully green; no code changes are needed. Summary:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
#43376) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
3 of 9
SuggestedFix-emitting linters (stringsindexcontains,sprintfint,stringreplaceminusone) usedanalysistest.Run, which only validates diagnostic positions/messages — never the actual text edits. Their fix builders were completely unexercised.Changes
Golden files added for each linter capturing exact post-fix source:
stringsindexcontains: all 6 comparison forms + 4 yoda variants →strings.Contains/!strings.Containssprintfint:fmt.Sprintf("%d", x)→strconv.Itoa(x)(int variable and literal)stringreplaceminusone:strings.Replace(..., -1)→strings.ReplaceAll(...)(literal and named constant)Test entrypoints switched from
analysistest.Run→analysistest.RunWithSuggestedFixes, matching the pattern already used byfprintlnsprintf,lenstringzero,tolowerequalfold, etc.No production analyzer code changed.