From cbaaf77822bd764dbce526acce4a0b51d772fcb9 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Mon, 9 Dec 2024 08:35:01 -0300 Subject: [PATCH 01/21] fixing gh gist view prompts with no TTY --- pkg/cmd/gist/view/view.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index 2a19f5958..a6f4d528a 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -55,7 +55,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman } if !opts.IO.IsStdoutTTY() { - opts.Raw = true + return cmdutil.FlagErrorf("run or gist ID required when not running interactively\n\nUsage: gh gist view [ | ] [flags]\n\nFlags:\n -f, --filename string Display a single file from the gist\n --files List file names from the gist\n -r, --raw Print raw instead of rendered gist contents\n -w, --web Open gist in the browser\n") } if runF != nil { From b740486b13046ad1560aa1c461947ef27c145eb2 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Mon, 9 Dec 2024 10:59:09 -0300 Subject: [PATCH 02/21] #10042 fixed test --- pkg/cmd/gist/view/view_test.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index 52e616f7c..2e80ee082 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -20,10 +20,11 @@ import ( func TestNewCmdView(t *testing.T) { tests := []struct { - name string - cli string - wants ViewOptions - tty bool + name string + cli string + wants ViewOptions + tty bool + wantsErr bool }{ { name: "tty no arguments", @@ -37,12 +38,14 @@ func TestNewCmdView(t *testing.T) { }, { name: "nontty no arguments", + tty: false, cli: "123", wants: ViewOptions{ Raw: true, Selector: "123", ListFiles: false, }, + wantsErr: true, }, { name: "filename passed", @@ -94,6 +97,12 @@ func TestNewCmdView(t *testing.T) { gotOpts = opts return nil }) + + _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } cmd.SetArgs(argv) cmd.SetIn(&bytes.Buffer{}) cmd.SetOut(&bytes.Buffer{}) From f4f8840c3f53587f398240571c5affebc149d8c1 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Thu, 9 Jan 2025 12:07:15 -0300 Subject: [PATCH 03/21] #10042: Add error messages for 'gh gist view/edit' prompts when no TTY is detected --- pkg/cmd/gist/edit/edit.go | 3 +++ pkg/cmd/gist/view/view.go | 6 +++++- pkg/cmd/gist/view/view_test.go | 17 +++++------------ 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index d777e9581..ced995f63 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -108,6 +108,9 @@ func editRun(opts *EditOptions) error { if gistID == "" { cs := opts.IO.ColorScheme() if gistID == "" { + if !opts.IO.CanPrompt() { + return cmdutil.FlagErrorf("gist ID or URL required when not running interactively") + } gistID, err = shared.PromptGists(opts.Prompter, client, host, cs) if err != nil { return err diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index a6f4d528a..25feccdc1 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -55,7 +55,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman } if !opts.IO.IsStdoutTTY() { - return cmdutil.FlagErrorf("run or gist ID required when not running interactively\n\nUsage: gh gist view [ | ] [flags]\n\nFlags:\n -f, --filename string Display a single file from the gist\n --files List file names from the gist\n -r, --raw Print raw instead of rendered gist contents\n -w, --web Open gist in the browser\n") + opts.Raw = true } if runF != nil { @@ -89,6 +89,10 @@ func viewRun(opts *ViewOptions) error { cs := opts.IO.ColorScheme() if gistID == "" { + if !opts.IO.CanPrompt() { + return cmdutil.FlagErrorf("gist ID or URL required when not running interactively") + } + gistID, err = shared.PromptGists(opts.Prompter, client, hostname, cs) if err != nil { return err diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index 2e80ee082..e52a3b9cb 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -20,11 +20,10 @@ import ( func TestNewCmdView(t *testing.T) { tests := []struct { - name string - cli string - wants ViewOptions - tty bool - wantsErr bool + name string + cli string + wants ViewOptions + tty bool }{ { name: "tty no arguments", @@ -45,7 +44,6 @@ func TestNewCmdView(t *testing.T) { Selector: "123", ListFiles: false, }, - wantsErr: true, }, { name: "filename passed", @@ -98,11 +96,6 @@ func TestNewCmdView(t *testing.T) { return nil }) - _, err = cmd.ExecuteC() - if tt.wantsErr { - assert.Error(t, err) - return - } cmd.SetArgs(argv) cmd.SetIn(&bytes.Buffer{}) cmd.SetOut(&bytes.Buffer{}) @@ -166,7 +159,7 @@ func Test_viewRun(t *testing.T) { }, }, }, - wantOut: "test interactive mode\n", + wantErr: true, }, { name: "filename selected", From 4c2ba5681d4777dc364bad989eda6a68189ca6a5 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Thu, 9 Jan 2025 13:14:27 -0300 Subject: [PATCH 04/21] #10042 removed unnecessary field --- pkg/cmd/gist/view/view_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index e52a3b9cb..37dda42d1 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -37,7 +37,6 @@ func TestNewCmdView(t *testing.T) { }, { name: "nontty no arguments", - tty: false, cli: "123", wants: ViewOptions{ Raw: true, From c5d6ae6cf698553e47b1fcd92385d7975296f35f Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Sun, 12 Jan 2025 14:53:20 -0300 Subject: [PATCH 05/21] Update pkg/cmd/gist/edit/edit.go Update pkg/cmd/gist/edit/edit.go after code review Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- pkg/cmd/gist/edit/edit.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index ced995f63..5f2815682 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -111,6 +111,7 @@ func editRun(opts *EditOptions) error { if !opts.IO.CanPrompt() { return cmdutil.FlagErrorf("gist ID or URL required when not running interactively") } + gistID, err = shared.PromptGists(opts.Prompter, client, host, cs) if err != nil { return err From 24e9fed7e0720a47870a495bd54e45cca2b4a5a5 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Sun, 12 Jan 2025 14:57:24 -0300 Subject: [PATCH 06/21] removed unnecessary space --- pkg/cmd/gist/edit/edit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 5f2815682..b201cdc14 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -111,7 +111,7 @@ func editRun(opts *EditOptions) error { if !opts.IO.CanPrompt() { return cmdutil.FlagErrorf("gist ID or URL required when not running interactively") } - + gistID, err = shared.PromptGists(opts.Prompter, client, host, cs) if err != nil { return err From 977e2326a25d48c7fb4250b2927760ca28f18a79 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Mon, 13 Jan 2025 11:06:13 -0300 Subject: [PATCH 07/21] #10042: Applied code review suggestions to view_test.go --- pkg/cmd/gist/edit/edit_test.go | 7 ++++ pkg/cmd/gist/view/view_test.go | 58 +++++++++++++++++++++++----------- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index 11a32bc92..0bf7cb51a 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -519,6 +519,11 @@ func Test_editRun(t *testing.T) { }, }, }, + { + name: "no arguments notty", + nontty: true, + wantErr: "gist ID or URL required when not running interactively", + }, } for _, tt := range tests { @@ -552,6 +557,8 @@ func Test_editRun(t *testing.T) { stdin.WriteString(tt.stdin) ios.SetStdoutTTY(!tt.nontty) ios.SetStdinTTY(!tt.nontty) + ios.SetStderrTTY(!tt.nontty) + tt.opts.IO = ios tt.opts.Selector = "1234" diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index 37dda42d1..820401f1e 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -16,6 +16,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdView(t *testing.T) { @@ -116,19 +117,22 @@ func Test_viewRun(t *testing.T) { opts *ViewOptions wantOut string gist *shared.Gist - wantErr bool + isTTY bool + wantErr string mockGistList bool }{ { - name: "no such gist", + name: "no such gist", + isTTY: false, opts: &ViewOptions{ Selector: "1234", ListFiles: false, }, - wantErr: true, + wantErr: "not found", }, { - name: "one file", + name: "one file", + isTTY: true, opts: &ViewOptions{ Selector: "1234", ListFiles: false, @@ -144,7 +148,8 @@ func Test_viewRun(t *testing.T) { wantOut: "bwhiizzzbwhuiiizzzz\n", }, { - name: "one file, no ID supplied", + name: "one file, no ID supplied", + isTTY: true, opts: &ViewOptions{ Selector: "", ListFiles: false, @@ -158,10 +163,16 @@ func Test_viewRun(t *testing.T) { }, }, }, - wantErr: true, + wantOut: "test interactive mode\n", }, { - name: "filename selected", + name: "no arguments notty", + isTTY: false, + wantErr: "gist ID or URL required when not running interactively", + }, + { + name: "filename selected", + isTTY: true, opts: &ViewOptions{ Selector: "1234", Filename: "cicada.txt", @@ -182,7 +193,8 @@ func Test_viewRun(t *testing.T) { wantOut: "bwhiizzzbwhuiiizzzz\n", }, { - name: "filename selected, raw", + name: "filename selected, raw", + isTTY: true, opts: &ViewOptions{ Selector: "1234", Filename: "cicada.txt", @@ -204,7 +216,8 @@ func Test_viewRun(t *testing.T) { wantOut: "bwhiizzzbwhuiiizzzz\n", }, { - name: "multiple files, no description", + name: "multiple files, no description", + isTTY: true, opts: &ViewOptions{ Selector: "1234", ListFiles: false, @@ -224,7 +237,8 @@ func Test_viewRun(t *testing.T) { wantOut: "cicada.txt\n\nbwhiizzzbwhuiiizzzz\n\nfoo.md\n\n\n # foo \n\n", }, { - name: "multiple files, trailing newlines", + name: "multiple files, trailing newlines", + isTTY: true, opts: &ViewOptions{ Selector: "1234", ListFiles: false, @@ -244,7 +258,8 @@ func Test_viewRun(t *testing.T) { wantOut: "cicada.txt\n\nbwhiizzzbwhuiiizzzz\n\nfoo.txt\n\nbar\n", }, { - name: "multiple files, description", + name: "multiple files, description", + isTTY: true, opts: &ViewOptions{ Selector: "1234", ListFiles: false, @@ -265,7 +280,8 @@ func Test_viewRun(t *testing.T) { wantOut: "some files\n\ncicada.txt\n\nbwhiizzzbwhuiiizzzz\n\nfoo.md\n\n\n \n • foo \n\n", }, { - name: "multiple files, raw", + name: "multiple files, raw", + isTTY: true, opts: &ViewOptions{ Selector: "1234", Raw: true, @@ -287,7 +303,8 @@ func Test_viewRun(t *testing.T) { wantOut: "some files\n\ncicada.txt\n\nbwhiizzzbwhuiiizzzz\n\nfoo.md\n\n- foo\n", }, { - name: "one file, list files", + name: "one file, list files", + isTTY: true, opts: &ViewOptions{ Selector: "1234", Raw: false, @@ -305,7 +322,8 @@ func Test_viewRun(t *testing.T) { wantOut: "cicada.txt\n", }, { - name: "multiple file, list files", + name: "multiple file, list files", + isTTY: true, opts: &ViewOptions{ Selector: "1234", Raw: false, @@ -377,16 +395,20 @@ func Test_viewRun(t *testing.T) { } ios, _, stdout, _ := iostreams.Test() - ios.SetStdoutTTY(true) + ios.SetStdoutTTY(tt.isTTY) + ios.SetStdinTTY(tt.isTTY) + ios.SetStderrTTY(tt.isTTY) + tt.opts.IO = ios t.Run(tt.name, func(t *testing.T) { err := viewRun(tt.opts) - if tt.wantErr { - assert.Error(t, err) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) return + } else { + require.NoError(t, err) } - assert.NoError(t, err) assert.Equal(t, tt.wantOut, stdout.String()) reg.Verify(t) From c0f6eb0598bad1463f8dca4b574c69342d653aa0 Mon Sep 17 00:00:00 2001 From: Mateus Marquezini Date: Mon, 13 Jan 2025 11:39:48 -0300 Subject: [PATCH 08/21] #10042: Attempt to add a new test scenario for handling error messages when TTY is unavailable --- pkg/cmd/gist/edit/edit_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index 0bf7cb51a..be610b6b4 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -520,8 +520,11 @@ func Test_editRun(t *testing.T) { }, }, { - name: "no arguments notty", - nontty: true, + name: "no arguments notty", + nontty: true, + opts: &EditOptions{ + Selector: "", + }, wantErr: "gist ID or URL required when not running interactively", }, } From 7300f0d56888dd74605f8921aba8e796612ffcd6 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:23:32 +0000 Subject: [PATCH 09/21] Bump github.com/gabriel-vasile/mimetype from 1.4.7 to 1.4.8 Bumps [github.com/gabriel-vasile/mimetype](https://github.com/gabriel-vasile/mimetype) from 1.4.7 to 1.4.8. - [Release notes](https://github.com/gabriel-vasile/mimetype/releases) - [Commits](https://github.com/gabriel-vasile/mimetype/compare/v1.4.7...v1.4.8) --- updated-dependencies: - dependency-name: github.com/gabriel-vasile/mimetype dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5661ac091..00d18bbd2 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/creack/pty v1.1.24 github.com/digitorus/timestamp v0.0.0-20231217203849-220c5c2851b7 github.com/distribution/reference v0.5.0 - github.com/gabriel-vasile/mimetype v1.4.7 + github.com/gabriel-vasile/mimetype v1.4.8 github.com/gdamore/tcell/v2 v2.5.4 github.com/golang/snappy v0.0.4 github.com/google/go-cmp v0.6.0 diff --git a/go.sum b/go.sum index d17d1251d..16bcb6ed7 100644 --- a/go.sum +++ b/go.sum @@ -149,8 +149,8 @@ github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHk github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= -github.com/gabriel-vasile/mimetype v1.4.7 h1:SKFKl7kD0RiPdbht0s7hFtjl489WcQ1VyPW8ZzUMYCA= -github.com/gabriel-vasile/mimetype v1.4.7/go.mod h1:GDlAgAyIRT27BhFl53XNAFtfjzOkLaF35JdEG0P7LtU= +github.com/gabriel-vasile/mimetype v1.4.8 h1:FfZ3gj38NjllZIeJAmMhr+qKL8Wu+nOoI3GqacKw1NM= +github.com/gabriel-vasile/mimetype v1.4.8/go.mod h1:ByKUIKGjh1ODkGM1asKUbQZOLGrPjydw3hYPU2YU9t8= github.com/gdamore/encoding v1.0.0 h1:+7OoQ1Bc6eTm5niUzBa0Ctsh6JbMW6Ra+YNuAtDBdko= github.com/gdamore/encoding v1.0.0/go.mod h1:alR0ol34c49FCSBLjhosxzcPHQbf2trDkoo5dl+VrEg= github.com/gdamore/tcell/v2 v2.5.4 h1:TGU4tSjD3sCL788vFNeJnTdzpNKIw1H5dgLnJRQVv/k= From bc8c46b0b1c1fd411e286990b950aad254cfb78f Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 13 Jan 2025 20:38:03 -0500 Subject: [PATCH 10/21] Make extension update check non-blocking Fixes #10235 This commit updates the Cobra command logic around extension upgrade checks to be non-blocking. Previously, we were waiting for 1 second after the extension completed to allow the update checking logic complete, however users want the GitHub CLI to run as far as possible. --- pkg/cmd/root/extension.go | 31 ++++++++++++--------- pkg/cmd/root/extension_test.go | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index 7f2325e18..7e2d7aca7 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -52,20 +52,25 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex }, // PostRun handles communicating extension release information if found PostRun: func(c *cobra.Command, args []string) { - releaseInfo := <-updateMessageChan - if releaseInfo != nil { - stderr := io.ErrOut - fmt.Fprintf(stderr, "\n\n%s %s → %s\n", - cs.Yellowf("A new release of %s is available:", ext.Name()), - cs.Cyan(strings.TrimPrefix(ext.CurrentVersion(), "v")), - cs.Cyan(strings.TrimPrefix(releaseInfo.Version, "v"))) - if ext.IsPinned() { - fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name()) - } else { - fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name()) + select { + case releaseInfo := <-updateMessageChan: + if releaseInfo != nil { + stderr := io.ErrOut + fmt.Fprintf(stderr, "\n\n%s %s → %s\n", + cs.Yellowf("A new release of %s is available:", ext.Name()), + cs.Cyan(strings.TrimPrefix(ext.CurrentVersion(), "v")), + cs.Cyan(strings.TrimPrefix(releaseInfo.Version, "v"))) + if ext.IsPinned() { + fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name()) + } else { + fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name()) + } + fmt.Fprintf(stderr, "%s\n\n", + cs.Yellow(releaseInfo.URL)) } - fmt.Fprintf(stderr, "%s\n\n", - cs.Yellow(releaseInfo.URL)) + default: + // Do not make the user wait for extension update check if incomplete by this time. + // This is being handled in non-blocking default as there is no context to cancel like in gh update checks. } }, GroupID: "extension", diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go index 32b250de5..7dd66df31 100644 --- a/pkg/cmd/root/extension_test.go +++ b/pkg/cmd/root/extension_test.go @@ -3,6 +3,7 @@ package root_test import ( "io" "testing" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/update" @@ -121,6 +122,8 @@ func TestNewCmdExtension_Updates(t *testing.T) { em := &extensions.ExtensionManagerMock{ DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // Assume extension executed / dispatched without problems as test is focused on upgrade checking. + // Sleep for 1 second to allow update checking logic to complete. + time.Sleep(1 * time.Second) return true, nil }, } @@ -169,3 +172,51 @@ func TestNewCmdExtension_Updates(t *testing.T) { } } } + +func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) { + ios, _, _, stderr := iostreams.Test() + + em := &extensions.ExtensionManagerMock{ + DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { + // Assume extension executed / dispatched without problems as test is focused on upgrade checking. + return true, nil + }, + } + + ext := &extensions.ExtensionMock{ + CurrentVersionFunc: func() string { + return "1.0.0" + }, + IsPinnedFunc: func() bool { + return false + }, + LatestVersionFunc: func() string { + return "2.0.0" + }, + NameFunc: func() string { + return "major-update" + }, + UpdateAvailableFunc: func() bool { + return true + }, + URLFunc: func() string { + return "https//github.com/dne/major-update" + }, + } + + checkFunc := func(em extensions.ExtensionManager, ext extensions.Extension) (*update.ReleaseInfo, error) { + // This function runs in the background as a goroutine while the extension is dispatched. + // Testing whether the extension cobra command is non-blocking for extension update check, + // this function will cause goroutine to sleep for a sufficiently long time before panicking. + // The idea is that the test will conclude because the dispatch function completes immediately + // and exits before the check update function completes. + time.Sleep(30 * time.Second) + panic("It seems like the extension cobra command might be blocking on update checking when it should not block on long update checks") + } + + cmd := root.NewCmdExtension(ios, em, ext, checkFunc) + + _, err := cmd.ExecuteC() + require.NoError(t, err) + require.Emptyf(t, stderr.String(), "long running, non-blocking extension update check should not have displayed anything on stderr") +} From 2757d22b4b236ad0d24104a5027d6ec87cc1e657 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 15 Jan 2025 14:49:31 +0000 Subject: [PATCH 11/21] Bump google.golang.org/protobuf from 1.36.2 to 1.36.3 Bumps google.golang.org/protobuf from 1.36.2 to 1.36.3. --- updated-dependencies: - dependency-name: google.golang.org/protobuf dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 0921a544f..910ab484d 100644 --- a/go.mod +++ b/go.mod @@ -51,7 +51,7 @@ require ( golang.org/x/term v0.27.0 golang.org/x/text v0.21.0 google.golang.org/grpc v1.64.1 - google.golang.org/protobuf v1.36.2 + google.golang.org/protobuf v1.36.3 gopkg.in/h2non/gock.v1 v1.1.2 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index 73158e4e0..4314215db 100644 --- a/go.sum +++ b/go.sum @@ -548,8 +548,8 @@ google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 h1: google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5/go.mod h1:EfXuqaE1J41VCDicxHzUDm+8rk+7ZdXzHV0IhO/I6s0= google.golang.org/grpc v1.64.1 h1:LKtvyfbX3UGVPFcGqJ9ItpVWW6oN/2XqTxfAnwRRXiA= google.golang.org/grpc v1.64.1/go.mod h1:hiQF4LFZelK2WKaP6W0L92zGHtiQdZxk8CrSdvyjeP0= -google.golang.org/protobuf v1.36.2 h1:R8FeyR1/eLmkutZOM5CWghmo5itiG9z0ktFlTVLuTmU= -google.golang.org/protobuf v1.36.2/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE= +google.golang.org/protobuf v1.36.3 h1:82DV7MYdb8anAVi3qge1wSnMDrnKK7ebr+I0hHRN1BU= +google.golang.org/protobuf v1.36.3/go.mod h1:9fA7Ob0pmnwhb644+1+CVWFRbNajQ6iRojtC/QF5bRE= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= From 243acaf5792d62234f769c2a9125f81616cd6328 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Thu, 16 Jan 2025 00:08:45 -0500 Subject: [PATCH 12/21] Refactor test based on PR feedback --- pkg/cmd/root/extension_test.go | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go index 7dd66df31..8246c0e2a 100644 --- a/pkg/cmd/root/extension_test.go +++ b/pkg/cmd/root/extension_test.go @@ -1,6 +1,7 @@ package root_test import ( + "fmt" "io" "testing" "time" @@ -122,8 +123,6 @@ func TestNewCmdExtension_Updates(t *testing.T) { em := &extensions.ExtensionManagerMock{ DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // Assume extension executed / dispatched without problems as test is focused on upgrade checking. - // Sleep for 1 second to allow update checking logic to complete. - time.Sleep(1 * time.Second) return true, nil }, } @@ -174,7 +173,7 @@ func TestNewCmdExtension_Updates(t *testing.T) { } func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) { - ios, _, _, stderr := iostreams.Test() + ios, _, _, _ := iostreams.Test() em := &extensions.ExtensionManagerMock{ DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { @@ -204,19 +203,30 @@ func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) { }, } + // When the extension command is executed, the checkFunc will run in the background longer than the extension dispatch. + // If the update check is non-blocking, then the extension command will complete immediately while checkFunc is still running. checkFunc := func(em extensions.ExtensionManager, ext extensions.Extension) (*update.ReleaseInfo, error) { - // This function runs in the background as a goroutine while the extension is dispatched. - // Testing whether the extension cobra command is non-blocking for extension update check, - // this function will cause goroutine to sleep for a sufficiently long time before panicking. - // The idea is that the test will conclude because the dispatch function completes immediately - // and exits before the check update function completes. time.Sleep(30 * time.Second) - panic("It seems like the extension cobra command might be blocking on update checking when it should not block on long update checks") + return nil, fmt.Errorf("update check should not have completed") } cmd := root.NewCmdExtension(ios, em, ext, checkFunc) - _, err := cmd.ExecuteC() - require.NoError(t, err) - require.Emptyf(t, stderr.String(), "long running, non-blocking extension update check should not have displayed anything on stderr") + // The test whether update check is non-blocking is based on how long it takes for the extension command execution. + // If there is no wait time as checkFunc is sleeping sufficiently long, we can trust update check is non-blocking. + // Otherwise, if any amount of wait is encountered, it is a decent indicator that update checking is blocking. + // This is not an ideal test and indicates the update design should be revisited to be easier to understand and manage. + completed := make(chan struct{}) + go func() { + _, err := cmd.ExecuteC() + require.NoError(t, err) + close(completed) + }() + + select { + case <-completed: + // Expected behavior assuming extension dispatch exits immediately while checkFunc is still running. + case <-time.After(1 * time.Second): + t.Fatal("extension update check should have exited") + } } From cc4bf0fc9fc58da7f4eb522aad3fa41f0f0068f5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Jan 2025 14:10:42 +0100 Subject: [PATCH 13/21] Add small wait to extension update tests --- pkg/cmd/root/extension_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go index 8246c0e2a..5e9e9b9bc 100644 --- a/pkg/cmd/root/extension_test.go +++ b/pkg/cmd/root/extension_test.go @@ -123,6 +123,10 @@ func TestNewCmdExtension_Updates(t *testing.T) { em := &extensions.ExtensionManagerMock{ DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // Assume extension executed / dispatched without problems as test is focused on upgrade checking. + // Sleep for 100 milliseconds to allow update checking logic to complete. This would be better + // served by making the behaviour controllable by channels, but it's a larger change than desired + // just to improve the test. + time.Sleep(100 * time.Millisecond) return true, nil }, } From 35d81e3ff596071005ce07d39c0e8e5a4d09fb50 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Jan 2025 10:03:48 -0700 Subject: [PATCH 14/21] Fix: Gist edit tests for interactivity This changes the gist edit tests to use the positive `istty` instead of the previous inverse `nontty`, which is consistent with the way other commands are written. --- pkg/cmd/gist/edit/edit_test.go | 223 ++++++++++++++++++++++----------- 1 file changed, 148 insertions(+), 75 deletions(-) diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index be610b6b4..a40695c39 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -3,11 +3,13 @@ package edit import ( "bytes" "encoding/json" + "fmt" "io" "net/http" "os" "path/filepath" "testing" + "time" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" @@ -141,23 +143,31 @@ func Test_editRun(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - opts *EditOptions - gist *shared.Gist - httpStubs func(*httpmock.Registry) - prompterStubs func(*prompter.MockPrompter) - nontty bool - stdin string - wantErr string - wantParams map[string]interface{} + name string + opts *EditOptions + mockGist *shared.Gist + mockGistList bool + httpStubs func(*httpmock.Registry) + prompterStubs func(*prompter.MockPrompter) + istty bool + stdin string + wantErr string + wantLastRequestParameters map[string]interface{} }{ { name: "no such gist", wantErr: "gist not found: 1234", + opts: &EditOptions{ + Selector: "1234", + }, }, { - name: "one file", - gist: &shared.Gist{ + name: "one file", + istty: false, + opts: &EditOptions{ + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -172,7 +182,7 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "", "files": map[string]interface{}{ "cicada.txt": map[string]interface{}{ @@ -183,7 +193,9 @@ func Test_editRun(t *testing.T) { }, }, { - name: "multiple files, submit", + name: "multiple files, submit, with TTY", + istty: true, + mockGistList: true, prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Edit which file?", []string{"cicada.txt", "unix.md"}, @@ -196,7 +208,7 @@ func Test_editRun(t *testing.T) { return prompter.IndexFor(opts, "Submit") }) }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ ID: "1234", Description: "catbug", Files: map[string]*shared.GistFile{ @@ -215,7 +227,7 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "catbug", "files": map[string]interface{}{ "cicada.txt": map[string]interface{}{ @@ -230,7 +242,11 @@ func Test_editRun(t *testing.T) { }, }, { - name: "multiple files, cancel", + name: "multiple files, cancel", + istty: true, + opts: &EditOptions{ + Selector: "1234", + }, prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Edit which file?", []string{"cicada.txt", "unix.md"}, @@ -244,7 +260,7 @@ func Test_editRun(t *testing.T) { }) }, wantErr: "CancelError", - gist: &shared.Gist{ + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -263,7 +279,10 @@ func Test_editRun(t *testing.T) { }, { name: "not change", - gist: &shared.Gist{ + opts: &EditOptions{ + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -277,7 +296,10 @@ func Test_editRun(t *testing.T) { }, { name: "another user's gist", - gist: &shared.Gist{ + opts: &EditOptions{ + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -292,7 +314,11 @@ func Test_editRun(t *testing.T) { }, { name: "add file to existing gist", - gist: &shared.Gist{ + opts: &EditOptions{ + AddFilename: fileToAdd, + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -307,16 +333,14 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - opts: &EditOptions{ - AddFilename: fileToAdd, - }, }, { name: "change description", opts: &EditOptions{ Description: "my new description", + Selector: "1234", }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ ID: "1234", Description: "my old description", Files: map[string]*shared.GistFile{ @@ -331,7 +355,7 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "my new description", "files": map[string]interface{}{ "sample.txt": map[string]interface{}{ @@ -343,7 +367,12 @@ func Test_editRun(t *testing.T) { }, { name: "add file to existing gist from source parameter", - gist: &shared.Gist{ + opts: &EditOptions{ + AddFilename: "from_source.txt", + SourceFile: fileToAdd, + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -358,11 +387,7 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - opts: &EditOptions{ - AddFilename: "from_source.txt", - SourceFile: fileToAdd, - }, - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "", "files": map[string]interface{}{ "from_source.txt": map[string]interface{}{ @@ -374,7 +399,12 @@ func Test_editRun(t *testing.T) { }, { name: "add file to existing gist from stdin", - gist: &shared.Gist{ + opts: &EditOptions{ + AddFilename: "from_source.txt", + SourceFile: "-", + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -389,12 +419,8 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - opts: &EditOptions{ - AddFilename: "from_source.txt", - SourceFile: "-", - }, stdin: "data from stdin", - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "", "files": map[string]interface{}{ "from_source.txt": map[string]interface{}{ @@ -406,7 +432,11 @@ func Test_editRun(t *testing.T) { }, { name: "remove file, file does not exist", - gist: &shared.Gist{ + opts: &EditOptions{ + RemoveFilename: "sample2.txt", + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -417,14 +447,15 @@ func Test_editRun(t *testing.T) { }, Owner: &shared.GistOwner{Login: "octocat"}, }, - opts: &EditOptions{ - RemoveFilename: "sample2.txt", - }, wantErr: "gist has no file \"sample2.txt\"", }, { name: "remove file from existing gist", - gist: &shared.Gist{ + opts: &EditOptions{ + RemoveFilename: "sample2.txt", + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -444,10 +475,7 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - opts: &EditOptions{ - RemoveFilename: "sample2.txt", - }, - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "", "files": map[string]interface{}{ "sample.txt": map[string]interface{}{ @@ -460,7 +488,11 @@ func Test_editRun(t *testing.T) { }, { name: "edit gist using file from source parameter", - gist: &shared.Gist{ + opts: &EditOptions{ + SourceFile: fileToAdd, + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -475,10 +507,7 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - opts: &EditOptions{ - SourceFile: fileToAdd, - }, - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "", "files": map[string]interface{}{ "sample.txt": map[string]interface{}{ @@ -490,7 +519,11 @@ func Test_editRun(t *testing.T) { }, { name: "edit gist using stdin", - gist: &shared.Gist{ + opts: &EditOptions{ + SourceFile: "-", + Selector: "1234", + }, + mockGist: &shared.Gist{ ID: "1234", Files: map[string]*shared.GistFile{ "sample.txt": { @@ -505,11 +538,8 @@ func Test_editRun(t *testing.T) { reg.Register(httpmock.REST("POST", "gists/1234"), httpmock.StatusStringResponse(201, "{}")) }, - opts: &EditOptions{ - SourceFile: "-", - }, stdin: "data from stdin", - wantParams: map[string]interface{}{ + wantLastRequestParameters: map[string]interface{}{ "description": "", "files": map[string]interface{}{ "sample.txt": map[string]interface{}{ @@ -520,8 +550,8 @@ func Test_editRun(t *testing.T) { }, }, { - name: "no arguments notty", - nontty: true, + name: "no arguments notty", + istty: false, opts: &EditOptions{ Selector: "", }, @@ -531,24 +561,62 @@ func Test_editRun(t *testing.T) { for _, tt := range tests { reg := &httpmock.Registry{} - if tt.gist == nil { + pm := prompter.NewMockPrompter(t) + + if tt.opts == nil { + tt.opts = &EditOptions{} + } + + if tt.opts.Selector != "" { + // Only register the HTTP stubs for a direct gist lookup if a selector is provided. + if tt.mockGist == nil { + // If no gist is provided, we expect a 404. + reg.Register(httpmock.REST("GET", fmt.Sprintf("gists/%s", tt.opts.Selector)), + httpmock.StatusStringResponse(404, "Not Found")) + } else { + // If a gist is provided, we expect the gist to be fetched. + reg.Register(httpmock.REST("GET", fmt.Sprintf("gists/%s", tt.opts.Selector)), + httpmock.JSONResponse(tt.mockGist)) + reg.Register(httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"octocat"}}}`)) + } + } + + if tt.mockGistList { + sixHours, _ := time.ParseDuration("6h") + sixHoursAgo := time.Now().Add(-sixHours) + reg.Register(httpmock.GraphQL(`query GistList\b`), + httpmock.StringResponse( + fmt.Sprintf(`{ "data": { "viewer": { "gists": { "nodes": [ + { + "description": "whatever", + "files": [{ "name": "cicada.txt" }, { "name": "unix.md" }], + "isPublic": true, + "name": "1234", + "updatedAt": "%s" + } + ], + "pageInfo": { + "hasNextPage": false, + "endCursor": "somevaluedoesnotmatter" + } } } } }`, sixHoursAgo.Format(time.RFC3339)))) reg.Register(httpmock.REST("GET", "gists/1234"), - httpmock.StatusStringResponse(404, "Not Found")) - } else { - reg.Register(httpmock.REST("GET", "gists/1234"), - httpmock.JSONResponse(tt.gist)) + httpmock.JSONResponse(tt.mockGist)) reg.Register(httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data":{"viewer":{"login":"octocat"}}}`)) + + gistList := "cicada.txt whatever about 6 hours ago" + pm.RegisterSelect("Select a gist", + []string{gistList}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, gistList) + }) } if tt.httpStubs != nil { tt.httpStubs(reg) } - if tt.opts == nil { - tt.opts = &EditOptions{} - } - tt.opts.Edit = func(_, _, _ string, _ *iostreams.IOStreams) (string, error) { return "new file content", nil } @@ -558,19 +626,17 @@ func Test_editRun(t *testing.T) { } ios, stdin, stdout, stderr := iostreams.Test() stdin.WriteString(tt.stdin) - ios.SetStdoutTTY(!tt.nontty) - ios.SetStdinTTY(!tt.nontty) - ios.SetStderrTTY(!tt.nontty) + ios.SetStdoutTTY(tt.istty) + ios.SetStdinTTY(tt.istty) + ios.SetStderrTTY(tt.istty) tt.opts.IO = ios - tt.opts.Selector = "1234" tt.opts.Config = func() (gh.Config, error) { return config.NewBlankConfig(), nil } t.Run(tt.name, func(t *testing.T) { - pm := prompter.NewMockPrompter(t) if tt.prompterStubs != nil { tt.prompterStubs(pm) } @@ -584,14 +650,21 @@ func Test_editRun(t *testing.T) { } assert.NoError(t, err) - if tt.wantParams != nil { - bodyBytes, _ := io.ReadAll(reg.Requests[2].Body) + if tt.wantLastRequestParameters != nil { + // Currently only checking that the last request has + // the expected request parameters. + // + // This might need to be changed, if a test were to be added + // that needed to check that a request other than the last + // has the desired parameters. + lastRequest := reg.Requests[len(reg.Requests)-1] + bodyBytes, _ := io.ReadAll(lastRequest.Body) reqBody := make(map[string]interface{}) err = json.Unmarshal(bodyBytes, &reqBody) if err != nil { t.Fatalf("error decoding JSON: %v", err) } - assert.Equal(t, tt.wantParams, reqBody) + assert.Equal(t, tt.wantLastRequestParameters, reqBody) } assert.Equal(t, "", stdout.String()) From dec46670bb356a214e0ae4ecc8fd689a55df5214 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Jan 2025 11:14:33 -0700 Subject: [PATCH 15/21] Fix: gist edit/view tests var name consistency --- pkg/cmd/gist/edit/edit_test.go | 16 ++++++++-------- pkg/cmd/gist/view/view_test.go | 28 ++++++++++++++-------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index a40695c39..5a3f0a268 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -149,7 +149,7 @@ func Test_editRun(t *testing.T) { mockGistList bool httpStubs func(*httpmock.Registry) prompterStubs func(*prompter.MockPrompter) - istty bool + isTTY bool stdin string wantErr string wantLastRequestParameters map[string]interface{} @@ -163,7 +163,7 @@ func Test_editRun(t *testing.T) { }, { name: "one file", - istty: false, + isTTY: false, opts: &EditOptions{ Selector: "1234", }, @@ -194,7 +194,7 @@ func Test_editRun(t *testing.T) { }, { name: "multiple files, submit, with TTY", - istty: true, + isTTY: true, mockGistList: true, prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect("Edit which file?", @@ -243,7 +243,7 @@ func Test_editRun(t *testing.T) { }, { name: "multiple files, cancel", - istty: true, + isTTY: true, opts: &EditOptions{ Selector: "1234", }, @@ -551,7 +551,7 @@ func Test_editRun(t *testing.T) { }, { name: "no arguments notty", - istty: false, + isTTY: false, opts: &EditOptions{ Selector: "", }, @@ -626,9 +626,9 @@ func Test_editRun(t *testing.T) { } ios, stdin, stdout, stderr := iostreams.Test() stdin.WriteString(tt.stdin) - ios.SetStdoutTTY(tt.istty) - ios.SetStdinTTY(tt.istty) - ios.SetStderrTTY(tt.istty) + ios.SetStdoutTTY(tt.isTTY) + ios.SetStdinTTY(tt.isTTY) + ios.SetStderrTTY(tt.isTTY) tt.opts.IO = ios diff --git a/pkg/cmd/gist/view/view_test.go b/pkg/cmd/gist/view/view_test.go index 820401f1e..706b85f10 100644 --- a/pkg/cmd/gist/view/view_test.go +++ b/pkg/cmd/gist/view/view_test.go @@ -116,10 +116,10 @@ func Test_viewRun(t *testing.T) { name string opts *ViewOptions wantOut string - gist *shared.Gist + mockGist *shared.Gist + mockGistList bool isTTY bool wantErr string - mockGistList bool }{ { name: "no such gist", @@ -137,7 +137,7 @@ func Test_viewRun(t *testing.T) { Selector: "1234", ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { Content: "bwhiizzzbwhuiiizzzz", @@ -155,7 +155,7 @@ func Test_viewRun(t *testing.T) { ListFiles: false, }, mockGistList: true, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { Content: "test interactive mode", @@ -178,7 +178,7 @@ func Test_viewRun(t *testing.T) { Filename: "cicada.txt", ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { Content: "bwhiizzzbwhuiiizzzz", @@ -201,7 +201,7 @@ func Test_viewRun(t *testing.T) { Raw: true, ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { Content: "bwhiizzzbwhuiiizzzz", @@ -222,7 +222,7 @@ func Test_viewRun(t *testing.T) { Selector: "1234", ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { Content: "bwhiizzzbwhuiiizzzz", @@ -243,7 +243,7 @@ func Test_viewRun(t *testing.T) { Selector: "1234", ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Files: map[string]*shared.GistFile{ "cicada.txt": { Content: "bwhiizzzbwhuiiizzzz\n", @@ -264,7 +264,7 @@ func Test_viewRun(t *testing.T) { Selector: "1234", ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Description: "some files", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -287,7 +287,7 @@ func Test_viewRun(t *testing.T) { Raw: true, ListFiles: false, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Description: "some files", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -310,7 +310,7 @@ func Test_viewRun(t *testing.T) { Raw: false, ListFiles: true, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Description: "some files", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -329,7 +329,7 @@ func Test_viewRun(t *testing.T) { Raw: false, ListFiles: true, }, - gist: &shared.Gist{ + mockGist: &shared.Gist{ Description: "some files", Files: map[string]*shared.GistFile{ "cicada.txt": { @@ -348,12 +348,12 @@ func Test_viewRun(t *testing.T) { for _, tt := range tests { reg := &httpmock.Registry{} - if tt.gist == nil { + if tt.mockGist == nil { reg.Register(httpmock.REST("GET", "gists/1234"), httpmock.StatusStringResponse(404, "Not Found")) } else { reg.Register(httpmock.REST("GET", "gists/1234"), - httpmock.JSONResponse(tt.gist)) + httpmock.JSONResponse(tt.mockGist)) } if tt.opts == nil { From 5b6fd53a88498caab47c0e24e06c427f53574814 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Jan 2025 11:36:48 -0700 Subject: [PATCH 16/21] Fix: gist edit test name --- pkg/cmd/gist/edit/edit_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/gist/edit/edit_test.go b/pkg/cmd/gist/edit/edit_test.go index 5a3f0a268..728f6e894 100644 --- a/pkg/cmd/gist/edit/edit_test.go +++ b/pkg/cmd/gist/edit/edit_test.go @@ -242,7 +242,7 @@ func Test_editRun(t *testing.T) { }, }, { - name: "multiple files, cancel", + name: "multiple files, cancel, with TTY", isTTY: true, opts: &EditOptions{ Selector: "1234", From facd0caa2971ad3620f451d1968e04aab2cce17c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 16 Jan 2025 11:44:46 -0700 Subject: [PATCH 17/21] Fix: accidental whitespace in gist edit --- pkg/cmd/gist/edit/edit.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index b85118937..e0d6e3b84 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -113,7 +113,6 @@ func editRun(opts *EditOptions) error { } gist, err := shared.PromptGists(opts.Prompter, client, host, cs) - if err != nil { return err } From 7942be56aad37d163e970df17d0b5a4cdf2c09c1 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Fri, 17 Jan 2025 12:07:32 -0700 Subject: [PATCH 18/21] Add affected version to bug report form --- .github/ISSUE_TEMPLATE/bug_report.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 00f2a0ba5..ae0d29096 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -9,7 +9,11 @@ assignees: '' ### Describe the bug -A clear and concise description of what the bug is. Include version by typing `gh --version`. +A clear and concise description of what the bug is. + +### Affected version + +Please run `gh version` and paste the output below. ### Steps to reproduce the behavior From 31bff5b5a8020b684566c73e2618fc65b3b8daa4 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Wed, 8 Jan 2025 02:30:08 +0000 Subject: [PATCH 19/21] Better handling for codespace state polling --- internal/codespaces/api/api.go | 2 + internal/codespaces/codespaces.go | 70 ++++++---- internal/codespaces/codespaces_test.go | 175 +++++++++++++++++++++++++ 3 files changed, 220 insertions(+), 27 deletions(-) create mode 100644 internal/codespaces/codespaces_test.go diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 9e79eafe1..0d1eaf5b3 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -240,6 +240,8 @@ const ( CodespaceStateAvailable = "Available" // CodespaceStateShutdown is the state for a shutdown codespace environment. CodespaceStateShutdown = "Shutdown" + // CodespaceStateShuttingDown is the state for a shutting down codespace environment. + CodespaceStateShuttingDown = "ShuttingDown" // CodespaceStateStarting is the state for a starting codespace environment. CodespaceStateStarting = "Starting" // CodespaceStateRebuilding is the state for a rebuilding codespace environment. diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index da2eacd44..38a2e903e 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -13,6 +13,10 @@ import ( "github.com/cli/cli/v2/internal/codespaces/connection" ) +// codespaceStatePollingBackoff is the delay between state polls while waiting for codespaces to become +// available. It's only exposed so that it can be shortened for testing, otherwise it should not be changed +var codespaceStatePollingBackoff = backoff.NewConstantBackOff(10 * time.Second) + func connectionReady(codespace *api.Codespace) bool { // If the codespace is not available, it is not ready if codespace.State != api.CodespaceStateAvailable { @@ -67,41 +71,53 @@ func GetCodespaceConnection(ctx context.Context, progress progressIndicator, api // waitUntilCodespaceConnectionReady waits for a Codespace to be running and is able to be connected to. func waitUntilCodespaceConnectionReady(ctx context.Context, progress progressIndicator, apiClient apiClient, codespace *api.Codespace) (*api.Codespace, error) { - if codespace.State != api.CodespaceStateAvailable { - progress.StartProgressIndicatorWithLabel("Starting codespace") - defer progress.StopProgressIndicator() - if err := apiClient.StartCodespace(ctx, codespace.Name); err != nil { - return nil, fmt.Errorf("error starting codespace: %w", err) - } + if connectionReady(codespace) { + return codespace, nil } - if !connectionReady(codespace) { - expBackoff := backoff.NewExponentialBackOff() - expBackoff.Multiplier = 1.1 - expBackoff.MaxInterval = 10 * time.Second - expBackoff.MaxElapsedTime = 5 * time.Minute + progress.StartProgressIndicatorWithLabel("Waiting for codespace to become ready") + defer progress.StopProgressIndicator() - err := backoff.Retry(func() error { - var err error + lastState := "" + firstRetry := true + + err := backoff.Retry(func() error { + var err error + if firstRetry { + firstRetry = false + } else { codespace, err = apiClient.GetCodespace(ctx, codespace.Name, true) if err != nil { return backoff.Permanent(fmt.Errorf("error getting codespace: %w", err)) } - - if connectionReady(codespace) { - return nil - } - - return &TimeoutError{message: "codespace not ready yet"} - }, backoff.WithContext(expBackoff, ctx)) - if err != nil { - var timeoutErr *TimeoutError - if errors.As(err, &timeoutErr) { - return nil, errors.New("timed out while waiting for the codespace to start") - } - - return nil, err } + + if connectionReady(codespace) { + return nil + } + + // Only react to changes in the state (so that we don't try to start the codespace twice) + if codespace.State != lastState { + if codespace.State == api.CodespaceStateShutdown { + err = apiClient.StartCodespace(ctx, codespace.Name) + if err != nil { + return backoff.Permanent(fmt.Errorf("error starting codespace: %w", err)) + } + } + } + + lastState = codespace.State + + return &TimeoutError{message: "codespace not ready yet"} + }, backoff.WithContext(codespaceStatePollingBackoff, ctx)) + + if err != nil { + var timeoutErr *TimeoutError + if errors.As(err, &timeoutErr) { + return nil, errors.New("timed out while waiting for the codespace to start") + } + + return nil, err } return codespace, nil diff --git a/internal/codespaces/codespaces_test.go b/internal/codespaces/codespaces_test.go new file mode 100644 index 000000000..c9d37aca2 --- /dev/null +++ b/internal/codespaces/codespaces_test.go @@ -0,0 +1,175 @@ +package codespaces + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/cenkalti/backoff/v4" + "github.com/cli/cli/v2/internal/codespaces/api" +) + +func init() { + // Set the backoff to 0 for testing so that they run quickly + codespaceStatePollingBackoff = backoff.NewConstantBackOff(time.Second * 0) +} + +// This is just enough to trick `connectionReady` +var readyCodespace = &api.Codespace{ + State: api.CodespaceStateAvailable, + Connection: api.CodespaceConnection{ + TunnelProperties: api.TunnelProperties{ + ConnectAccessToken: "test", + ManagePortsAccessToken: "test", + ServiceUri: "test", + TunnelId: "test", + ClusterId: "test", + Domain: "test", + }, + }, +} + +func TestWaitUntilCodespaceConnectionReady_WhenAlreadyReady(t *testing.T) { + t.Parallel() + + apiClient := &mockApiClient{} + waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, readyCodespace) +} + +func TestWaitUntilCodespaceConnectionReady_PollsApi(t *testing.T) { + t.Parallel() + + apiClient := &mockApiClient{ + onGetCodespace: func() (*api.Codespace, error) { + return readyCodespace, nil + }, + } + waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, &api.Codespace{State: api.CodespaceStateStarting}) +} + +func TestWaitUntilCodespaceConnectionReady_StartsCodespace(t *testing.T) { + t.Parallel() + + codespace := &api.Codespace{State: api.CodespaceStateShutdown} + + apiClient := &mockApiClient{ + onGetCodespace: func() (*api.Codespace, error) { + return codespace, nil + }, + onStartCodespace: func() error { + *codespace = *readyCodespace + return nil + }, + } + waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) +} + +func TestWaitUntilCodespaceConnectionReady_PollsCodespaceUntilReady(t *testing.T) { + t.Parallel() + + codespace := &api.Codespace{State: api.CodespaceStateShutdown} + hasPolled := false + + apiClient := &mockApiClient{ + onGetCodespace: func() (*api.Codespace, error) { + if hasPolled { + *codespace = *readyCodespace + } + + hasPolled = true + + return codespace, nil + }, + onStartCodespace: func() error { + codespace.State = api.CodespaceStateStarting + return nil + }, + } + waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) +} + +func TestWaitUntilCodespaceConnectionReady_WaitsForShutdownBeforeStarting(t *testing.T) { + t.Parallel() + + codespace := &api.Codespace{State: api.CodespaceStateShuttingDown} + + apiClient := &mockApiClient{ + onGetCodespace: func() (*api.Codespace, error) { + // Make sure that we poll at least once before going to shutdown + if codespace.State == api.CodespaceStateShuttingDown { + codespace.State = api.CodespaceStateShutdown + } + return codespace, nil + }, + onStartCodespace: func() error { + if codespace.State != api.CodespaceStateShutdown { + t.Fatalf("Codespace started from non-shutdown state: %s", codespace.State) + } + *codespace = *readyCodespace + return nil + }, + } + waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) +} + +func TestWaitUntilCodespaceConnectionReady_DoesntStartTwice(t *testing.T) { + t.Parallel() + + codespace := &api.Codespace{State: api.CodespaceStateShutdown} + didStart := false + didPollAfterStart := false + + apiClient := &mockApiClient{ + onGetCodespace: func() (*api.Codespace, error) { + // Make sure that we are in shutdown state for one poll after starting to make sure we don't try to start again + if didPollAfterStart { + *codespace = *readyCodespace + } + + if didStart { + didPollAfterStart = true + } + + return codespace, nil + }, + onStartCodespace: func() error { + if didStart { + t.Fatal("Should not start multiple times") + } + didStart = true + return nil + }, + } + waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) +} + +type mockApiClient struct { + onStartCodespace func() error + onGetCodespace func() (*api.Codespace, error) +} + +func (m *mockApiClient) StartCodespace(ctx context.Context, name string) error { + if m.onStartCodespace == nil { + panic("onStartCodespace not set and StartCodespace was called") + } + + return m.onStartCodespace() +} + +func (m *mockApiClient) GetCodespace(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) { + if m.onGetCodespace == nil { + panic("onGetCodespace not set and GetCodespace was called") + } + + return m.onGetCodespace() +} + +func (m *mockApiClient) HTTPClient() (*http.Client, error) { + panic("Not implemented") +} + +type mockProgressIndicator struct{} + +func (m *mockProgressIndicator) StartProgressIndicatorWithLabel(s string) {} +func (m *mockProgressIndicator) StopProgressIndicator() {} From 9b8cd7babd342a0a295d92078ef960cbc3cb0c2a Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Fri, 10 Jan 2025 20:11:17 +0000 Subject: [PATCH 20/21] Linter --- internal/codespaces/codespaces_test.go | 51 ++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 7 deletions(-) diff --git a/internal/codespaces/codespaces_test.go b/internal/codespaces/codespaces_test.go index c9d37aca2..d931b96ef 100644 --- a/internal/codespaces/codespaces_test.go +++ b/internal/codespaces/codespaces_test.go @@ -34,7 +34,13 @@ func TestWaitUntilCodespaceConnectionReady_WhenAlreadyReady(t *testing.T) { t.Parallel() apiClient := &mockApiClient{} - waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, readyCodespace) + result, err := waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, readyCodespace) + if err != nil { + t.Fatalf("Expected nil error, but was %v", err) + } + if result.State != api.CodespaceStateAvailable { + t.Fatalf("Expected final state to be %s, but was %s", api.CodespaceStateAvailable, result.State) + } } func TestWaitUntilCodespaceConnectionReady_PollsApi(t *testing.T) { @@ -45,7 +51,14 @@ func TestWaitUntilCodespaceConnectionReady_PollsApi(t *testing.T) { return readyCodespace, nil }, } - waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, &api.Codespace{State: api.CodespaceStateStarting}) + result, err := waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, &api.Codespace{State: api.CodespaceStateStarting}) + + if err != nil { + t.Fatalf("Expected nil error, but was %v", err) + } + if result.State != api.CodespaceStateAvailable { + t.Fatalf("Expected final state to be %s, but was %s", api.CodespaceStateAvailable, result.State) + } } func TestWaitUntilCodespaceConnectionReady_StartsCodespace(t *testing.T) { @@ -62,7 +75,13 @@ func TestWaitUntilCodespaceConnectionReady_StartsCodespace(t *testing.T) { return nil }, } - waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + result, err := waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + if err != nil { + t.Fatalf("Expected nil error, but was %v", err) + } + if result.State != api.CodespaceStateAvailable { + t.Fatalf("Expected final state to be %s, but was %s", api.CodespaceStateAvailable, result.State) + } } func TestWaitUntilCodespaceConnectionReady_PollsCodespaceUntilReady(t *testing.T) { @@ -86,7 +105,13 @@ func TestWaitUntilCodespaceConnectionReady_PollsCodespaceUntilReady(t *testing.T return nil }, } - waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + result, err := waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + if err != nil { + t.Fatalf("Expected nil error, but was %v", err) + } + if result.State != api.CodespaceStateAvailable { + t.Fatalf("Expected final state to be %s, but was %s", api.CodespaceStateAvailable, result.State) + } } func TestWaitUntilCodespaceConnectionReady_WaitsForShutdownBeforeStarting(t *testing.T) { @@ -110,10 +135,16 @@ func TestWaitUntilCodespaceConnectionReady_WaitsForShutdownBeforeStarting(t *tes return nil }, } - waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + result, err := waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + if err != nil { + t.Fatalf("Expected nil error, but was %v", err) + } + if result.State != api.CodespaceStateAvailable { + t.Fatalf("Expected final state to be %s, but was %s", api.CodespaceStateAvailable, result.State) + } } -func TestWaitUntilCodespaceConnectionReady_DoesntStartTwice(t *testing.T) { +func TestUntilCodespaceConnectionReady_DoesntStartTwice(t *testing.T) { t.Parallel() codespace := &api.Codespace{State: api.CodespaceStateShutdown} @@ -141,7 +172,13 @@ func TestWaitUntilCodespaceConnectionReady_DoesntStartTwice(t *testing.T) { return nil }, } - waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + result, err := waitUntilCodespaceConnectionReady(context.Background(), &mockProgressIndicator{}, apiClient, codespace) + if err != nil { + t.Fatalf("Expected nil error, but was %v", err) + } + if result.State != api.CodespaceStateAvailable { + t.Fatalf("Expected final state to be %s, but was %s", api.CodespaceStateAvailable, result.State) + } } type mockApiClient struct { From 0c8591064dec9ed95d7fba10a9decb5fff1ee605 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Mon, 13 Jan 2025 22:09:11 +0000 Subject: [PATCH 21/21] Change back to exponential backoff --- internal/codespaces/codespaces.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 38a2e903e..92185120a 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -15,7 +15,12 @@ import ( // codespaceStatePollingBackoff is the delay between state polls while waiting for codespaces to become // available. It's only exposed so that it can be shortened for testing, otherwise it should not be changed -var codespaceStatePollingBackoff = backoff.NewConstantBackOff(10 * time.Second) +var codespaceStatePollingBackoff backoff.BackOff = backoff.NewExponentialBackOff( + backoff.WithInitialInterval(1*time.Second), + backoff.WithMultiplier(1.02), + backoff.WithMaxInterval(10*time.Second), + backoff.WithMaxElapsedTime(5*time.Minute), +) func connectionReady(codespace *api.Codespace) bool { // If the codespace is not available, it is not ready