Refactor autolink list and test to use http interface for simpler testing

This defines an AutolinkClient interface with a Get method used for
fetching the autolinks lists from the api. Then, the http client for
autolinks implements this interface with the AutolinkGetter struct.

This allows for dependency injection of the AutolinkGetter struct into the
listOptions, enabling mocking of the AutolinkGetter for testing. The
result of this is simpler tests that are easier to maintain, because the
interface for the table tests now allow for defining autolink structs as
the response instead of large mocked api calls.

This also allows for bespoke testing of the http file, which I'll follow
up with in the next commit.

pkg/cmd/repo/autolink/http.go

@@ -11,7 +11,17 @@ import (
 	"github.com/cli/cli/v2/internal/ghrepo"
 )
 
-func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink, error) {
+type AutolinkGetter struct {
+	HttpClient *http.Client
+}
+
+func NewAutolinkGetter(httpClient *http.Client) *AutolinkGetter {
+	return &AutolinkGetter{
+		HttpClient: httpClient,
+	}
+}
+
+func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) {
 	path := fmt.Sprintf("repos/%s/%s/autolinks", repo.RepoOwner(), repo.RepoName())
 	url := ghinstance.RESTPrefix(repo.RepoHost()) + path
 	req, err := http.NewRequest("GET", url, nil)
@@ -19,7 +29,7 @@ func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink,
 		return nil, err
 	}
 
-	resp, err := httpClient.Do(req)
+	resp, err := a.HttpClient.Do(req)
 	if err != nil {
 		return nil, err
 	}

pkg/cmd/repo/autolink/list.go

