From 23c16c9c4cb033840065e0c1d08dbdf6cce7ca3f Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 20 Dec 2024 13:06:47 -0500 Subject: [PATCH 01/15] Introduce repo autolinks list commands --- pkg/cmd/repo/autolink/autolink.go | 19 +++ pkg/cmd/repo/autolink/http.go | 44 +++++ pkg/cmd/repo/autolink/list.go | 123 ++++++++++++++ pkg/cmd/repo/autolink/list_test.go | 256 +++++++++++++++++++++++++++++ pkg/cmd/repo/autolink/shared.go | 14 ++ pkg/cmd/repo/repo.go | 3 + 6 files changed, 459 insertions(+) create mode 100644 pkg/cmd/repo/autolink/autolink.go create mode 100644 pkg/cmd/repo/autolink/http.go create mode 100644 pkg/cmd/repo/autolink/list.go create mode 100644 pkg/cmd/repo/autolink/list_test.go create mode 100644 pkg/cmd/repo/autolink/shared.go diff --git a/pkg/cmd/repo/autolink/autolink.go b/pkg/cmd/repo/autolink/autolink.go new file mode 100644 index 000000000..1c3c16d44 --- /dev/null +++ b/pkg/cmd/repo/autolink/autolink.go @@ -0,0 +1,19 @@ +package autolink + +import ( + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" +) + +func NewCmdAutolink(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "autolink ", + Short: "Manage autolink references", + Long: "Work with GitHub autolink references.", + } + cmdutil.EnableRepoOverride(cmd, f) + + cmd.AddCommand(newCmdList(f, nil)) + + return cmd +} diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go new file mode 100644 index 000000000..b8316a336 --- /dev/null +++ b/pkg/cmd/repo/autolink/http.go @@ -0,0 +1,44 @@ +package autolink + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/internal/ghrepo" +) + +func repoAutolinks(httpClient *http.Client, 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) + if err != nil { + return nil, err + } + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return nil, api.HandleHTTPError(resp) + } + + b, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + var autolinks []autolink + err = json.Unmarshal(b, &autolinks) + if err != nil { + return nil, err + } + + return autolinks, nil +} diff --git a/pkg/cmd/repo/autolink/list.go b/pkg/cmd/repo/autolink/list.go new file mode 100644 index 000000000..45606bfa9 --- /dev/null +++ b/pkg/cmd/repo/autolink/list.go @@ -0,0 +1,123 @@ +package autolink + +import ( + "fmt" + "net/http" + "strconv" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/browser" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/tableprinter" + "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +var autolinkFields = []string{ + "id", + "isAlphanumeric", + "keyPrefix", + "urlTemplate", +} + +type listOptions struct { + BaseRepo func() (ghrepo.Interface, error) + Browser browser.Browser + HTTPClient func() (*http.Client, error) + IO *iostreams.IOStreams + + Exporter cmdutil.Exporter + WebMode bool +} + +func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { + opts := &listOptions{ + Browser: f.Browser, + HTTPClient: f.HttpClient, + IO: f.IOStreams, + } + + cmd := &cobra.Command{ + Use: "list", + Short: "List autolink references for a GitHub repository", + Long: heredoc.Doc(` + Gets all autolink references that are configured for a repository. + + Information about autolinks is only available to repository administrators. + `), + Aliases: []string{"ls"}, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + opts.BaseRepo = f.BaseRepo + + if runF != nil { + return runF(opts) + } + + return listRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "List autolink references in the web browser") + cmdutil.AddJSONFlags(cmd, &opts.Exporter, autolinkFields) + + return cmd +} + +func listRun(opts *listOptions) error { + client, err := opts.HTTPClient() + if err != nil { + return err + } + + repo, err := opts.BaseRepo() + if err != nil { + return err + } + + if opts.WebMode { + autolinksListURL := ghrepo.GenerateRepoURL(repo, "settings/key_links") + + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(autolinksListURL)) + } + + return opts.Browser.Browse(autolinksListURL) + } + + autolinks, err := repoAutolinks(client, repo) + if err != nil { + return err + } + + if len(autolinks) == 0 { + return cmdutil.NewNoResultsError(fmt.Sprintf("no autolinks found in %s", ghrepo.FullName(repo))) + } + + if opts.Exporter != nil { + return opts.Exporter.Write(opts.IO, autolinks) + } + + if opts.IO.IsStdoutTTY() { + title := listHeader(ghrepo.FullName(repo), len(autolinks)) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) + } + + tp := tableprinter.New(opts.IO, tableprinter.WithHeader("ID", "KEY PREFIX", "URL TEMPLATE", "ALPHANUMERIC")) + + for _, autolink := range autolinks { + tp.AddField(fmt.Sprintf("%d", autolink.ID)) + tp.AddField(autolink.KeyPrefix) + tp.AddField(autolink.URLTemplate) + tp.AddField(strconv.FormatBool(autolink.IsAlphanumeric)) + tp.EndRow() + } + + return tp.Render() +} + +func listHeader(repoName string, count int) string { + return fmt.Sprintf("Showing %s in %s", text.Pluralize(count, "autolink reference"), repoName) +} diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go new file mode 100644 index 000000000..2e3c21374 --- /dev/null +++ b/pkg/cmd/repo/autolink/list_test.go @@ -0,0 +1,256 @@ +package autolink + +import ( + "bytes" + "net/http" + "testing" + + "github.com/MakeNowJust/heredoc" + "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" + "github.com/stretchr/testify/require" +) + +func TestNewCmdList(t *testing.T) { + tests := []struct { + name string + input string + output listOptions + wantErr bool + wantExporter bool + errMsg string + }{ + { + name: "no argument", + input: "", + output: listOptions{}, + }, + { + name: "web flag", + input: "--web", + output: listOptions{WebMode: true}, + }, + { + name: "json flag", + input: "--json id", + output: listOptions{}, + wantExporter: true, + }, + { + name: "invalid json flag", + input: "--json invalid", + wantErr: true, + errMsg: heredoc.Doc(` + Unknown JSON field: "invalid" + Available fields: + id + isAlphanumeric + keyPrefix + urlTemplate`), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + } + + argv, err := shlex.Split(tt.input) + require.NoError(t, err) + + var gotOpts *listOptions + cmd := newCmdList(f, func(opts *listOptions) error { + gotOpts = opts + return nil + }) + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantErr { + require.EqualError(t, err, tt.errMsg) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) + assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + }) + } +} + +func TestListRun(t *testing.T) { + tests := []struct { + name string + opts *listOptions + isTTY bool + httpStubs func(t *testing.T, reg *httpmock.Registry) + wantStdout string + wantStderr string + wantErr bool + }{ + { + 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 + } + ]`), + ) + }, + wantStdout: heredoc.Doc(` + + Showing 2 autolink references in OWNER/REPO + + ID KEY PREFIX URL TEMPLATE ALPHANUMERIC + 1 TICKET- https://example.com/TICKET?query= true + 2 STORY- https://example.com/STORY?id= false + `), + wantStderr: "", + }, + { + name: "list json", + opts: &listOptions{ + Exporter: func() cmdutil.Exporter { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields([]string{"id"}) + return exporter + }(), + }, + 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 + } + ]`), + ) + }, + wantStdout: "[{\"id\":1},{\"id\":2}]\n", + wantStderr: "", + }, + { + 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 + } + ]`), + ) + }, + wantStdout: heredoc.Doc(` + 1 TICKET- https://example.com/TICKET?query= true + 2 STORY- https://example.com/STORY?id= false + `), + wantStderr: "", + }, + + { + 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(`[]`), + ) + }, + wantStdout: "", + wantStderr: "", + wantErr: true, + }, + { + name: "web mode", + isTTY: true, + opts: &listOptions{WebMode: true}, + wantStderr: "Opening https://github.com/OWNER/REPO/settings/key_links in your browser.\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(tt.isTTY) + 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{} + + 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 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()) + } + }) + } +} diff --git a/pkg/cmd/repo/autolink/shared.go b/pkg/cmd/repo/autolink/shared.go new file mode 100644 index 000000000..c8478b346 --- /dev/null +++ b/pkg/cmd/repo/autolink/shared.go @@ -0,0 +1,14 @@ +package autolink + +import "github.com/cli/cli/v2/pkg/cmdutil" + +type autolink struct { + ID int `json:"id"` + IsAlphanumeric bool `json:"is_alphanumeric"` + KeyPrefix string `json:"key_prefix"` + URLTemplate string `json:"url_template"` +} + +func (s *autolink) ExportData(fields []string) map[string]interface{} { + return cmdutil.StructExportData(s, fields) +} diff --git a/pkg/cmd/repo/repo.go b/pkg/cmd/repo/repo.go index 14a4bf49c..687cf5a8a 100644 --- a/pkg/cmd/repo/repo.go +++ b/pkg/cmd/repo/repo.go @@ -3,6 +3,7 @@ package repo import ( "github.com/MakeNowJust/heredoc" repoArchiveCmd "github.com/cli/cli/v2/pkg/cmd/repo/archive" + repoAutolinkCmd "github.com/cli/cli/v2/pkg/cmd/repo/autolink" repoCloneCmd "github.com/cli/cli/v2/pkg/cmd/repo/clone" repoCreateCmd "github.com/cli/cli/v2/pkg/cmd/repo/create" creditsCmd "github.com/cli/cli/v2/pkg/cmd/repo/credits" @@ -19,6 +20,7 @@ import ( repoSyncCmd "github.com/cli/cli/v2/pkg/cmd/repo/sync" repoUnarchiveCmd "github.com/cli/cli/v2/pkg/cmd/repo/unarchive" repoViewCmd "github.com/cli/cli/v2/pkg/cmd/repo/view" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -64,6 +66,7 @@ func NewCmdRepo(f *cmdutil.Factory) *cobra.Command { repoDeleteCmd.NewCmdDelete(f, nil), creditsCmd.NewCmdRepoCredits(f, nil), gardenCmd.NewCmdGarden(f, nil), + repoAutolinkCmd.NewCmdAutolink(f), ) return cmd From 5fb98524e0d864dd36e6c6dd6fe25e5568f7fe3a Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 14:34:21 -0800 Subject: [PATCH 02/15] Apply PR comment changes --- pkg/cmd/repo/autolink/autolink.go | 11 ++++++++++- pkg/cmd/repo/autolink/http.go | 4 +++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/repo/autolink/autolink.go b/pkg/cmd/repo/autolink/autolink.go index 1c3c16d44..9c0972f7c 100644 --- a/pkg/cmd/repo/autolink/autolink.go +++ b/pkg/cmd/repo/autolink/autolink.go @@ -1,6 +1,7 @@ package autolink import ( + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -9,7 +10,15 @@ func NewCmdAutolink(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "autolink ", Short: "Manage autolink references", - Long: "Work with GitHub autolink references.", + Long: heredoc.Docf(` + Work with GitHub autolink references. + + GitHub autolinks require admin access to configure and can be found at + https://github.com/{owner}/{repo}/settings/key_links. + Use %[1]sgh repo autolink list --web%[1]s to open this page for the current repository. + + For more information about GitHub autolinks, see https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/configuring-autolinks-to-reference-external-resources + `, "`"), } cmdutil.EnableRepoOverride(cmd, f) diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go index b8316a336..d51ac369c 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/http.go @@ -25,7 +25,9 @@ func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink, } defer resp.Body.Close() - if resp.StatusCode > 299 { + if resp.StatusCode == 404 { + return nil, fmt.Errorf("error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/%s)", path) + } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } From a390ce4f10677c8fd9d87ac49f08e1f05f29a709 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 15:08:58 -0800 Subject: [PATCH 03/15] 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()) } }) } From 4a74cc8856f3c0178856c2b5ec7c6afd3df3bc86 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 15:43:02 -0800 Subject: [PATCH 04/15] Add testing for AutoLinkGetter This adds the missing mocked http tests to the http_test.go file. These tests were previously bundled with the tests in list_test.go, creating a testing pattern that was difficult to understand and maintain. The refactor in the previous commit replaced these tests with the AutolinkClient interface, allowing for the httpmocks to be isolated to the AutolinkGetter that implements that interface. --- pkg/cmd/repo/autolink/http.go | 2 +- pkg/cmd/repo/autolink/http_test.go | 79 ++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 pkg/cmd/repo/autolink/http_test.go diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go index 13e8adebe..a10e98f7b 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/http.go @@ -36,7 +36,7 @@ func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { defer resp.Body.Close() if resp.StatusCode == 404 { - return nil, fmt.Errorf("error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/%s)", path) + return nil, fmt.Errorf("error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/%s)", path) } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/repo/autolink/http_test.go b/pkg/cmd/repo/autolink/http_test.go new file mode 100644 index 000000000..64ffc1e40 --- /dev/null +++ b/pkg/cmd/repo/autolink/http_test.go @@ -0,0 +1,79 @@ +package autolink + +import ( + "fmt" + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewAutolinkGetter(t *testing.T) { + httpClient := &http.Client{} + autolinkGetter := NewAutolinkGetter(httpClient) + assert.NotNil(t, autolinkGetter) +} + +func TestAutoLinkGetter_Get(t *testing.T) { + tests := []struct { + name string + repo ghrepo.Interface + resp []autolink + status int + }{ + { + name: "no autolinks", + repo: ghrepo.New("OWNER", "REPO"), + resp: []autolink{}, + status: 200, + }, + { + name: "two autolinks", + repo: ghrepo.New("OWNER", "REPO"), + resp: []autolink{ + { + ID: 1, + IsAlphanumeric: true, + KeyPrefix: "key", + URLTemplate: "https://example.com", + }, + { + ID: 2, + IsAlphanumeric: false, + KeyPrefix: "key2", + URLTemplate: "https://example2.com", + }, + }, + status: 200, + }, + { + name: "http error", + repo: ghrepo.New("OWNER", "REPO"), + status: 404, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register( + httpmock.REST("GET", fmt.Sprintf("repos/%s/%s/autolinks", tt.repo.RepoOwner(), tt.repo.RepoName())), + httpmock.StatusJSONResponse(tt.status, tt.resp), + ) + defer reg.Verify(t) + + autolinkGetter := NewAutolinkGetter(&http.Client{Transport: reg}) + autolinks, err := autolinkGetter.Get(tt.repo) + if tt.status == 404 { + require.Error(t, err) + assert.Equal(t, "error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) + } else { + require.NoError(t, err) + assert.Equal(t, tt.resp, autolinks) + } + }) + } +} From 869d25193a39e4644311b97bcb6a5a1b33badec0 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Mon, 23 Dec 2024 20:26:37 -0500 Subject: [PATCH 05/15] Refactor out early return in test code Co-authored-by: Tyler McGoffin --- pkg/cmd/repo/autolink/list_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go index 4a41f9ad2..4b759aaf7 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list_test.go @@ -82,12 +82,11 @@ func TestNewCmdList(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantErr { require.EqualError(t, err, tt.errMsg) - return - } - - require.NoError(t, err) - assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) - assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + } else { + require.NoError(t, err) + assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) + assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + } }) } } From ea04d2da30767dbd08e4d94f6f47b420f471fbba Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Mon, 23 Dec 2024 20:27:11 -0500 Subject: [PATCH 06/15] Whitespace --- pkg/cmd/repo/autolink/list_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go index 4b759aaf7..0c24c5ba6 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list_test.go @@ -83,10 +83,10 @@ func TestNewCmdList(t *testing.T) { if tt.wantErr { require.EqualError(t, err, tt.errMsg) } else { - require.NoError(t, err) - assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) - assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) - } + require.NoError(t, err) + assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) + assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + } }) } } From e98ff2ea387b7d18e2b94f124a0d61c46417f565 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Mon, 23 Dec 2024 21:09:10 -0500 Subject: [PATCH 07/15] Refactor autolink subcommands into their own packages --- pkg/cmd/repo/autolink/autolink.go | 3 ++- pkg/cmd/repo/autolink/{ => list}/http.go | 2 +- pkg/cmd/repo/autolink/{ => list}/http_test.go | 2 +- pkg/cmd/repo/autolink/{ => list}/list.go | 15 +++++++++++++-- pkg/cmd/repo/autolink/{ => list}/list_test.go | 4 ++-- pkg/cmd/repo/autolink/shared.go | 14 -------------- 6 files changed, 19 insertions(+), 21 deletions(-) rename pkg/cmd/repo/autolink/{ => list}/http.go (98%) rename pkg/cmd/repo/autolink/{ => list}/http_test.go (99%) rename pkg/cmd/repo/autolink/{ => list}/list.go (88%) rename pkg/cmd/repo/autolink/{ => list}/list_test.go (98%) delete mode 100644 pkg/cmd/repo/autolink/shared.go diff --git a/pkg/cmd/repo/autolink/autolink.go b/pkg/cmd/repo/autolink/autolink.go index 9c0972f7c..d9430f562 100644 --- a/pkg/cmd/repo/autolink/autolink.go +++ b/pkg/cmd/repo/autolink/autolink.go @@ -2,6 +2,7 @@ package autolink import ( "github.com/MakeNowJust/heredoc" + cmdList "github.com/cli/cli/v2/pkg/cmd/repo/autolink/list" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -22,7 +23,7 @@ func NewCmdAutolink(f *cmdutil.Factory) *cobra.Command { } cmdutil.EnableRepoOverride(cmd, f) - cmd.AddCommand(newCmdList(f, nil)) + cmd.AddCommand(cmdList.NewCmdList(f, nil)) return cmd } diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/list/http.go similarity index 98% rename from pkg/cmd/repo/autolink/http.go rename to pkg/cmd/repo/autolink/list/http.go index a10e98f7b..e3fb27e7b 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "encoding/json" diff --git a/pkg/cmd/repo/autolink/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go similarity index 99% rename from pkg/cmd/repo/autolink/http_test.go rename to pkg/cmd/repo/autolink/list/http_test.go index 64ffc1e40..8a2d5c296 100644 --- a/pkg/cmd/repo/autolink/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "fmt" diff --git a/pkg/cmd/repo/autolink/list.go b/pkg/cmd/repo/autolink/list/list.go similarity index 88% rename from pkg/cmd/repo/autolink/list.go rename to pkg/cmd/repo/autolink/list/list.go index 41d0d7266..9db044668 100644 --- a/pkg/cmd/repo/autolink/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "fmt" @@ -21,6 +21,17 @@ var autolinkFields = []string{ "urlTemplate", } +type autolink struct { + ID int `json:"id"` + IsAlphanumeric bool `json:"is_alphanumeric"` + KeyPrefix string `json:"key_prefix"` + URLTemplate string `json:"url_template"` +} + +func (s *autolink) ExportData(fields []string) map[string]interface{} { + return cmdutil.StructExportData(s, fields) +} + type listOptions struct { BaseRepo func() (ghrepo.Interface, error) Browser browser.Browser @@ -35,7 +46,7 @@ type AutolinkClient interface { Get(repo ghrepo.Interface) ([]autolink, error) } -func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { +func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { opts := &listOptions{ Browser: f.Browser, IO: f.IOStreams, diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go similarity index 98% rename from pkg/cmd/repo/autolink/list_test.go rename to pkg/cmd/repo/autolink/list/list_test.go index 0c24c5ba6..5db767032 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "bytes" @@ -69,7 +69,7 @@ func TestNewCmdList(t *testing.T) { require.NoError(t, err) var gotOpts *listOptions - cmd := newCmdList(f, func(opts *listOptions) error { + cmd := NewCmdList(f, func(opts *listOptions) error { gotOpts = opts return nil }) diff --git a/pkg/cmd/repo/autolink/shared.go b/pkg/cmd/repo/autolink/shared.go deleted file mode 100644 index c8478b346..000000000 --- a/pkg/cmd/repo/autolink/shared.go +++ /dev/null @@ -1,14 +0,0 @@ -package autolink - -import "github.com/cli/cli/v2/pkg/cmdutil" - -type autolink struct { - ID int `json:"id"` - IsAlphanumeric bool `json:"is_alphanumeric"` - KeyPrefix string `json:"key_prefix"` - URLTemplate string `json:"url_template"` -} - -func (s *autolink) ExportData(fields []string) map[string]interface{} { - return cmdutil.StructExportData(s, fields) -} From 67266e9cb883629a0cbdd21b630497b318c8c6ad Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 21:40:52 -0500 Subject: [PATCH 08/15] PR nits --- pkg/cmd/repo/autolink/list/http.go | 4 ++-- pkg/cmd/repo/autolink/list/http_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index e3fb27e7b..269775883 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -35,8 +35,8 @@ func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { } defer resp.Body.Close() - if resp.StatusCode == 404 { - return nil, fmt.Errorf("error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/%s)", path) + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/%s)", path) } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index 8a2d5c296..77923ed11 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -69,7 +69,7 @@ func TestAutoLinkGetter_Get(t *testing.T) { autolinks, err := autolinkGetter.Get(tt.repo) if tt.status == 404 { require.Error(t, err) - assert.Equal(t, "error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) + assert.Equal(t, "error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) } else { require.NoError(t, err) assert.Equal(t, tt.resp, autolinks) From cc2428983228b4ae4f1fd522a13cbe0cd891cc01 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 21:43:47 -0500 Subject: [PATCH 09/15] Break out autolink list json fields test --- pkg/cmd/repo/autolink/list/list_test.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index 5db767032..0d9562d6d 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -11,11 +11,21 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/jsonfieldstest" "github.com/google/shlex" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestJSONFields(t *testing.T) { + jsonfieldstest.ExpectCommandToSupportJSONFields(t, NewCmdList, []string{ + "id", + "isAlphanumeric", + "keyPrefix", + "urlTemplate", + }) +} + func TestNewCmdList(t *testing.T) { tests := []struct { name string @@ -41,18 +51,6 @@ func TestNewCmdList(t *testing.T) { output: listOptions{}, wantExporter: true, }, - { - name: "invalid json flag", - input: "--json invalid", - wantErr: true, - errMsg: heredoc.Doc(` - Unknown JSON field: "invalid" - Available fields: - id - isAlphanumeric - keyPrefix - urlTemplate`), - }, } for _, tt := range tests { From dc6320f7f7e274f44de8e458e1eb808f851240c9 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 21:47:27 -0500 Subject: [PATCH 10/15] Remove NewAutolinkClient --- pkg/cmd/repo/autolink/list/http.go | 10 ++-------- pkg/cmd/repo/autolink/list/http_test.go | 6 ++++-- pkg/cmd/repo/autolink/list/list.go | 2 +- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index 269775883..8ce2ba166 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -12,13 +12,7 @@ import ( ) type AutolinkGetter struct { - HttpClient *http.Client -} - -func NewAutolinkGetter(httpClient *http.Client) *AutolinkGetter { - return &AutolinkGetter{ - HttpClient: httpClient, - } + HTTPClient *http.Client } func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { @@ -29,7 +23,7 @@ func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { return nil, err } - resp, err := a.HttpClient.Do(req) + resp, err := a.HTTPClient.Do(req) if err != nil { return nil, err } diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index 77923ed11..f75d7e473 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -13,7 +13,7 @@ import ( func TestNewAutolinkGetter(t *testing.T) { httpClient := &http.Client{} - autolinkGetter := NewAutolinkGetter(httpClient) + autolinkGetter := &AutolinkGetter{HTTPClient: httpClient} assert.NotNil(t, autolinkGetter) } @@ -65,7 +65,9 @@ func TestAutoLinkGetter_Get(t *testing.T) { ) defer reg.Verify(t) - autolinkGetter := NewAutolinkGetter(&http.Client{Transport: reg}) + autolinkGetter := &AutolinkGetter{ + HTTPClient: &http.Client{Transport: reg}, + } autolinks, err := autolinkGetter.Get(tt.repo) if tt.status == 404 { require.Error(t, err) diff --git a/pkg/cmd/repo/autolink/list/list.go b/pkg/cmd/repo/autolink/list/list.go index 9db044668..90c652044 100644 --- a/pkg/cmd/repo/autolink/list/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -69,7 +69,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman if err != nil { return err } - opts.AutolinkClient = NewAutolinkGetter(httpClient) + opts.AutolinkClient = &AutolinkGetter{HTTPClient: httpClient} if runF != nil { return runF(opts) From 63488a1a06d1426c0146c14f0d67ccefdc32a982 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 22:02:19 -0500 Subject: [PATCH 11/15] Use 'list' instead of 'get' for autolink list type and method --- pkg/cmd/repo/autolink/list/http.go | 4 ++-- pkg/cmd/repo/autolink/list/http_test.go | 6 +++--- pkg/cmd/repo/autolink/list/list.go | 6 +++--- pkg/cmd/repo/autolink/list/list_test.go | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index 8ce2ba166..b5de22140 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -11,11 +11,11 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" ) -type AutolinkGetter struct { +type AutolinkLister struct { HTTPClient *http.Client } -func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { +func (a *AutolinkLister) List(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) diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index f75d7e473..eec74cf38 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -13,7 +13,7 @@ import ( func TestNewAutolinkGetter(t *testing.T) { httpClient := &http.Client{} - autolinkGetter := &AutolinkGetter{HTTPClient: httpClient} + autolinkGetter := &AutolinkLister{HTTPClient: httpClient} assert.NotNil(t, autolinkGetter) } @@ -65,10 +65,10 @@ func TestAutoLinkGetter_Get(t *testing.T) { ) defer reg.Verify(t) - autolinkGetter := &AutolinkGetter{ + autolinkGetter := &AutolinkLister{ HTTPClient: &http.Client{Transport: reg}, } - autolinks, err := autolinkGetter.Get(tt.repo) + autolinks, err := autolinkGetter.List(tt.repo) if tt.status == 404 { require.Error(t, err) assert.Equal(t, "error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) diff --git a/pkg/cmd/repo/autolink/list/list.go b/pkg/cmd/repo/autolink/list/list.go index 90c652044..6f1240605 100644 --- a/pkg/cmd/repo/autolink/list/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -43,7 +43,7 @@ type listOptions struct { } type AutolinkClient interface { - Get(repo ghrepo.Interface) ([]autolink, error) + List(repo ghrepo.Interface) ([]autolink, error) } func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { @@ -69,7 +69,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman if err != nil { return err } - opts.AutolinkClient = &AutolinkGetter{HTTPClient: httpClient} + opts.AutolinkClient = &AutolinkLister{HTTPClient: httpClient} if runF != nil { return runF(opts) @@ -102,7 +102,7 @@ func listRun(opts *listOptions) error { return opts.Browser.Browse(autolinksListURL) } - autolinks, err := opts.AutolinkClient.Get(repo) + autolinks, err := opts.AutolinkClient.List(repo) if err != nil { return err } diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index 0d9562d6d..d6ed6c9cf 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -93,7 +93,7 @@ type mockAutoLinkGetter struct { Response []autolink } -func (m *mockAutoLinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { +func (m *mockAutoLinkGetter) List(repo ghrepo.Interface) ([]autolink, error) { return m.Response, nil } @@ -101,7 +101,7 @@ type mockAutoLinkGetterError struct { err error } -func (me *mockAutoLinkGetterError) Get(repo ghrepo.Interface) ([]autolink, error) { +func (me *mockAutoLinkGetterError) List(repo ghrepo.Interface) ([]autolink, error) { return nil, me.err } From 20f086549adff9b437c2de5ab118010da0af5dd2 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 22:03:25 -0500 Subject: [PATCH 12/15] Decode instead of unmarshal --- pkg/cmd/repo/autolink/list/http.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index b5de22140..70d913d70 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -3,7 +3,6 @@ package list import ( "encoding/json" "fmt" - "io" "net/http" "github.com/cli/cli/v2/api" @@ -34,14 +33,8 @@ func (a *AutolinkLister) List(repo ghrepo.Interface) ([]autolink, error) { } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var autolinks []autolink - err = json.Unmarshal(b, &autolinks) + err = json.NewDecoder(resp.Body).Decode(&autolinks) if err != nil { return nil, err } From da826db34227bb8aeb8d7e273a9abafcb6e7e748 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 22:58:12 -0500 Subject: [PATCH 13/15] Better error testing for autolink TestListRun --- pkg/cmd/repo/autolink/list/list.go | 1 - pkg/cmd/repo/autolink/list/list_test.go | 170 ++++++++++++++---------- 2 files changed, 97 insertions(+), 74 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/list.go b/pkg/cmd/repo/autolink/list/list.go index 6f1240605..d8a9c9f12 100644 --- a/pkg/cmd/repo/autolink/list/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -86,7 +86,6 @@ func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman } func listRun(opts *listOptions) error { - repo, err := opts.BaseRepo() if err != nil { return err diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index d6ed6c9cf..e6d0603ef 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -2,7 +2,6 @@ package list import ( "bytes" - "fmt" "net/http" "testing" @@ -51,6 +50,13 @@ func TestNewCmdList(t *testing.T) { output: listOptions{}, wantExporter: true, }, + { + name: "invalid json flag", + input: "--json invalid", + output: listOptions{}, + wantErr: true, + errMsg: "Unknown JSON field: \"invalid\"\nAvailable fields:\n id\n isAlphanumeric\n keyPrefix\n urlTemplate", + }, } for _, tt := range tests { @@ -89,48 +95,34 @@ func TestNewCmdList(t *testing.T) { } } -type mockAutoLinkGetter struct { - Response []autolink -} - -func (m *mockAutoLinkGetter) List(repo ghrepo.Interface) ([]autolink, error) { - return m.Response, nil -} - -type mockAutoLinkGetterError struct { - err error -} - -func (me *mockAutoLinkGetterError) List(repo ghrepo.Interface) ([]autolink, error) { - return nil, me.err -} - func TestListRun(t *testing.T) { tests := []struct { - name string - opts *listOptions - isTTY bool - response []autolink - wantStdout string - wantStderr string - wantErr bool + name string + opts *listOptions + isTTY bool + stubLister stubAutoLinkLister + expectedErr error + wantStdout string + wantStderr string }{ { name: "list tty", opts: &listOptions{}, isTTY: true, - 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, + stubLister: stubAutoLinkLister{ + autolinks: []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(` @@ -153,18 +145,20 @@ func TestListRun(t *testing.T) { }(), }, isTTY: true, - 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, + stubLister: stubAutoLinkLister{ + autolinks: []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", @@ -174,18 +168,20 @@ func TestListRun(t *testing.T) { name: "list non-tty", opts: &listOptions{}, isTTY: 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, + stubLister: stubAutoLinkLister{ + autolinks: []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(` @@ -194,15 +190,26 @@ func TestListRun(t *testing.T) { `), wantStderr: "", }, - { - name: "no results", - opts: &listOptions{}, - isTTY: true, - response: []autolink{}, - wantStdout: "", - wantStderr: "", - wantErr: true, + name: "no results", + opts: &listOptions{}, + isTTY: true, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{}, + }, + expectedErr: cmdutil.NewNoResultsError("no autolinks found in OWNER/REPO"), + wantStderr: "", + }, + { + name: "client error", + opts: &listOptions{}, + isTTY: true, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{}, + err: testAutolinkClientListError{}, + }, + expectedErr: testAutolinkClientListError{}, + wantStderr: "", }, { name: "web mode", @@ -226,13 +233,13 @@ func TestListRun(t *testing.T) { opts.IO = ios opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } - if tt.wantErr { - opts.AutolinkClient = &mockAutoLinkGetterError{err: fmt.Errorf("mock error")} - err := listRun(opts) + opts.AutolinkClient = &tt.stubLister + err := listRun(opts) + + if tt.expectedErr != nil { require.Error(t, err) + require.ErrorIs(t, err, tt.expectedErr) } else { - opts.AutolinkClient = &mockAutoLinkGetter{Response: tt.response} - err := listRun(opts) require.NoError(t, err) assert.Equal(t, tt.wantStdout, stdout.String()) } @@ -243,3 +250,20 @@ func TestListRun(t *testing.T) { }) } } + +type ( + testAutolinkClientListError struct{} + + stubAutoLinkLister struct { + autolinks []autolink + err error + } +) + +func (g stubAutoLinkLister) List(repo ghrepo.Interface) ([]autolink, error) { + return g.autolinks, g.err +} + +func (e testAutolinkClientListError) Error() string { + return "autolink client list error" +} From fa254ba205bd8253178ffec50f89ad405aa969d7 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Sat, 28 Dec 2024 07:51:47 -0500 Subject: [PATCH 14/15] Complete get -> list renaming --- pkg/cmd/repo/autolink/list/http_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index eec74cf38..fc1e44b23 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -11,13 +11,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestNewAutolinkGetter(t *testing.T) { - httpClient := &http.Client{} - autolinkGetter := &AutolinkLister{HTTPClient: httpClient} - assert.NotNil(t, autolinkGetter) -} - -func TestAutoLinkGetter_Get(t *testing.T) { +func TestAutoLinkLister_List(t *testing.T) { tests := []struct { name string repo ghrepo.Interface @@ -65,10 +59,10 @@ func TestAutoLinkGetter_Get(t *testing.T) { ) defer reg.Verify(t) - autolinkGetter := &AutolinkLister{ + autolinkLister := &AutolinkLister{ HTTPClient: &http.Client{Transport: reg}, } - autolinks, err := autolinkGetter.List(tt.repo) + autolinks, err := autolinkLister.List(tt.repo) if tt.status == 404 { require.Error(t, err) assert.Equal(t, "error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) From a5cf3751cde5278fa76a5ff6c4d2a38a97a21dd4 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Thu, 2 Jan 2025 13:14:23 -0500 Subject: [PATCH 15/15] Separate type decrarations --- pkg/cmd/repo/autolink/list/list_test.go | 32 ++++++++++++------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index e6d0603ef..3fc8e0261 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -95,6 +95,21 @@ func TestNewCmdList(t *testing.T) { } } +type stubAutoLinkLister struct { + autolinks []autolink + err error +} + +func (g stubAutoLinkLister) List(repo ghrepo.Interface) ([]autolink, error) { + return g.autolinks, g.err +} + +type testAutolinkClientListError struct{} + +func (e testAutolinkClientListError) Error() string { + return "autolink client list error" +} + func TestListRun(t *testing.T) { tests := []struct { name string @@ -250,20 +265,3 @@ func TestListRun(t *testing.T) { }) } } - -type ( - testAutolinkClientListError struct{} - - stubAutoLinkLister struct { - autolinks []autolink - err error - } -) - -func (g stubAutoLinkLister) List(repo ghrepo.Interface) ([]autolink, error) { - return g.autolinks, g.err -} - -func (e testAutolinkClientListError) Error() string { - return "autolink client list error" -}