From bc8c46b0b1c1fd411e286990b950aad254cfb78f Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 13 Jan 2025 20:38:03 -0500 Subject: [PATCH 1/3] 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 243acaf5792d62234f769c2a9125f81616cd6328 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Thu, 16 Jan 2025 00:08:45 -0500 Subject: [PATCH 2/3] 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 3/3] 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 }, }