From aa0de5f636ce38a211749b6e1ea04e716c19eaa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 Jan 2021 14:46:45 +0100 Subject: [PATCH 1/2] Stop using utils.Spinner in `repo fork` --- pkg/cmd/repo/fork/fork.go | 17 ++--------------- pkg/cmd/repo/fork/fork_test.go | 21 --------------------- 2 files changed, 2 insertions(+), 36 deletions(-) diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 760bb6537..322e08173 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -127,18 +127,6 @@ func forkRun(opts *ForkOptions) error { cs := opts.IO.ColorScheme() stderr := opts.IO.ErrOut - s := utils.Spinner(stderr) - stopSpinner := func() {} - - if connectedToTerminal { - loading := cs.Gray("Forking ") + cs.Bold(cs.Gray(ghrepo.FullName(repoToFork))) + cs.Gray("...") - s.Suffix = " " + loading - s.FinalMSG = cs.Gray(fmt.Sprintf("- %s\n", loading)) - utils.StartSpinner(s) - stopSpinner = func() { - utils.StopSpinner(s) - } - } httpClient, err := opts.HttpClient() if err != nil { @@ -147,14 +135,13 @@ func forkRun(opts *ForkOptions) error { apiClient := api.NewClientFromHTTP(httpClient) + opts.IO.StartProgressIndicator() forkedRepo, err := api.ForkRepo(apiClient, repoToFork) + opts.IO.StopProgressIndicator() if err != nil { - stopSpinner() return fmt.Errorf("failed to fork: %w", err) } - stopSpinner() - // This is weird. There is not an efficient way to determine via the GitHub API whether or not a // given user has forked a given repo. We noticed, also, that the create fork API endpoint just // returns the fork repo data even if it already exists -- with no change in status code or diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 20f396394..93c73d740 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/briandowns/spinner" "github.com/cli/cli/context" "github.com/cli/cli/git" "github.com/cli/cli/internal/config" @@ -20,7 +19,6 @@ import ( "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/test" - "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/stretchr/testify/assert" ) @@ -155,7 +153,6 @@ func TestRepoFork_outside_parent_nontty(t *testing.T) { } func TestRepoFork_already_forked(t *testing.T) { - stubSpinner() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() httpClient := &http.Client{Transport: reg} @@ -180,7 +177,6 @@ func TestRepoFork_already_forked(t *testing.T) { } func TestRepoFork_reuseRemote(t *testing.T) { - stubSpinner() remotes := []*context.Remote{ { Remote: &git.Remote{Name: "origin", FetchURL: &url.URL{}}, @@ -208,7 +204,6 @@ func TestRepoFork_reuseRemote(t *testing.T) { } func TestRepoFork_in_parent(t *testing.T) { - stubSpinner() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() httpClient := &http.Client{Transport: reg} @@ -234,7 +229,6 @@ func TestRepoFork_in_parent(t *testing.T) { } func TestRepoFork_outside(t *testing.T) { - stubSpinner() tests := []struct { name string args string @@ -273,7 +267,6 @@ func TestRepoFork_outside(t *testing.T) { } func TestRepoFork_in_parent_yes(t *testing.T) { - stubSpinner() defer stubSince(2 * time.Second)() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() @@ -308,7 +301,6 @@ func TestRepoFork_in_parent_yes(t *testing.T) { } func TestRepoFork_outside_yes(t *testing.T) { - stubSpinner() defer stubSince(2 * time.Second)() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() @@ -337,7 +329,6 @@ func TestRepoFork_outside_yes(t *testing.T) { } func TestRepoFork_outside_survey_yes(t *testing.T) { - stubSpinner() defer stubSince(2 * time.Second)() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() @@ -368,7 +359,6 @@ func TestRepoFork_outside_survey_yes(t *testing.T) { } func TestRepoFork_outside_survey_no(t *testing.T) { - stubSpinner() defer stubSince(2 * time.Second)() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() @@ -400,7 +390,6 @@ func TestRepoFork_outside_survey_no(t *testing.T) { } func TestRepoFork_in_parent_survey_yes(t *testing.T) { - stubSpinner() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() httpClient := &http.Client{Transport: reg} @@ -438,7 +427,6 @@ func TestRepoFork_in_parent_survey_yes(t *testing.T) { } func TestRepoFork_in_parent_survey_no(t *testing.T) { - stubSpinner() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() httpClient := &http.Client{Transport: reg} @@ -470,7 +458,6 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) { } func TestRepoFork_in_parent_match_protocol(t *testing.T) { - stubSpinner() defer stubSince(2 * time.Second)() reg := &httpmock.Registry{} defer reg.StubWithFixturePath(200, "./forkResult.json")() @@ -513,14 +500,6 @@ func TestRepoFork_in_parent_match_protocol(t *testing.T) { reg.Verify(t) } -func stubSpinner() { - // not bothering with teardown since we never want spinners when doing tests - utils.StartSpinner = func(_ *spinner.Spinner) { - } - utils.StopSpinner = func(_ *spinner.Spinner) { - } -} - func stubSince(d time.Duration) func() { originalSince := Since Since = func(t time.Time) time.Duration { From a2bee1fad349093ca6300ae34986904270fa40e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 Jan 2021 14:48:14 +0100 Subject: [PATCH 2/2] :fire: utils.Spinner --- pkg/cmd/issue/view/view_test.go | 8 -------- pkg/cmd/pr/view/view_test.go | 8 -------- utils/utils.go | 16 ---------------- 3 files changed, 32 deletions(-) diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index 82890eac9..44e4d20f1 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -8,7 +8,6 @@ import ( "os/exec" "testing" - "github.com/briandowns/spinner" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" @@ -16,7 +15,6 @@ import ( "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/test" - "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/stretchr/testify/assert" ) @@ -404,7 +402,6 @@ func TestIssueView_tty_Comments(t *testing.T) { } for name, tc := range tests { t.Run(name, func(t *testing.T) { - stubSpinner() http := &httpmock.Registry{} defer http.Verify(t) for name, file := range tc.fixtures { @@ -496,8 +493,3 @@ func TestIssueView_nontty_Comments(t *testing.T) { }) } } - -func stubSpinner() { - utils.StartSpinner = func(_ *spinner.Spinner) {} - utils.StopSpinner = func(_ *spinner.Spinner) {} -} diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go index e61ccd614..8f3e7ae04 100644 --- a/pkg/cmd/pr/view/view_test.go +++ b/pkg/cmd/pr/view/view_test.go @@ -9,7 +9,6 @@ import ( "strings" "testing" - "github.com/briandowns/spinner" "github.com/cli/cli/context" "github.com/cli/cli/git" "github.com/cli/cli/internal/config" @@ -19,7 +18,6 @@ import ( "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/test" - "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -777,7 +775,6 @@ func TestPRView_tty_Comments(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { - stubSpinner() http := &httpmock.Registry{} defer http.Verify(t) for name, file := range tt.fixtures { @@ -873,8 +870,3 @@ func TestPRView_nontty_Comments(t *testing.T) { }) } } - -func stubSpinner() { - utils.StartSpinner = func(_ *spinner.Spinner) {} - utils.StopSpinner = func(_ *spinner.Spinner) {} -} diff --git a/utils/utils.go b/utils/utils.go index 4b58508ef..57b39b5ba 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -2,12 +2,10 @@ package utils import ( "fmt" - "io" "net/url" "strings" "time" - "github.com/briandowns/spinner" "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/browser" ) @@ -88,20 +86,6 @@ func Humanize(s string) string { return strings.Map(h, s) } -// We do this so we can stub out the spinner in tests -- it made things really flakey. This is not -// an elegant solution. -var StartSpinner = func(s *spinner.Spinner) { - s.Start() -} - -var StopSpinner = func(s *spinner.Spinner) { - s.Stop() -} - -func Spinner(w io.Writer) *spinner.Spinner { - return spinner.New(spinner.CharSets[11], 400*time.Millisecond, spinner.WithWriter(w)) -} - func IsURL(s string) bool { return strings.HasPrefix(s, "http:/") || strings.HasPrefix(s, "https:/") }