Skip to content

Improve test quality for pkg/console/confirm_test.go #51434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Aug 8, 2026
+43 −8
Merged

Improve test quality for pkg/console/confirm_test.go #51434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13b11bd
Initial plan
Copilot Aug 8, 2026
59e2c2d
Improve test quality for pkg/console/confirm_test.go
Copilot Aug 8, 2026
e97f2be
Plan PR feedback follow-up
Copilot Aug 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/skills/agentic-workflows/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Repository overlay (optional):
Read only the files you need:
Load these files from `github/gh-aw` (they are not available locally).
- `.github/aw/action-container-substitutions.md`
- `.github/aw/agent-runtime-instructions.md`
- `.github/aw/agentic-chat.md`
- `.github/aw/agentic-workflows-mcp.md`
- `.github/aw/asciicharts.md`
Expand Down
50 changes: 42 additions & 8 deletions pkg/console/confirm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,52 @@
package console

import (
"os"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestConfirmAction(t *testing.T) {
// Note: This test can't fully test the interactive behavior without mocking
// the terminal input, but we can verify the function signature and basic setup
// TestConfirmAction_NonTTY verifies that ConfirmAction falls back to the
// 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 {

@github-actions github-actions Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

name string
input string
wantResult bool
wantErr bool
}{
{name: "yes", input: "y\n", wantResult: true},
{name: "no", input: "n\n", wantResult: false},
{name: "invalid", input: "maybe\n", wantErr: true},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
oldStdin := os.Stdin
r, w, err := os.Pipe()
require.NoError(t, err)
t.Cleanup(func() { os.Stdin = oldStdin })
t.Cleanup(func() { r.Close() })

@github-actions github-actions Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 1st

Currently harmless (ConfirmAction has returned by cleanup time), but the ordering will silently cause problems if cleanup logic ever changes.

@copilot please address this.

os.Stdin = r
Comment on lines +31 to +36

go func() {
_, _ = w.WriteString(tt.input)
w.Close()
}()

@github-actions github-actions Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t.Run("function signature", func(t *testing.T) {
// This test just verifies the function exists and has the right signature
// Actual interactive testing would require a mock terminal
_ = ConfirmAction
})
result, err := ConfirmAction("Delete all workflows?", "Yes, delete", "Cancel")
if tt.wantErr {
require.Error(t, err)
} else {

@github-actions github-actions Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

require.NoError(t, err)
assert.Equal(t, tt.wantResult, result)
}
})
}
}

func TestShowTextConfirm(t *testing.T) {
Expand All @@ -37,7 +67,11 @@ func TestShowTextConfirm(t *testing.T) {
{name: "NO uppercase", input: "NO\n", wantResult: false},
{name: "no full", input: "no\n", wantResult: false},
{name: "2 for negative", input: "2\n", wantResult: false},
{name: "single letter uppercase Y", input: "Y\n", wantResult: true},
{name: "single letter uppercase N", input: "N\n", wantResult: false},
{name: "whitespace padded yes", input: " y \n", wantResult: true},
{name: "invalid input", input: "maybe\n", wantErr: true, errContains: "invalid input"},
{name: "empty input EOF", input: "", wantErr: true, errContains: "invalid input"},
}

for _, tt := range tests {
Expand Down