test: fix golangci-lint errcheck/staticcheck issues in test code#323
Merged
Conversation
First step of replacing the sunset Go Report Card with golangci-lint. Resolve all 54 lint findings in test files (errcheck unchecked error returns, ST1018 Unicode control chars, SA6003 range over []rune, ineffassign): setup calls now assert errors via t.Fatalf, cleanup and best-effort calls use explicit `_ =`. No production code touched; make check and make test both pass.
Contributor
|
✅ OpenCodeReview: No supported files changed. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR removes existing golangci-lint findings in test code only to prepare the repo for adopting golangci-lint as a CI/static-analysis gate.
Changes:
- Replaced unchecked error returns in tests with
t.Fatalf(...)for setup-critical operations and explicit ignores (_ = ...) for best-effort cleanup. - Fixed staticcheck/ST1018 issues by escaping Unicode control characters in string literals and simplified a rune-range loop.
- Addressed an ineffectual assignment by adding an assertion that validates the intended slice behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/viewer/store_load_test.go | Ignore cleanup Chmod errors explicitly to satisfy errcheck. |
| internal/viewer/server_extra_test.go | Explicitly ignore Close() error in test. |
| internal/viewer/handler_test.go | Ignore cleanup Chmod errors explicitly. |
| internal/telemetry/provider_test.go | Explicitly ignore os.Unsetenv errors. |
| internal/telemetry/exporter_test.go | Ignore pipe close errors and explicitly ignore io.Copy return values. |
| internal/telemetry/events_test.go | Explicitly ignore pipe Close() error. |
| internal/telemetry/config_test.go | Check WriteFile errors; explicitly ignore Unsetenv errors. |
| internal/session/persist_test.go | Ignore file close / flush errors explicitly. |
| internal/pathutil/path_test.go | Ignore cleanup Chdir error explicitly. |
| internal/model/model_test.go | Check json.Unmarshal error paths explicitly. |
| internal/mcp/stdio_test.go | Ignore Close() error explicitly in deferred cleanup. |
| internal/mcp/client_test.go | Ignore Close() error explicitly in deferred cleanup. |
| internal/llm/resolver_test.go | Check os.WriteFile errors for generated config fixtures. |
| internal/llm/message_test.go | Check os.WriteFile errors for test RC fixtures. |
| internal/config/toolsconfig/toolsconfig_test.go | Check os.WriteFile errors for tools.json fixtures. |
| internal/agent/util_test.go | Add assertion to ensure appended copy is actually validated. |
| cmd/opencodereview/shared_test.go | Check Chdir error and ignore deferred restore error explicitly. |
| cmd/opencodereview/provider_tui_test.go | Simplify range []rune("abc") to range "abc". |
| cmd/opencodereview/provider_cmd_test.go | Explicitly ignore pipe Close() error. |
| cmd/opencodereview/output_test.go | Replace raw Unicode control chars with escaped sequences. |
| cmd/opencodereview/output_helpers_test.go | Explicitly ignore pipe Close() and ReadFrom return values; check Unmarshal in some cases. |
| cmd/opencodereview/git_test.go | Check git command results and Chdir error; ignore deferred restore error explicitly. |
| cmd/opencodereview/emit_run_result_test.go | Ignore tracer provider Shutdown() error explicitly in deferred cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+82
to
+86
| origDir, _ := os.Getwd() | ||
| defer os.Chdir(origDir) | ||
| os.Chdir(dir) | ||
| defer func() { _ = os.Chdir(origDir) }() | ||
| if err := os.Chdir(dir); err != nil { | ||
| t.Fatalf("chdir: %v", err) | ||
| } |
Comment on lines
+102
to
+106
| origDir, _ := os.Getwd() | ||
| defer os.Chdir(origDir) | ||
| os.Chdir(dir) | ||
| defer func() { _ = os.Chdir(origDir) }() | ||
| if err := os.Chdir(dir); err != nil { | ||
| t.Fatalf("chdir: %v", err) | ||
| } |
Comment on lines
166
to
170
| if sh.persist != nil { | ||
| sh.persist.mu.Lock() | ||
| sh.persist.writer.Flush() | ||
| _ = sh.persist.writer.Flush() | ||
| sh.persist.mu.Unlock() | ||
| } |
Comment on lines
164
to
165
| _ = w.Close() | ||
| os.Stderr = oldStderr |
Comment on lines
+186
to
+187
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) | ||
| _, _ = io.Copy(&buf, r) |
Address Copilot review feedback on #323. Some errors previously silenced with `_ =` to satisfy errcheck actually affect test correctness or can pollute later tests, so fail loudly instead: - shared_test.go / git_test.go: check os.Getwd() and report deferred os.Chdir restore failures (t.Errorf) so a failed restore can't leave later tests running in the wrong working directory. - persist_test.go: fail on writer.Flush() error since it is a precondition for the JSONL assertions. - exporter_test.go: fail on pipe Close()/io.Copy errors that determine the asserted stderr output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes all 54 golangci-lint findings in test code (production code untouched):
os.WriteFile,os.Chdirinto the target dir,Shutdown) now assert viat.Fatalf; cleanup / best-effort calls (defer Close,os.Chmod,os.Unsetenv, stdout-capture scaffolding) use an explicit_ =.output_test.goreplaced with�/�escapes.range []rune("abc")simplified torange "abc"inprovider_tui_test.go.util_test.go(added an assertion so the appended copy is actually checked).The bulk is
internal/llm/resolver_test.go(36os.WriteFilesetup calls).Why
Go Report Card has been sunset (its badge was removed in #319). This is the first step of replacing it with golangci-lint as a real static-analysis gate. Clearing the existing lint findings first — starting with zero-risk test-only changes — keeps the eventual "wire up golangci-lint + CI + badge" change decoupled and easy to review. Production-code findings will follow in a separate PR.
Verification
make check(gofmt + vet) passesmake testpasses (all 22 packages, no failures)golangci-lint run --max-issues-per-linter 0 --max-same-issues 0 ./...reports zero remaining test-code issues