diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 3bc197b75..3f036c0cd 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -37,7 +37,7 @@ type finder struct { branchFn func() (string, error) remotesFn func() (remotes.Remotes, error) httpClient func() (*http.Client, error) - branchConfig func(string) git.BranchConfig + branchConfig func(string) (git.BranchConfig, error) progress progressIndicator repo ghrepo.Interface @@ -58,9 +58,8 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { remotesFn: factory.Remotes, httpClient: factory.HttpClient, progress: factory.IOStreams, - branchConfig: func(s string) git.BranchConfig { - branchConfig, _ := factory.GitClient.ReadBranchConfig(context.Background(), s) - return branchConfig + branchConfig: func(s string) (git.BranchConfig, error) { + return factory.GitClient.ReadBranchConfig(context.Background(), s) }, } } @@ -239,7 +238,10 @@ func (f *finder) parseCurrentBranch() (string, int, error) { return "", 0, err } - branchConfig := f.branchConfig(prHeadRef) + branchConfig, err := f.branchConfig(prHeadRef) + if err != nil { + return "", 0, err + } // the branch is configured to merge a special PR head ref if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 258ef01e8..dd96e684a 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -10,19 +10,21 @@ import ( "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type args struct { + baseRepoFn func() (ghrepo.Interface, error) + branchFn func() (string, error) + branchConfig func(string) (git.BranchConfig, error) + remotesFn func() (context.Remotes, error) + selector string + fields []string + baseBranch string +} + func TestFind(t *testing.T) { - type args struct { - baseRepoFn func() (ghrepo.Interface, error) - branchFn func() (string, error) - branchConfig func(string) git.BranchConfig - remotesFn func() (context.Remotes, error) - selector string - fields []string - baseBranch string - } tests := []struct { name string args args @@ -231,9 +233,7 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { - return - }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -265,9 +265,7 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { - return - }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -315,11 +313,10 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { - c.MergeRef = "refs/heads/blue-upstream-berries" - c.RemoteName = "origin" - return - }, + branchConfig: stubBranchConfig(git.BranchConfig{ + MergeRef: "refs/heads/blue-upstream-berries", + RemoteName: "origin", + }, nil), remotesFn: func() (context.Remotes, error) { return context.Remotes{{ Remote: &git.Remote{Name: "origin"}, @@ -357,11 +354,12 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { + branchConfig: func(branch string) (git.BranchConfig, error) { u, _ := url.Parse("https://github.com/UPSTREAMOWNER/REPO") - c.MergeRef = "refs/heads/blue-upstream-berries" - c.RemoteURL = u - return + return stubBranchConfig(git.BranchConfig{ + MergeRef: "refs/heads/blue-upstream-berries", + RemoteURL: u, + }, nil)(branch) }, remotesFn: nil, }, @@ -395,10 +393,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { - c.RemoteName = "origin" - return - }, + branchConfig: stubBranchConfig(git.BranchConfig{ + RemoteName: "origin", + }, nil), remotesFn: func() (context.Remotes, error) { return context.Remotes{{ Remote: &git.Remote{Name: "origin"}, @@ -436,10 +433,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { - c.MergeRef = "refs/pull/13/head" - return - }, + branchConfig: stubBranchConfig(git.BranchConfig{ + MergeRef: "refs/pull/13/head", + }, nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -462,10 +458,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: func(branch string) (c git.BranchConfig) { - c.MergeRef = "refs/pull/13/head" - return - }, + branchConfig: stubBranchConfig(git.BranchConfig{ + MergeRef: "refs/pull/13/head", + }, nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -561,3 +556,49 @@ func TestFind(t *testing.T) { }) } } + +func Test_parseCurrentBranch(t *testing.T) { + tests := []struct { + name string + args args + wantSelector string + wantPR int + wantError error + }{ + { + name: "failed branch config", + args: args{ + branchConfig: stubBranchConfig(git.BranchConfig{}, errors.New("branchConfigErr")), + branchFn: func() (string, error) { + return "blueberries", nil + }, + }, + wantSelector: "", + wantPR: 0, + wantError: errors.New("branchConfigErr"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := finder{ + httpClient: func() (*http.Client, error) { + return &http.Client{}, nil + }, + baseRepoFn: tt.args.baseRepoFn, + branchFn: tt.args.branchFn, + branchConfig: tt.args.branchConfig, + remotesFn: tt.args.remotesFn, + } + selector, pr, err := f.parseCurrentBranch() + assert.Equal(t, tt.wantSelector, selector) + assert.Equal(t, tt.wantPR, pr) + assert.Equal(t, tt.wantError, err) + }) + } +} + +func stubBranchConfig(branchConfig git.BranchConfig, err error) func(string) (git.BranchConfig, error) { + return func(branch string) (git.BranchConfig, error) { + return branchConfig, err + } +}