From c75bd05d42eb1a22c0ae7fb2b08aeef6dd3faf4d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 15 Apr 2026 06:25:00 +0000 Subject: [PATCH 1/2] refactor: eliminate runGHWithSpinner duplication and simplify setupGHCommand logging - Remove the duplicated runGHWithSpinner function; RunGH and RunGHCombined now delegate to RunGHContext/RunGHCombinedContext with context.Background(), which is functionally equivalent since context.Background() never cancels - Consolidate the token logging in setupGHCommand: hoist the log.Printf calls outside the ctx branch since the token-check logic is identical in both branches; the 'with context' wording was not useful for debugging Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/workflow/github_cli.go | 50 ++++++--------------------------- pkg/workflow/github_cli_test.go | 38 ++++++------------------- 2 files changed, 17 insertions(+), 71 deletions(-) diff --git a/pkg/workflow/github_cli.go b/pkg/workflow/github_cli.go index 787d33abfca..ca887593f19 100644 --- a/pkg/workflow/github_cli.go +++ b/pkg/workflow/github_cli.go @@ -28,18 +28,14 @@ func setupGHCommand(ctx context.Context, args ...string) *exec.Cmd { var cmd *exec.Cmd if ctx != nil { cmd = exec.CommandContext(ctx, "gh", args...) - if ghToken != "" || githubToken != "" { - githubCLILog.Printf("Using gh CLI via go-gh/v2 for command with context: gh %v", args) - } else { - githubCLILog.Printf("No token available, using default gh CLI with context for command: gh %v", args) - } } else { cmd = exec.Command("gh", args...) - if ghToken != "" || githubToken != "" { - githubCLILog.Printf("Using gh CLI via go-gh/v2 for command: gh %v", args) - } else { - githubCLILog.Printf("No token available, using default gh CLI for command: gh %v", args) - } + } + + if ghToken != "" || githubToken != "" { + githubCLILog.Printf("Using gh CLI for command: gh %v", args) + } else { + githubCLILog.Printf("No token available, using default gh CLI for command: gh %v", args) } // Set up environment to ensure token is available @@ -97,7 +93,7 @@ func enrichGHError(err error) error { } // runGHWithSpinnerContext executes a gh CLI command with context support, a spinner, -// and returns the output. This is the core implementation for RunGHContext. +// and returns the output. This is the core implementation for all RunGH* functions. func runGHWithSpinnerContext(ctx context.Context, spinnerMessage string, combined bool, args ...string) ([]byte, error) { cmd := ExecGHContext(ctx, args...) @@ -124,34 +120,6 @@ func runGHWithSpinnerContext(ctx context.Context, spinnerMessage string, combine return output, enrichGHError(err) } -// runGHWithSpinner executes a gh CLI command with a spinner and returns the output. -// This is the core implementation shared by RunGH and RunGHCombined. -func runGHWithSpinner(spinnerMessage string, combined bool, args ...string) ([]byte, error) { - cmd := ExecGH(args...) - - // Show spinner in interactive terminals - if tty.IsStderrTerminal() { - spinner := console.NewSpinner(spinnerMessage) - spinner.Start() - var output []byte - var err error - if combined { - output, err = cmd.CombinedOutput() - } else { - output, err = cmd.Output() - err = enrichGHError(err) - } - spinner.Stop() - return output, err - } - - if combined { - return cmd.CombinedOutput() - } - output, err := cmd.Output() - return output, enrichGHError(err) -} - // RunGH executes a gh CLI command with a spinner and returns the stdout output. // The spinner is shown in interactive terminals to provide feedback during network operations. // The spinnerMessage parameter describes what operation is being performed. @@ -160,7 +128,7 @@ func runGHWithSpinner(spinnerMessage string, combined bool, args ...string) ([]b // // output, err := RunGH("Fetching user info...", "api", "/user") func RunGH(spinnerMessage string, args ...string) ([]byte, error) { - return runGHWithSpinner(spinnerMessage, false, args...) + return RunGHContext(context.Background(), spinnerMessage, args...) } // RunGHContext executes a gh CLI command with context support (for cancellation/timeout), a @@ -182,7 +150,7 @@ func RunGHContext(ctx context.Context, spinnerMessage string, args ...string) ([ // // output, err := RunGHCombined("Creating repository...", "repo", "create", "myrepo") func RunGHCombined(spinnerMessage string, args ...string) ([]byte, error) { - return runGHWithSpinner(spinnerMessage, true, args...) + return RunGHCombinedContext(context.Background(), spinnerMessage, args...) } // RunGHCombinedContext executes a gh CLI command with context support (for cancellation/timeout), diff --git a/pkg/workflow/github_cli_test.go b/pkg/workflow/github_cli_test.go index 89574c3637a..08ca54289c6 100644 --- a/pkg/workflow/github_cli_test.go +++ b/pkg/workflow/github_cli_test.go @@ -341,10 +341,8 @@ func TestSetupGHCommand(t *testing.T) { } } -// TestRunGHWithSpinner tests the core runGHWithSpinner function -// Note: This test validates the function exists and handles arguments correctly -// Actual spinner behavior is tested via RunGH and RunGHCombined -func TestRunGHWithSpinnerHelperExists(t *testing.T) { +// TestRunGHFunctions tests that RunGH and RunGHCombined delegate correctly to their context variants. +func TestRunGHFunctions(t *testing.T) { // Save original environment originalGHToken := os.Getenv("GH_TOKEN") originalGitHubToken := os.Getenv("GITHUB_TOKEN") @@ -357,33 +355,13 @@ func TestRunGHWithSpinnerHelperExists(t *testing.T) { os.Unsetenv("GH_TOKEN") os.Unsetenv("GITHUB_TOKEN") - // Test that the function exists and can be called - // We use a command that will fail quickly without credentials - // to verify the integration works - tests := []struct { - name string - combined bool - }{ - { - name: "Test stdout mode", - combined: false, - }, - { - name: "Test combined mode", - combined: true, - }, - } + // Verify both public functions can be called; they delegate to RunGHContext/RunGHCombinedContext. + // We expect an error since gh requires auth, but no panic. + _, err := RunGH("Test spinner...", "auth", "status") + _ = err - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Just verify the function can be called - // We expect it to fail since gh command requires auth - _, err := runGHWithSpinner("Test spinner...", tt.combined, "auth", "status") - // We don't care about the error - we just want to verify the function exists - // and doesn't panic when called - _ = err - }) - } + _, err = RunGHCombined("Test spinner...", "auth", "status") + _ = err } // TestEnrichGHError tests that enrichGHError appends stderr from *exec.ExitError From 4fbdd56706b836d23b9d7e3135c24c7784107df2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 13:06:05 +0000 Subject: [PATCH 2/2] refactor: improve test assertions and log message clarity per review feedback Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4452668a-e5cf-4e0f-b590-7645574d92d5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/github_cli.go | 2 +- pkg/workflow/github_cli_test.go | 32 +++++++++++++++++++++++++------- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/pkg/workflow/github_cli.go b/pkg/workflow/github_cli.go index ca887593f19..91b78caed23 100644 --- a/pkg/workflow/github_cli.go +++ b/pkg/workflow/github_cli.go @@ -33,7 +33,7 @@ func setupGHCommand(ctx context.Context, args ...string) *exec.Cmd { } if ghToken != "" || githubToken != "" { - githubCLILog.Printf("Using gh CLI for command: gh %v", args) + githubCLILog.Printf("Token detected, using gh CLI for command: gh %v", args) } else { githubCLILog.Printf("No token available, using default gh CLI for command: gh %v", args) } diff --git a/pkg/workflow/github_cli_test.go b/pkg/workflow/github_cli_test.go index 08ca54289c6..1e3b15b7816 100644 --- a/pkg/workflow/github_cli_test.go +++ b/pkg/workflow/github_cli_test.go @@ -351,17 +351,35 @@ func TestRunGHFunctions(t *testing.T) { os.Setenv("GITHUB_TOKEN", originalGitHubToken) }() - // Set up test environment - no tokens so command won't actually execute + // Set up test environment with no tokens to keep behavior deterministic for unit tests. os.Unsetenv("GH_TOKEN") os.Unsetenv("GITHUB_TOKEN") - // Verify both public functions can be called; they delegate to RunGHContext/RunGHCombinedContext. - // We expect an error since gh requires auth, but no panic. - _, err := RunGH("Test spinner...", "auth", "status") - _ = err + t.Run("RunGH matches RunGHContext", func(t *testing.T) { + gotOut, gotErr := RunGH("Test spinner...", "auth", "status") + wantOut, wantErr := RunGHContext(context.Background(), "Test spinner...", "auth", "status") + + assert.Equal(t, wantOut, gotOut, "RunGH should return the same output as RunGHContext") + if wantErr == nil { + assert.NoError(t, gotErr, "RunGH should match RunGHContext error behavior") + } else { + require.Error(t, gotErr, "RunGH should match RunGHContext error behavior") + assert.Equal(t, wantErr.Error(), gotErr.Error(), "RunGH should return the same error text as RunGHContext") + } + }) - _, err = RunGHCombined("Test spinner...", "auth", "status") - _ = err + t.Run("RunGHCombined matches RunGHCombinedContext", func(t *testing.T) { + gotOut, gotErr := RunGHCombined("Test spinner...", "auth", "status") + wantOut, wantErr := RunGHCombinedContext(context.Background(), "Test spinner...", "auth", "status") + + assert.Equal(t, wantOut, gotOut, "RunGHCombined should return the same output as RunGHCombinedContext") + if wantErr == nil { + assert.NoError(t, gotErr, "RunGHCombined should match RunGHCombinedContext error behavior") + } else { + require.Error(t, gotErr, "RunGHCombined should match RunGHCombinedContext error behavior") + assert.Equal(t, wantErr.Error(), gotErr.Error(), "RunGHCombined should return the same error text as RunGHCombinedContext") + } + }) } // TestEnrichGHError tests that enrichGHError appends stderr from *exec.ExitError