From 8b615ec2e69f6dff869a89e0a931605018c23587 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 16 Apr 2025 15:40:58 +0200 Subject: [PATCH] Issue close early arg parsing --- pkg/cmd/issue/argparsetest/argparsetest.go | 129 +++++++++++++++++++++ pkg/cmd/issue/close/close.go | 29 +++-- pkg/cmd/issue/close/close_test.go | 67 +++++------ pkg/cmd/issue/shared/lookup.go | 10 +- 4 files changed, 187 insertions(+), 48 deletions(-) create mode 100644 pkg/cmd/issue/argparsetest/argparsetest.go diff --git a/pkg/cmd/issue/argparsetest/argparsetest.go b/pkg/cmd/issue/argparsetest/argparsetest.go new file mode 100644 index 000000000..1433e6104 --- /dev/null +++ b/pkg/cmd/issue/argparsetest/argparsetest.go @@ -0,0 +1,129 @@ +package argparsetest + +import ( + "bytes" + "reflect" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/google/shlex" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newCmdFunc represents the typical function signature we use for creating commands e.g. `NewCmdView`. +// +// It is generic over `T` as each command construction has their own Options type e.g. `ViewOptions` +type newCmdFunc[T any] func(f *cmdutil.Factory, runF func(*T) error) *cobra.Command + +func TestArgParsing[T any](t *testing.T, fn newCmdFunc[T]) { + tests := []struct { + name string + input string + expectedissueNumber int + expectedBaseRepo ghrepo.Interface + expectErr bool + }{ + { + name: "no argument", + input: "", + expectErr: true, + }, + { + name: "issue number argument", + input: "23 --repo owner/repo", + expectedissueNumber: 23, + expectedBaseRepo: ghrepo.New("owner", "repo"), + }, + { + name: "argument is hash prefixed number", + // Escaping is required here to avoid what I think is shellex treating it as a comment. + input: "\\#23 --repo owner/repo", + expectedissueNumber: 23, + expectedBaseRepo: ghrepo.New("owner", "repo"), + }, + { + name: "argument is a URL", + input: "https://github.com/cli/cli/issues/23", + expectedissueNumber: 23, + expectedBaseRepo: ghrepo.New("cli", "cli"), + }, + { + name: "argument cannot be parsed to an issue", + input: "unparseable", + expectErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &cmdutil.Factory{} + + argv, err := shlex.Split(tt.input) + assert.NoError(t, err) + + var gotOpts T + cmd := fn(f, func(opts *T) error { + gotOpts = *opts + return nil + }) + + cmdutil.EnableRepoOverride(cmd, f) + + // TODO: remember why we do this + cmd.Flags().BoolP("help", "x", false, "") + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + + if tt.expectErr { + require.Error(t, err) + return + } else { + require.NoError(t, err) + } + + actualIssueNumber := issueNumberFromOpts(t, gotOpts) + assert.Equal(t, tt.expectedissueNumber, actualIssueNumber) + + actualBaseRepo := baseRepoFromOpts(t, gotOpts) + assert.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, actualBaseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, actualBaseRepo, + ) + }) + } +} + +func issueNumberFromOpts(t *testing.T, v any) int { + rv := reflect.ValueOf(v) + field := rv.FieldByName("IssueNumber") + if !field.IsValid() || field.Kind() != reflect.Int { + t.Fatalf("Type %T does not have IssueNumber int field", v) + } + return int(field.Int()) +} + +func baseRepoFromOpts(t *testing.T, v any) ghrepo.Interface { + rv := reflect.ValueOf(v) + field := rv.FieldByName("BaseRepo") + // check whether the field is valid and of type func() (ghrepo.Interface, error) + if !field.IsValid() || field.Kind() != reflect.Func { + t.Fatalf("Type %T does not have BaseRepo func field", v) + } + // call the function and check the return value + results := field.Call([]reflect.Value{}) + if len(results) != 2 { + t.Fatalf("%T.BaseRepo() does not return two values", v) + } + if !results[1].IsNil() { + t.Fatalf("%T.BaseRepo() returned an error: %v", v, results[1].Interface()) + } + return results[0].Interface().(ghrepo.Interface) +} diff --git a/pkg/cmd/issue/close/close.go b/pkg/cmd/issue/close/close.go index 9197abff6..21fe45dd6 100644 --- a/pkg/cmd/issue/close/close.go +++ b/pkg/cmd/issue/close/close.go @@ -21,7 +21,7 @@ type CloseOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) - SelectorArg string + IssueNumber int Comment string Reason string @@ -39,13 +39,23 @@ func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Comm Short: "Close issue", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo - - if len(args) > 0 { - opts.SelectorArg = args[0] + issueNumber, baseRepo, err := shared.ParseIssueFromArg(args[0]) + if err != nil { + return err } + // If the args provided the base repo then use that directly. + if baseRepo, present := baseRepo.Value(); present { + opts.BaseRepo = func() (ghrepo.Interface, error) { + return baseRepo, nil + } + } else { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + } + + opts.IssueNumber = issueNumber + if runF != nil { return runF(opts) } @@ -67,7 +77,12 @@ func closeRun(opts *CloseOptions) error { return err } - issue, baseRepo, err := shared.IssueFromArgWithFields(httpClient, opts.BaseRepo, opts.SelectorArg, []string{"id", "number", "title", "state"}) + baseRepo, err := opts.BaseRepo() + if err != nil { + return err + } + + issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number", "title", "state"}) if err != nil { return err } diff --git a/pkg/cmd/issue/close/close_test.go b/pkg/cmd/issue/close/close_test.go index 4d50e56b2..04c39cd8d 100644 --- a/pkg/cmd/issue/close/close_test.go +++ b/pkg/cmd/issue/close/close_test.go @@ -7,46 +7,32 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/issue/argparsetest" "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 TestNewCmdClose(t *testing.T) { + // Test shared parsing of issue number / URL. + argparsetest.TestArgParsing(t, NewCmdClose) + tests := []struct { - name string - input string - output CloseOptions - wantErr bool - errMsg string + name string + input string + output CloseOptions + expectedBaseRepo ghrepo.Interface + wantErr bool + errMsg string }{ - { - name: "no argument", - input: "", - wantErr: true, - errMsg: "accepts 1 arg(s), received 0", - }, - { - name: "issue number", - input: "123", - output: CloseOptions{ - SelectorArg: "123", - }, - }, - { - name: "issue url", - input: "https://github.com/cli/cli/3", - output: CloseOptions{ - SelectorArg: "https://github.com/cli/cli/3", - }, - }, { name: "comment", input: "123 --comment 'closing comment'", output: CloseOptions{ - SelectorArg: "123", + IssueNumber: 123, Comment: "closing comment", }, }, @@ -54,7 +40,7 @@ func TestNewCmdClose(t *testing.T) { name: "reason", input: "123 --reason 'not planned'", output: CloseOptions{ - SelectorArg: "123", + IssueNumber: 123, Reason: "not planned", }, }, @@ -79,15 +65,24 @@ func TestNewCmdClose(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantErr { - assert.Error(t, err) + require.Error(t, err) assert.Equal(t, tt.errMsg, err.Error()) return } - assert.NoError(t, err) - assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg) + require.NoError(t, err) + assert.Equal(t, tt.output.IssueNumber, gotOpts.IssueNumber) assert.Equal(t, tt.output.Comment, gotOpts.Comment) assert.Equal(t, tt.output.Reason, gotOpts.Reason) + if tt.expectedBaseRepo != nil { + baseRepo, err := gotOpts.BaseRepo() + require.NoError(t, err) + require.True( + t, + ghrepo.IsSame(tt.expectedBaseRepo, baseRepo), + "expected base repo %+v, got %+v", tt.expectedBaseRepo, baseRepo, + ) + } }) } } @@ -104,7 +99,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue by number", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, }, httpStubs: func(reg *httpmock.Registry) { reg.Register( @@ -128,7 +123,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue with comment", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, Comment: "closing comment", }, httpStubs: func(reg *httpmock.Registry) { @@ -164,7 +159,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue with reason", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, Reason: "not planned", Detector: &fd.EnabledDetectorMock{}, }, @@ -192,7 +187,7 @@ func TestCloseRun(t *testing.T) { { name: "close issue with reason when reason is not supported", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, Reason: "not planned", Detector: &fd.DisabledDetectorMock{}, }, @@ -219,7 +214,7 @@ func TestCloseRun(t *testing.T) { { name: "issue already closed", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, }, httpStubs: func(reg *httpmock.Registry) { reg.Register( @@ -236,7 +231,7 @@ func TestCloseRun(t *testing.T) { { name: "issues disabled", opts: &CloseOptions{ - SelectorArg: "13", + IssueNumber: 13, }, httpStubs: func(reg *httpmock.Registry) { reg.Register( diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index bff6018d9..370c1c3c6 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -24,7 +24,7 @@ func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], erro for i, arg := range args { // For each argument, parse the issue number and an optional repo - issueNumber, issueRepo, err := parseIssueFromArg(arg) + issueNumber, issueRepo, err := ParseIssueFromArg(arg) if err != nil { return nil, o.None[ghrepo.Interface](), err } @@ -53,7 +53,7 @@ func ParseIssuesFromArgs(args []string) ([]int, o.Option[ghrepo.Interface], erro return issueNumbers, repo, nil } -func parseIssueFromArg(arg string) (int, o.Option[ghrepo.Interface], error) { +func ParseIssueFromArg(arg string) (int, o.Option[ghrepo.Interface], error) { issueLocator := tryParseIssueFromURL(arg) if issueLocator, present := issueLocator.Value(); present { return issueLocator.issueNumber, o.Some(issueLocator.repo), nil @@ -111,7 +111,7 @@ func IssueFromArgWithFields(httpClient *http.Client, baseRepoFn func() (ghrepo.I } } - issue, err := findIssueOrPR(httpClient, baseRepo, issueNumber, fields) + issue, err := FindIssueOrPR(httpClient, baseRepo, issueNumber, fields) return issue, baseRepo, err } @@ -165,7 +165,7 @@ func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumber for _, num := range issueNumbers { issueNumber := num g.Go(func() error { - issue, err := findIssueOrPR(httpClient, repo, issueNumber, fields) + issue, err := FindIssueOrPR(httpClient, repo, issueNumber, fields) if err != nil { return err } @@ -190,7 +190,7 @@ func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumber return issues, nil } -func findIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) { +func FindIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) { fieldSet := set.NewStringSet() fieldSet.AddValues(fields) if fieldSet.Contains("stateReason") {