Restore old error functionality of prSelectorForCurrentBranch
Before this refactor, the errors emitted by ghrepo.FromURL and rem.FindName() were suppressed. It isn't clear whether this was intentional or not, but we've made the decision here to maintain the original error behavior while still refactoring the return values for more clarity. I've left a comment at each error handling block to explain this decision. Additionally, I've added the necessary git command stubs to the other tests in status_test.go so that the tests are now passing.
This commit is contained in:
Tyler McGoffin
parent
322a43feff
commit
74cbd50d3f
2 changed files with 77 additions and 7 deletions
|
|
@ -205,14 +205,20 @@ func prSelectorForCurrentBranch(branchConfig git.BranchConfig, baseRepo ghrepo.I
|
|||
// the branch merges from a remote specified by URL
|
||||
r, err := ghrepo.FromURL(branchConfig.RemoteURL)
|
||||
if err != nil {
|
||||
return 0, prHeadRef, err
|
||||
// TODO: We aren't returning the error because we discovered that it was shadowed
|
||||
// before refactoring to its current return pattern. Thus, we aren't confident
|
||||
// that returning the error won't break existing behavior.
|
||||
return 0, prHeadRef, nil
|
||||
}
|
||||
branchOwner = r.RepoOwner()
|
||||
} else if branchConfig.RemoteName != "" {
|
||||
// the branch merges from a remote specified by name
|
||||
r, err := rem.FindByName(branchConfig.RemoteName)
|
||||
if err != nil {
|
||||
return 0, prHeadRef, err
|
||||
// TODO: We aren't returning the error because we discovered that it was shadowed
|
||||
// before refactoring to its current return pattern. Thus, we aren't confident
|
||||
// that returning the error won't break existing behavior.
|
||||
return 0, prHeadRef, nil
|
||||
}
|
||||
branchOwner = r.RepoOwner()
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@ package status
|
|||
|
||||
import (
|
||||
"bytes"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/url"
|
||||
|
|
@ -16,6 +15,7 @@ import (
|
|||
fd "github.com/cli/cli/v2/internal/featuredetection"
|
||||
"github.com/cli/cli/v2/internal/gh"
|
||||
"github.com/cli/cli/v2/internal/ghrepo"
|
||||
"github.com/cli/cli/v2/internal/run"
|
||||
"github.com/cli/cli/v2/pkg/cmdutil"
|
||||
"github.com/cli/cli/v2/pkg/httpmock"
|
||||
"github.com/cli/cli/v2/pkg/iostreams"
|
||||
|
|
@ -96,6 +96,11 @@ func TestPRStatus(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatus.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -121,6 +126,11 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) {
|
|||
// status,conclusion matches the old StatusContextRollup query
|
||||
http.Register(httpmock.GraphQL(`status,conclusion`), httpmock.FileResponse("./fixtures/prStatusChecks.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -146,6 +156,11 @@ func TestPRStatus_reviewsAndChecksWithStatesByCount(t *testing.T) {
|
|||
// checkRunCount,checkRunCountsByState matches the new StatusContextRollup query
|
||||
http.Register(httpmock.GraphQL(`checkRunCount,checkRunCountsByState`), httpmock.FileResponse("./fixtures/prStatusChecksWithStatesByCount.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommandWithDetector(http, "blueberries", true, "", &fd.EnabledDetectorMock{})
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -170,6 +185,11 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatusCurrentBranch.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -198,6 +218,11 @@ func TestPRStatus_currentBranch_defaultBranch(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatusCurrentBranch.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -232,6 +257,11 @@ func TestPRStatus_currentBranch_Closed(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatusCurrentBranchClosed.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -249,6 +279,11 @@ func TestPRStatus_currentBranch_Closed_defaultBranch(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatusCurrentBranchClosedOnDefaultBranch.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -266,6 +301,11 @@ func TestPRStatus_currentBranch_Merged(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatusCurrentBranchMerged.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -283,6 +323,11 @@ func TestPRStatus_currentBranch_Merged_defaultBranch(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatusCurrentBranchMergedOnDefaultBranch.json"))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -300,6 +345,11 @@ func TestPRStatus_blankSlate(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.StringResponse(`{"data": {}}`))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "blueberries", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -353,6 +403,11 @@ func TestPRStatus_detachedHead(t *testing.T) {
|
|||
defer http.Verify(t)
|
||||
http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.StringResponse(`{"data": {}}`))
|
||||
|
||||
// stub successful git command
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 0, "")
|
||||
|
||||
output, err := runCommand(http, "", true, "")
|
||||
if err != nil {
|
||||
t.Errorf("error running command `pr status`: %v", err)
|
||||
|
|
@ -376,6 +431,15 @@ Requesting a code review from you
|
|||
}
|
||||
}
|
||||
|
||||
func TestPRStatus_error_ReadBranchConfig(t *testing.T) {
|
||||
rs, cleanup := run.Stub()
|
||||
defer cleanup(t)
|
||||
rs.Register(`git config --get-regexp \^branch\\.`, 1, "")
|
||||
|
||||
_, err := runCommand(initFakeHTTP(), "blueberries", true, "")
|
||||
assert.Error(t, err)
|
||||
}
|
||||
|
||||
func Test_prSelectorForCurrentBranch(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
|
@ -537,7 +601,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) {
|
|||
wantError: nil,
|
||||
},
|
||||
{
|
||||
name: "Remote URL error",
|
||||
name: "Remote URL errors",
|
||||
branchConfig: git.BranchConfig{
|
||||
RemoteURL: &url.URL{
|
||||
Scheme: "ssh",
|
||||
|
|
@ -549,10 +613,10 @@ func Test_prSelectorForCurrentBranch(t *testing.T) {
|
|||
prHeadRef: "monalisa/main",
|
||||
wantPrNumber: 0,
|
||||
wantSelector: "monalisa/main",
|
||||
wantError: fmt.Errorf("invalid path: /\\invalid?Path/"),
|
||||
wantError: nil,
|
||||
},
|
||||
{
|
||||
name: "Remote Name error",
|
||||
name: "Remote Name errors",
|
||||
branchConfig: git.BranchConfig{
|
||||
RemoteName: "nonexistentRemote",
|
||||
},
|
||||
|
|
@ -569,7 +633,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) {
|
|||
},
|
||||
wantPrNumber: 0,
|
||||
wantSelector: "monalisa/main",
|
||||
wantError: fmt.Errorf("no matching remote found"),
|
||||
wantError: nil,
|
||||
},
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue