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. |
|
✅ Test Quality Sentinel completed test quality analysis. |
…uggestedFixes Auto-generated by the Design Decision Gate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to business logic directories (147 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Analysis NotesEach test was upgraded from analysistest.Run to analysistest.RunWithSuggestedFixes, enforcing a stronger behavioral contract: the linter must now also produce correct autofix suggestions that match the golden files, not just emit the correct diagnostics. The three new golden files each contain positive cases with want directives verifying the fixed form of flagged patterns, and negative cases verifying that non-flagged patterns are left untouched. The stringsindexcontains golden file is notably thorough, covering 10 distinct bad patterns and 5 non-flagged cases. Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The golden files and RunWithSuggestedFixes switch are correct across all three linters.
What was verified:
sprintfint:bad/badLiteral→strconv.Itoa;fmtretained (still needed by 5 unflagged functions + the nolint-suppressed case); import block unchanged ✓stringreplaceminusone: bothbadLiteralandbadNamedConst→strings.ReplaceAll; orphanedreplaceAllConststays (unused constants are not a Go error) ✓stringsindexcontains: all 10 flagged comparisons correctly rewritten; yoda forms (0 > strings.Index) →!strings.Contains✓// wantannotation style in golden files matches the established project convention (lenstringzero,tolowerequalfold) ✓
This PR closes a real gap: fix generators for these three linters were previously completely unexercised by tests.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 58.4 AIC · ⌖ 6.44 AIC · ⊞ 4.9K
|
@copilot please run the
|
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>
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.