Improve test quality for pkg/console/confirm_test.go - #51434
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 42 new lines of code in business logic directories (threshold: 100).
|
There was a problem hiding this comment.
Pull request overview
Improves behavioral and edge-case coverage for console confirmation prompts.
Changes:
- Replaces the no-op
ConfirmActiontest with non-TTY cases. - Adds uppercase, whitespace, and EOF cases for
showTextConfirm.
Show a summary per file
| File | Description |
|---|---|
pkg/console/confirm_test.go |
Expands confirmation prompt tests. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| oldStdin := os.Stdin | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { os.Stdin = oldStdin }) | ||
| t.Cleanup(func() { r.Close() }) | ||
| os.Stdin = r |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two correctness issues in the new test and one missing coverage gap.
📋 Key Themes & Highlights
Key Themes
- Cleanup ordering bug:
t.Cleanupregistrations are LIFO, sor.Close()fires beforeos.Stdinis restored — opposite of intended order. - Unsynchronised goroutine: the goroutine writing to the pipe can race against cleanup if the test path panics or returns early; synchronous writes are simpler and safe for short inputs.
- Missing EOF case in
TestConfirmAction_NonTTY: the table covers yes/no/invalid but not empty/EOF, which is already tested inTestShowTextConfirm.
Positive Highlights
- ✅ Replacing the
_ = ConfirmActionno-op with a real exercise of the non-TTY path is exactly the right improvement. - ✅ Table-driven structure is clean and consistent with the rest of the file.
- ✅ New edge cases in
TestShowTextConfirm(uppercase, whitespace-padded) are well-chosen.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 25.8 AIC · ⌖ 9.99 AIC · ⊞ 7.1K
Comment /matt to run again
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { os.Stdin = oldStdin }) | ||
| t.Cleanup(func() { r.Close() }) |
There was a problem hiding this comment.
[/tdd] t.Cleanup registrations are LIFO, so r.Close() fires before os.Stdin = oldStdin is restored — inverted from the intended order.
💡 Fix: swap registration order
Register restore-stdin after r.Close so LIFO runs restore first:
t.Cleanup(func() { r.Close() }) // registered 1st → called 2nd
t.Cleanup(func() { os.Stdin = oldStdin }) // registered 2nd → called 1stCurrently harmless (ConfirmAction has returned by cleanup time), but the ordering will silently cause problems if cleanup logic ever changes.
@copilot please address this.
| result, err := ConfirmAction("Delete all workflows?", "Yes, delete", "Cancel") | ||
| if tt.wantErr { | ||
| require.Error(t, err) | ||
| } else { |
There was a problem hiding this comment.
[/tdd] The goroutine writing to w is not synchronised with the test's cleanup — if the test subtree panics or ConfirmAction returns early, w.WriteString / w.Close() may race against r.Close() in the cleanup.
💡 Use a done channel or write synchronously for short inputs
For short inputs you can write and close synchronously before calling ConfirmAction, since os.Pipe has a kernel buffer large enough for a few bytes:
_, _ = w.WriteString(tt.input)
w.Close()
result, err := ConfirmAction(...)If async writing is truly needed, track the goroutine with a WaitGroup and wait in a cleanup.
@copilot please address this.
| // text-based confirmation prompt when stderr is not a terminal (as is the | ||
| // case in `go test` runs), reading the response from os.Stdin. | ||
| func TestConfirmAction_NonTTY(t *testing.T) { | ||
| tests := []struct { |
There was a problem hiding this comment.
[/tdd] TestConfirmAction_NonTTY is missing an empty/EOF case analogous to the one added to TestShowTextConfirm. Since ConfirmAction delegates to showTextConfirm, the same EOF path is reachable here.
💡 Add an EOF test case
{name: "empty input EOF", input: "", wantErr: true},This rounds out the table and ensures the EOF error surfaces through the ConfirmAction wrapper too.
@copilot please address this.
There was a problem hiding this comment.
Review: Improve test quality for pkg/console/confirm_test.go
Overall this is a meaningful improvement — the old test only verified the function existed; the new tests exercise real behavior under non-TTY conditions.
Positive changes:
TestConfirmAction_NonTTYnow exercises the actualConfirmActionfallback path with stdin piping.- New
TestShowTextConfirmcases add coverage for uppercase Y/N and whitespace-padded input, which are real code paths inshowTextConfirm. - The EOF/empty-input case is correctly verified.
One minor issue flagged inline: the write-end of the pipe w in TestConfirmAction_NonTTY is only closed inside the goroutine; a t.Cleanup(func() { w.Close() }) would make teardown deterministic.
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.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22.6 AIC · ⌖ 7.23 AIC · ⊞ 5.5K
| _, _ = w.WriteString(tt.input) | ||
| w.Close() | ||
| }() | ||
|
|
There was a problem hiding this comment.
Minor: The write-end of the pipe w is only closed inside the goroutine. If the goroutine panics or is never scheduled before the test times out, the read end will block forever. Add a cleanup to ensure w is always closed deterministically:
t.Cleanup(func() { w.Close() })@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
Analysis
Build tag: Verdict
|
|
@copilot Quick triage nudge for this PR. Please address the remaining review feedback below, refresh the branch if GitHub can update it cleanly, run the Open review context (newest first):
Failed checks:
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31273170388
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
TestConfirmActionwas a placeholder (_ = ConfirmAction) that never exercisedConfirmAction, andTestShowTextConfirmwas missing several edge cases despite otherwise being a solid table-driven test.Changes
TestConfirmAction_NonTTYredirectsos.Stdinviaos.Pipe()and callsConfirmActiondirectly, exercising its non-TTY fallback path (whichshowTextConfirmimplements) with yes/no/invalid-input cases.TestShowTextConfirmtable: added empty-input/EOF, whitespace-padded input, and single-letter uppercase (Y/N) cases._ = ConfirmActionsubtest.