@@ -2,7 +2,6 @@ package autolink
 
 import (
 	"fmt"
-	"net/http"
 	"strconv"
 
 	"github.com/MakeNowJust/heredoc"
@@ -23,20 +22,23 @@ var autolinkFields = []string{
 }
 
 type listOptions struct {
-	BaseRepo   func() (ghrepo.Interface, error)
+	BaseRepo       func() (ghrepo.Interface, error)
-	Browser    browser.Browser
+	Browser        browser.Browser
-	HTTPClient func() (*http.Client, error)
+	AutolinkClient AutolinkClient
-	IO         *iostreams.IOStreams
+	IO             *iostreams.IOStreams
 
 	Exporter cmdutil.Exporter
 	WebMode  bool
 }
 
+type AutolinkClient interface {
+	Get(repo ghrepo.Interface) ([]autolink, error)
+}
+
 func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command {
 	opts := &listOptions{
-		Browser:    f.Browser,
+		Browser: f.Browser,
-		HTTPClient: f.HttpClient,
+		IO:      f.IOStreams,
-		IO:         f.IOStreams,
 	}
 
 	cmd := &cobra.Command{
@@ -52,6 +54,12 @@ func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman
 		RunE: func(cmd *cobra.Command, args []string) error {
 			opts.BaseRepo = f.BaseRepo
 
+			httpClient, err := f.HttpClient()
+			if err != nil {
+				return err
+			}
+			opts.AutolinkClient = NewAutolinkGetter(httpClient)
+
 			if runF != nil {
 				return runF(opts)
 			}
@@ -67,10 +75,6 @@ func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman
 }
 
 func listRun(opts *listOptions) error {
-	client, err := opts.HTTPClient()
-	if err != nil {
-		return err
-	}
 
 	repo, err := opts.BaseRepo()
 	if err != nil {
@@ -87,7 +91,7 @@ func listRun(opts *listOptions) error {
 		return opts.Browser.Browse(autolinksListURL)
 	}
 
-	autolinks, err := repoAutolinks(client, repo)
+	autolinks, err := opts.AutolinkClient.Get(repo)
 	if err != nil {
 		return err
 	}

pkg/cmd/repo/autolink/list_test.go

@@ -2,6 +2,7 @@ package autolink
 
 import (
 	"bytes"
+	"fmt"
 	"net/http"
 	"testing"
 
@@ -9,7 +10,6 @@ import (
 	"github.com/cli/cli/v2/internal/browser"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/cmdutil"
-	"github.com/cli/cli/v2/pkg/httpmock"
 	"github.com/cli/cli/v2/pkg/iostreams"
 	"github.com/google/shlex"
 	"github.com/stretchr/testify/assert"
@@ -61,6 +61,9 @@ func TestNewCmdList(t *testing.T) {
 			f := &cmdutil.Factory{
 				IOStreams: ios,
 			}
+			f.HttpClient = func() (*http.Client, error) {
+				return &http.Client{}, nil
+			}
 
 			argv, err := shlex.Split(tt.input)
 			require.NoError(t, err)
@@ -89,12 +92,28 @@ func TestNewCmdList(t *testing.T) {
 	}
 }
 
+type mockAutoLinkGetter struct {
+	Response []autolink
+}
+
+func (m *mockAutoLinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) {
+	return m.Response, nil
+}
+
+type mockAutoLinkGetterError struct {
+	err error
+}
+
+func (me *mockAutoLinkGetterError) Get(repo ghrepo.Interface) ([]autolink, error) {
+	return nil, me.err
+}
+
 func TestListRun(t *testing.T) {
 	tests := []struct {
 		name       string
 		opts       *listOptions
 		isTTY      bool
-		httpStubs  func(t *testing.T, reg *httpmock.Registry)
+		response   []autolink
 		wantStdout string
 		wantStderr string
 		wantErr    bool
@@ -103,24 +122,19 @@ func TestListRun(t *testing.T) {
 			name:  "list tty",
 			opts:  &listOptions{},
 			isTTY: true,
-			httpStubs: func(t *testing.T, reg *httpmock.Registry) {
+			response: []autolink{
-				reg.Register(
+				{
-					httpmock.REST("GET", "repos/OWNER/REPO/autolinks"),
+					ID:             1,
-					httpmock.StringResponse(`[
+					KeyPrefix:      "TICKET-",
-						{
+					URLTemplate:    "https://example.com/TICKET?query=<num>",
-							"id": 1,
+					IsAlphanumeric: true,
-							"key_prefix": "TICKET-",
+				},
-							"url_template": "https://example.com/TICKET?query=<num>",
+				{
-							"is_alphanumeric": true
+					ID:             2,
-						},
+					KeyPrefix:      "STORY-",
-						{
+					URLTemplate:    "https://example.com/STORY?id=<num>",
-							"id": 2,
+					IsAlphanumeric: false,
-							"key_prefix": "STORY-",
+				},
-							"url_template": "https://example.com/STORY?id=<num>",
-							"is_alphanumeric": false
-						}
-					]`),
-				)
 			},
 			wantStdout: heredoc.Doc(`
 
@@ -142,24 +156,19 @@ func TestListRun(t *testing.T) {
 				}(),
 			},
 			isTTY: true,
-			httpStubs: func(t *testing.T, reg *httpmock.Registry) {
+			response: []autolink{
-				reg.Register(
+				{
-					httpmock.REST("GET", "repos/OWNER/REPO/autolinks"),
+					ID:             1,
-					httpmock.StringResponse(`[
+					KeyPrefix:      "TICKET-",
-						{
+					URLTemplate:    "https://example.com/TICKET?query=<num>",
-							"id": 1,
+					IsAlphanumeric: true,
-							"key_prefix": "TICKET-",
+				},
-							"url_template": "https://example.com/TICKET?query=<num>",
+				{
-							"is_alphanumeric": true
+					ID:             2,
-						},
+					KeyPrefix:      "STORY-",
-						{
+					URLTemplate:    "https://example.com/STORY?id=<num>",
-							"id": 2,
+					IsAlphanumeric: false,
-							"key_prefix": "STORY-",
+				},
-							"url_template": "https://example.com/STORY?id=<num>",
-							"is_alphanumeric": false
-						}
-					]`),
-				)
 			},
 			wantStdout: "[{\"id\":1},{\"id\":2}]\n",
 			wantStderr: "",
@@ -168,24 +177,19 @@ func TestListRun(t *testing.T) {
 			name:  "list non-tty",
 			opts:  &listOptions{},
 			isTTY: false,
-			httpStubs: func(t *testing.T, reg *httpmock.Registry) {
+			response: []autolink{
-				reg.Register(
+				{
-					httpmock.REST("GET", "repos/OWNER/REPO/autolinks"),
+					ID:             1,
-					httpmock.StringResponse(`[
+					KeyPrefix:      "TICKET-",
-						{
+					URLTemplate:    "https://example.com/TICKET?query=<num>",
-							"id": 1,
+					IsAlphanumeric: true,
-							"key_prefix": "TICKET-",
+				},
-							"url_template": "https://example.com/TICKET?query=<num>",
+				{
-							"is_alphanumeric": true
+					ID:             2,
-						},
+					KeyPrefix:      "STORY-",
-						{
+					URLTemplate:    "https://example.com/STORY?id=<num>",
-							"id": 2,
+					IsAlphanumeric: false,
-							"key_prefix": "STORY-",
+				},
-							"url_template": "https://example.com/STORY?id=<num>",
-							"is_alphanumeric": false
-						}
-					]`),
-				)
 			},
 			wantStdout: heredoc.Doc(`
 				1	TICKET-	https://example.com/TICKET?query=<num>	true
@@ -195,15 +199,10 @@ func TestListRun(t *testing.T) {
 		},
 
 		{
-			name:  "no results",
+			name:       "no results",
-			opts:  &listOptions{},
+			opts:       &listOptions{},
-			isTTY: true,
+			isTTY:      true,
-			httpStubs: func(t *testing.T, reg *httpmock.Registry) {
+			response:   []autolink{},
-				reg.Register(
-					httpmock.REST("GET", "repos/OWNER/REPO/autolinks"),
-					httpmock.StringResponse(`[]`),
-				)
-			},
 			wantStdout: "",
 			wantStderr: "",
 			wantErr:    true,
@@ -223,13 +222,6 @@ func TestListRun(t *testing.T) {
 			ios.SetStdinTTY(tt.isTTY)
 			ios.SetStderrTTY(tt.isTTY)
 
-			reg := &httpmock.Registry{}
-			if tt.httpStubs != nil {
-				tt.httpStubs(t, reg)
-			}
-
-			defer reg.Verify(t)
-
 			opts := tt.opts
 			opts.IO = ios
 			opts.Browser = &browser.Stub{}
@@ -237,19 +229,19 @@ func TestListRun(t *testing.T) {
 			opts.IO = ios
 			opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }
 
-			opts.HTTPClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }
+			if tt.wantErr {
-
+				opts.AutolinkClient = &mockAutoLinkGetterError{err: fmt.Errorf("mock error")}
-			err := listRun(opts)
+				err := listRun(opts)
-			if (err != nil) != tt.wantErr {
+				require.Error(t, err)
-				t.Errorf("listRun() return error: %v", err)
+			} else {
-				return
+				opts.AutolinkClient = &mockAutoLinkGetter{Response: tt.response}
+				err := listRun(opts)
+				require.NoError(t, err)
+				assert.Equal(t, tt.wantStdout, stdout.String())
 			}
 
-			if stdout.String() != tt.wantStdout {
+			if tt.wantStderr != "" {
-				t.Errorf("wants stdout %q, got %q", tt.wantStdout, stdout.String())
+				assert.Equal(t, tt.wantStderr, stderr.String())
-			}
-			if stderr.String() != tt.wantStderr {
-				t.Errorf("wants stderr %q, got %q", tt.wantStderr, stderr.String())
 			}
 		})
 	}