Merge pull request #10773 from cli/kw/opt-into-no-spinners

Introduce option to opt-out of spinners

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

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

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.
 		`, "`"),
 	},
 	{

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()

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)
+	}
+}