diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 6286c999d..7a45efeb4 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -6,6 +6,7 @@ import ( "net/http" "os" "regexp" + "slices" "time" "github.com/cli/cli/v2/api" @@ -283,6 +284,12 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io.SetNeverPrompt(true) } + ghSpinnerDisabledValue, ghSpinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") + falseyValues := []string{"false", "0", "no", ""} + if ghSpinnerDisabledIsSet && !slices.Contains(falseyValues, ghSpinnerDisabledValue) { + io.SetSpinnerDisabled(true) + } + // Pager precedence // 1. GH_PAGER // 2. pager from config diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index c0275d1de..5036a1dc1 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -432,6 +432,54 @@ func Test_ioStreams_prompt(t *testing.T) { } } +func Test_ioStreams_spinnerDisabled(t *testing.T) { + tests := []struct { + name string + spinnerDisabled bool + env map[string]string + }{ + { + name: "default config", + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = 0", + env: map[string]string{"GH_SPINNER_DISABLED": "0"}, + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = false", + env: map[string]string{"GH_SPINNER_DISABLED": "false"}, + spinnerDisabled: false, + }, + { + name: "spinner disabled via GH_SPINNER_DISABLED env var = no", + env: map[string]string{"GH_SPINNER_DISABLED": "no"}, + spinnerDisabled: false, + }, + { + name: "spinner enabled via GH_SPINNER_DISABLED env var = 1", + env: map[string]string{"GH_SPINNER_DISABLED": "1"}, + spinnerDisabled: true, + }, + { + name: "spinner enabled via GH_SPINNER_DISABLED env var = true", + env: map[string]string{"GH_SPINNER_DISABLED": "true"}, + spinnerDisabled: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + f := New("1") + io := ioStreams(f) + assert.Equal(t, tt.spinnerDisabled, io.GetSpinnerDisabled()) + }) + } +} + func Test_ioStreams_colorLabels(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index b85d64ca3..4b692777c 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -114,6 +114,9 @@ var HelpTopics = []helpTopic{ %[1]sGH_ACCESSIBLE_PROMPTER%[1]s (preview): set to a truthy value to enable prompts that are more compatible with speech synthesis and braille screen readers. + + %[1]sGH_SPINNER_DISABLED%[1]s: set to a truthy value to replace the spinner animation with + a textual progress indicator. `, "`"), }, { diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index f5e3c2aee..ba2cc6b50 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -78,7 +78,8 @@ type IOStreams struct { pagerCommand string pagerProcess *os.Process - neverPrompt bool + neverPrompt bool + spinnerDisabled bool TempFileOverride *os.File } @@ -273,6 +274,14 @@ func (s *IOStreams) SetNeverPrompt(v bool) { s.neverPrompt = v } +func (s *IOStreams) GetSpinnerDisabled() bool { + return s.spinnerDisabled +} + +func (s *IOStreams) SetSpinnerDisabled(v bool) { + s.spinnerDisabled = v +} + func (s *IOStreams) StartProgressIndicator() { s.StartProgressIndicatorWithLabel("") } @@ -282,6 +291,15 @@ func (s *IOStreams) StartProgressIndicatorWithLabel(label string) { return } + if s.spinnerDisabled { + // If the spinner is disabled, simply print a + // textual progress indicator and return. + // This means that s.ProgressIndicator will be nil. + // See also: the comment on StopProgressIndicator() + s.startTextualProgressIndicator(label) + return + } + s.progressIndicatorMu.Lock() defer s.progressIndicatorMu.Unlock() @@ -295,8 +313,10 @@ func (s *IOStreams) StartProgressIndicatorWithLabel(label string) { } // https://github.com/briandowns/spinner#available-character-sets - dotStyle := spinner.CharSets[11] - sp := spinner.New(dotStyle, 120*time.Millisecond, spinner.WithWriter(s.ErrOut), spinner.WithColor("fgCyan")) + // ⣾ ⣷ ⣽ ⣻ ⡿ + spinnerStyle := spinner.CharSets[11] + + sp := spinner.New(spinnerStyle, 120*time.Millisecond, spinner.WithWriter(s.ErrOut), spinner.WithColor("fgCyan")) if label != "" { sp.Prefix = label + " " } @@ -305,6 +325,27 @@ func (s *IOStreams) StartProgressIndicatorWithLabel(label string) { s.progressIndicator = sp } +func (s *IOStreams) startTextualProgressIndicator(label string) { + s.progressIndicatorMu.Lock() + defer s.progressIndicatorMu.Unlock() + + // Default label when spinner disabled is "Working..." + if label == "" { + label = "Working..." + } + + // Add an ellipsis to the label if it doesn't already have one. + ellipsis := "..." + if !strings.HasSuffix(label, ellipsis) { + label = label + ellipsis + } + + fmt.Fprintf(s.ErrOut, "%s%s", s.ColorScheme().Cyan(label), "\n") +} + +// StopProgressIndicator stops the progress indicator if it is running. +// Note that a textual progess indicator does not create a progress indicator, +// so this method is a no-op in that case. func (s *IOStreams) StopProgressIndicator() { s.progressIndicatorMu.Lock() defer s.progressIndicatorMu.Unlock() diff --git a/pkg/iostreams/iostreams_progress_indicator_test.go b/pkg/iostreams/iostreams_progress_indicator_test.go new file mode 100644 index 000000000..60d0ece91 --- /dev/null +++ b/pkg/iostreams/iostreams_progress_indicator_test.go @@ -0,0 +1,254 @@ +//go:build !windows + +package iostreams + +import ( + "fmt" + "io" + "os" + "strings" + "testing" + "time" + + "github.com/Netflix/go-expect" + "github.com/creack/pty" + "github.com/hinshun/vt10x" + "github.com/stretchr/testify/require" +) + +func TestStartProgressIndicatorWithLabel(t *testing.T) { + osOut := os.Stdout + defer func() { os.Stdout = osOut }() + // Why do we need a channel in these tests to implement a timeout instead of + // relying on expect's timeout? + // + // Well, expect's timeout is based on the maximum time of a single read + // from the console. This works in cases like prompting where we block + // waiting for input because the console is not ready to be read. + // But in this case, we are not blocking waiting for input and stdout + // can be constantly read. This means the timeout will never be reached + // in the event of a expectation failure. + // To fix this, we need to implement our own timeout that is based + // specifically on the total time spent reading the console and waiting + // for the target string instead of the max time for a single read + // from the console. + t.Run("progress indicator respects GH_SPINNER_DISABLED is true", func(t *testing.T) { + console := newTestVirtualTerminal(t) + io := newTestIOStreams(t, console, true) + + done := make(chan error) + + go func() { + _, err := console.ExpectString("Working...") + done <- err + }() + + io.StartProgressIndicatorWithLabel("") + defer io.StopProgressIndicator() + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("Test timed out waiting for progress indicator") + } + }) + + t.Run("progress indicator respects GH_SPINNER_DISABLED is false", func(t *testing.T) { + console := newTestVirtualTerminal(t) + io := newTestIOStreams(t, console, false) + + done := make(chan error) + + go func() { + _, err := console.ExpectString("⣾") + done <- err + }() + + io.StartProgressIndicatorWithLabel("") + defer io.StopProgressIndicator() + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("Test timed out waiting for progress indicator") + } + }) + + t.Run("progress indicator with GH_SPINNER_DISABLED shows label", func(t *testing.T) { + console := newTestVirtualTerminal(t) + io := newTestIOStreams(t, console, true) + progressIndicatorLabel := "downloading happiness" + + done := make(chan error) + + go func() { + _, err := console.ExpectString(progressIndicatorLabel + "...") + done <- err + }() + + io.StartProgressIndicatorWithLabel(progressIndicatorLabel) + defer io.StopProgressIndicator() + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("Test timed out waiting for progress indicator") + } + }) + + t.Run("progress indicator shows label and spinner", func(t *testing.T) { + console := newTestVirtualTerminal(t) + io := newTestIOStreams(t, console, false) + progressIndicatorLabel := "downloading happiness" + + done := make(chan error) + + go func() { + _, err := console.ExpectString(progressIndicatorLabel) + require.NoError(t, err) + _, err = console.ExpectString("⣾") + done <- err + }() + + io.StartProgressIndicatorWithLabel(progressIndicatorLabel) + defer io.StopProgressIndicator() + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("Test timed out waiting for progress indicator") + } + }) + + t.Run("multiple calls to start progress indicator with GH_SPINNER_DISABLED prints additional labels", func(t *testing.T) { + console := newTestVirtualTerminal(t) + io := newTestIOStreams(t, console, true) + progressIndicatorLabel1 := "downloading happiness" + progressIndicatorLabel2 := "downloading sadness" + done := make(chan error) + go func() { + _, err := console.ExpectString(progressIndicatorLabel1 + "...") + require.NoError(t, err) + _, err = console.ExpectString(progressIndicatorLabel2 + "...") + done <- err + }() + io.StartProgressIndicatorWithLabel(progressIndicatorLabel1) + defer io.StopProgressIndicator() + io.StartProgressIndicatorWithLabel(progressIndicatorLabel2) + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(2 * time.Second): + t.Fatal("Test timed out waiting for progress indicator") + } + }) +} + +func newTestVirtualTerminal(t *testing.T) *expect.Console { + t.Helper() + + // Create a PTY and hook up a virtual terminal emulator + ptm, pts, err := pty.Open() + require.NoError(t, err) + + term := vt10x.New(vt10x.WithWriter(pts)) + + // Create a console via Expect that allows scripting against the terminal + consoleOpts := []expect.ConsoleOpt{ + expect.WithStdin(ptm), + expect.WithStdout(term), + expect.WithCloser(ptm, pts), + failOnExpectError(t), + failOnSendError(t), + expect.WithDefaultTimeout(time.Second), + } + + console, err := expect.NewConsole(consoleOpts...) + require.NoError(t, err) + t.Cleanup(func() { testCloser(t, console) }) + + return console +} + +func newTestIOStreams(t *testing.T, console *expect.Console, spinnerDisabled bool) *IOStreams { + t.Helper() + + in := console.Tty() + out := console.Tty() + errOut := console.Tty() + + // Because the briandowns/spinner checks os.Stdout directly, + // we need this hack to trick it into allowing the spinner to print... + os.Stdout = out + + io := &IOStreams{ + In: in, + Out: out, + ErrOut: errOut, + term: fakeTerm{}, + } + io.progressIndicatorEnabled = true + io.SetSpinnerDisabled(spinnerDisabled) + return io +} + +// failOnExpectError adds an observer that will fail the test in a standardised way +// if any expectation on the command output fails, without requiring an explicit +// assertion. +// +// Use WithRelaxedIO to disable this behaviour. +func failOnExpectError(t *testing.T) expect.ConsoleOpt { + t.Helper() + return expect.WithExpectObserver( + func(matchers []expect.Matcher, buf string, err error) { + t.Helper() + + if err == nil { + return + } + + if len(matchers) == 0 { + t.Fatalf("Error occurred while matching %q: %s\n", buf, err) + } + + var criteria []string + for _, matcher := range matchers { + criteria = append(criteria, fmt.Sprintf("%q", matcher.Criteria())) + } + t.Fatalf("Failed to find [%s] in %q: %s\n", strings.Join(criteria, ", "), buf, err) + }, + ) +} + +// failOnSendError adds an observer that will fail the test in a standardised way +// if any sending of input fails, without requiring an explicit assertion. +// +// Use WithRelaxedIO to disable this behaviour. +func failOnSendError(t *testing.T) expect.ConsoleOpt { + t.Helper() + return expect.WithSendObserver( + func(msg string, n int, err error) { + t.Helper() + + if err != nil { + t.Fatalf("Failed to send %q: %s\n", msg, err) + } + if len(msg) != n { + t.Fatalf("Only sent %d of %d bytes for %q\n", n, len(msg), msg) + } + }, + ) +} + +// testCloser is a helper to fail the test if a Closer fails to close. +func testCloser(t *testing.T, closer io.Closer) { + t.Helper() + if err := closer.Close(); err != nil { + t.Errorf("Close failed: %s", err) + } +}