STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Surface and handle error from ReadBranchConfig in parseCurrentBranch

Browse source 
I've only added the one test for parseCurrentBranch because the function
appears to be largely exercised by TestFind. There's definitely an
opportunity for a bigger refactor of the tests, here, but I want to avoid
scope creep as I propagate the ReadBranchConfig api changes throughout the
codebase
This commit is contained in:
Tyler McGoffin 2025-01-09 12:46:48 -08:00
parent e4320ccc4b
commit e4f0b79173
2 changed files with 84 additions and 41 deletions
Download patch file Download diff file Expand all files Collapse all files

12
pkg/cmd/pr/shared/finder.go
View file

@ -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 {

113
pkg/cmd/pr/shared/finder_test.go
View file

@ -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
}
}