From a390ce4f10677c8fd9d87ac49f08e1f05f29a709 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 15:08:58 -0800 Subject: [PATCH] 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 | 14 ++- pkg/cmd/repo/autolink/list.go | 30 +++--- pkg/cmd/repo/autolink/list_test.go | 158 ++++++++++++++--------------- 3 files changed, 104 insertions(+), 98 deletions(-) diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go index d51ac369c..13e8adebe 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/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 } diff --git a/pkg/cmd/repo/autolink/list.go b/pkg/cmd/repo/autolink/list.go index 45606bfa9..41d0d7266 100644 --- a/pkg/cmd/repo/autolink/list.go +++ b/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) - Browser browser.Browser - HTTPClient func() (*http.Client, error) - IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Browser browser.Browser + AutolinkClient AutolinkClient + 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, - HTTPClient: f.HttpClient, - IO: f.IOStreams, + Browser: f.Browser, + 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 } diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go index 2e3c21374..4a41f9ad2 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/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) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[ - { - "id": 1, - "key_prefix": "TICKET-", - "url_template": "https://example.com/TICKET?query=", - "is_alphanumeric": true - }, - { - "id": 2, - "key_prefix": "STORY-", - "url_template": "https://example.com/STORY?id=", - "is_alphanumeric": false - } - ]`), - ) + response: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, wantStdout: heredoc.Doc(` @@ -142,24 +156,19 @@ func TestListRun(t *testing.T) { }(), }, isTTY: true, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[ - { - "id": 1, - "key_prefix": "TICKET-", - "url_template": "https://example.com/TICKET?query=", - "is_alphanumeric": true - }, - { - "id": 2, - "key_prefix": "STORY-", - "url_template": "https://example.com/STORY?id=", - "is_alphanumeric": false - } - ]`), - ) + response: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: 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) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[ - { - "id": 1, - "key_prefix": "TICKET-", - "url_template": "https://example.com/TICKET?query=", - "is_alphanumeric": true - }, - { - "id": 2, - "key_prefix": "STORY-", - "url_template": "https://example.com/STORY?id=", - "is_alphanumeric": false - } - ]`), - ) + response: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, wantStdout: heredoc.Doc(` 1 TICKET- https://example.com/TICKET?query= true @@ -195,15 +199,10 @@ func TestListRun(t *testing.T) { }, { - name: "no results", - opts: &listOptions{}, - isTTY: true, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[]`), - ) - }, + name: "no results", + opts: &listOptions{}, + isTTY: true, + response: []autolink{}, 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 } - - err := listRun(opts) - if (err != nil) != tt.wantErr { - t.Errorf("listRun() return error: %v", err) - return + if tt.wantErr { + opts.AutolinkClient = &mockAutoLinkGetterError{err: fmt.Errorf("mock error")} + err := listRun(opts) + require.Error(t, err) + } else { + 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 { - t.Errorf("wants stdout %q, got %q", tt.wantStdout, stdout.String()) - } - if stderr.String() != tt.wantStderr { - t.Errorf("wants stderr %q, got %q", tt.wantStderr, stderr.String()) + if tt.wantStderr != "" { + assert.Equal(t, tt.wantStderr, stderr.String()) } }) }