Add goleak-based package leak checks for pkg/cli and pkg/console tests#47865
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/cli and pkg/console tests
|
Triage | Category: Recommended action: Score breakdown: impact=10 (test quality), urgency=10 (draft), quality=10 (no CI yet)
|
There was a problem hiding this comment.
Pull request overview
Adds package-level goroutine leak detection to unit tests in pkg/cli and pkg/console.
Changes:
- Adds
goleaktest dependency. - Integrates leak checks into both packages’
TestMain. - Preserves CLI test-artifact cleanup.
Show a summary per file
| File | Description |
|---|---|
pkg/console/test_main_test.go |
Adds package-level leak verification. |
pkg/cli/test_main_test.go |
Adds leak verification while preserving cleanup. |
go.mod |
Declares the goleak dependency. |
go.sum |
Adds dependency checksums. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/4 changed files
- Comments generated: 1
- Review effort level: Medium
| go.opentelemetry.io/otel v1.44.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.44.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.44.0 // indirect | ||
| go.uber.org/goleak v1.3.0 // indirect |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47865 does not have the 'implementation' label and has only 23 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Review: Add goleak-based package leak checks
The implementation is correct — goleak.VerifyTestMain is wired properly in both packages, and the goleak.Cleanup pattern in pkg/cli correctly preserves the existing artifact cleanup.
One issue to address (non-blocking): go.uber.org/goleak is added as // indirect in go.mod, but it is directly imported by test files in pkg/cli and pkg/console. Direct test imports are direct dependencies. Running go mod tidy should promote it to the direct block and remove the // indirect marker. CI go mod tidy checks will flag this.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 12.3 AIC · ⌖ 7.3 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — one consistency issue found between the two new TestMain files.
📋 Key Themes & Highlights
Key Themes
- Build tag inconsistency:
pkg/console/test_main_test.gohas//go:build !integrationbutpkg/cli/test_main_test.godoes not, so goleak fires on different test modes across the two packages. go.modindirect marker:go.uber.org/goleakis a direct test dependency and should not be marked// indirect(already noted in an existing review comment).
Positive Highlights
- ✅ Clean use of
goleak.Cleanup(...)to preserve existing filesystem teardown while integrating leak detection. - ✅ Minimal, surgical diff — only the two target packages are touched.
- ✅
goleak.VerifyTestMain(m)is the idiomatic pattern; no custom wiring needed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 17.4 AIC · ⌖ 4.64 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -17,15 +19,12 @@ func TestMain(m *testing.M) { | |||
| panic("Failed to get current working directory: " + err.Error()) | |||
There was a problem hiding this comment.
[/codebase-design] Missing //go:build !integration build tag — pkg/console/test_main_test.go has it but pkg/cli/test_main_test.go does not. This means goleak runs during integration tests for pkg/cli but not for pkg/console, creating inconsistent leak coverage.
💡 Suggested fix
Add the build constraint at the top of pkg/cli/test_main_test.go to match pkg/console:
(go/redacted):build !integration
package cliOr remove the tag from pkg/console/test_main_test.go if goleak should cover integration test runs in both packages.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 70/100 — Acceptable
📊 Metrics (2 TestMain entries)
Analysis Notes
Verdict
|
There was a problem hiding this comment.
REQUEST_CHANGES — one build-constraint bug must be fixed before merge.
Missing !js && !wasm build tags in pkg/console/test_main_test.go
The new TestMain only carries //go:build !integration, but go.uber.org/goleak doesn't compile for js/wasm targets. Every other wasm-incompatible test in pkg/console (e.g., confirm_test.go, prompt_form_test.go) uses //go:build !integration && !js && !wasm. Without the matching constraint here, any wasm/js build of the test binary will fail to compile.
The go.mod indirect-vs-direct classification is also wrong (goleak is directly imported by test files), but that's a pre-existing CI concern already noted in comments.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 35.6 AIC · ⌖ 4.61 AIC · ⊞ 5.7K
Comment /review to run again
| @@ -0,0 +1,13 @@ | |||
| //go:build !integration | |||
There was a problem hiding this comment.
Missing !js && !wasm build constraints: goleak.VerifyTestMain is imported, but go.uber.org/goleak is not wasm/js-compatible. Other test files in this package (e.g., confirm_test.go, prompt_form_test.go) gate themselves with !js && !wasm, but this new TestMain does not—breaking compilation for those targets.
💡 Suggested fix
Change line 1 from:
(go/redacted):build !integrationto:
(go/redacted):build !integration && !js && !wasmThis matches the constraint pattern in the other wasm-incompatible test files in pkg/console.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran |
|
🎉 This pull request is included in a new release. Release: |
The repo has goroutine-heavy code paths but no CI guard for leaked goroutines in tests. This change adds package-level
goleakverification in the first two target packages (pkg/cli,pkg/console) to catch regressions early.Test lifecycle guardrails
go.uber.org/goleakto test dependencies.goleak.VerifyTestMain(...)in:pkg/clipkg/consolepkg/cliTestMain integrationTestMainto run undergoleak.VerifyTestMain..githubtest-artifact cleanup by moving it intogoleak.Cleanup(...).pkg/consolecoverageTestMainforpkg/consolethat appliesgoleak.VerifyTestMain.