From d5cb5574d01671bc3bae24cbfc7fa04f29102e8d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mislav=20Marohni=C4=87?= <mislav@github.com>
Date: Wed, 21 Oct 2020 11:55:20 +0000
Subject: [PATCH] :nail_care: simplify `generateCompareURL` and test

---
 pkg/cmd/pr/create/create.go      |   6 +-
 pkg/cmd/pr/create/create_test.go | 100 ++++++++++++++++++++-----------
 2 files changed, 65 insertions(+), 41 deletions(-)

diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go
index 63e2fca2c..8ad123226 100644
--- a/pkg/cmd/pr/create/create.go
+++ b/pkg/cmd/pr/create/create.go
@@ -554,11 +554,7 @@ func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.Tr
 }
 
 func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestones []string) (string, error) {
-	// This is to fix issues with names like "branch/#123"
-	b := url.QueryEscape(base)
-	h := url.QueryEscape(head)
-
-	u := ghrepo.GenerateRepoURL(r, "compare/%s...%s?expand=1", b, h)
+	u := ghrepo.GenerateRepoURL(r, "compare/%s...%s?expand=1", url.QueryEscape(base), url.QueryEscape(head))
 	url, err := shared.WithPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestones)
 	if err != nil {
 		return "", err
diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go
index c3a54159f..456da84a9 100644
--- a/pkg/cmd/pr/create/create_test.go
+++ b/pkg/cmd/pr/create/create_test.go
@@ -636,42 +636,6 @@ func TestPRCreate_web(t *testing.T) {
 	eq(t, browserCall[len(browserCall)-1], "https://github.com/OWNER/REPO/compare/master...feature?expand=1")
 }
 
-func TestPRCreate_web_uncommon_branch_names(t *testing.T) {
-	http := initFakeHTTP()
-	defer http.Verify(t)
-
-	http.StubRepoInfoResponse("OWNER", "REPO", "master")
-	http.StubRepoResponse("OWNER", "REPO")
-	http.Register(
-		httpmock.GraphQL(`query UserCurrent\b`),
-		httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`))
-
-	cs, cmdTeardown := test.InitCmdStubber()
-	defer cmdTeardown()
-
-	cs.Stub("")                                         // git config --get-regexp (determineTrackingBranch)
-	cs.Stub("")                                         // git show-ref --verify   (determineTrackingBranch)
-	cs.Stub("")                                         // git status
-	cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log
-	cs.Stub("")                                         // git push
-	cs.Stub("")                                         // browser
-
-	ask, cleanupAsk := prompt.InitAskStubber()
-	defer cleanupAsk()
-	ask.StubOne(0)
-
-	output, err := runCommand(http, nil, "feature/#123", true, `--web`)
-	require.NoError(t, err)
-
-	eq(t, output.String(), "")
-	eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/compare/master...feature/#123 in your browser.\n")
-
-	eq(t, len(cs.Calls), 6)
-	eq(t, strings.Join(cs.Calls[4].Args, " "), "git push --set-upstream origin HEAD:feature/#123")
-	browserCall := cs.Calls[5].Args
-	eq(t, browserCall[len(browserCall)-1], "https://github.com/OWNER/REPO/compare/master...feature%2F%23123?expand=1")
-}
-
 func Test_determineTrackingBranch_empty(t *testing.T) {
 	cs, cmdTeardown := test.InitCmdStubber()
 	defer cmdTeardown()
@@ -766,3 +730,67 @@ deadb00f refs/remotes/origin/feature`) // git show-ref --verify (ShowRefs)
 
 	eq(t, cs.Calls[1].Args, []string{"git", "show-ref", "--verify", "--", "HEAD", "refs/remotes/origin/great-feat", "refs/remotes/origin/feature"})
 }
+
+func Test_generateCompareURL(t *testing.T) {
+	type args struct {
+		r          ghrepo.Interface
+		base       string
+		head       string
+		title      string
+		body       string
+		assignees  []string
+		labels     []string
+		projects   []string
+		milestones []string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    string
+		wantErr bool
+	}{
+		{
+			name: "basic",
+			args: args{
+				r:    ghrepo.New("OWNER", "REPO"),
+				base: "main",
+				head: "feature",
+			},
+			want:    "https://github.com/OWNER/REPO/compare/main...feature?expand=1",
+			wantErr: false,
+		},
+		{
+			name: "with labels",
+			args: args{
+				r:      ghrepo.New("OWNER", "REPO"),
+				base:   "a",
+				head:   "b",
+				labels: []string{"one", "two three"},
+			},
+			want:    "https://github.com/OWNER/REPO/compare/a...b?expand=1&labels=one%2Ctwo+three",
+			wantErr: false,
+		},
+		{
+			name: "complex branch names",
+			args: args{
+				r:    ghrepo.New("OWNER", "REPO"),
+				base: "main/trunk",
+				head: "owner:feature",
+			},
+			want:    "https://github.com/OWNER/REPO/compare/main%2Ftrunk...owner%3Afeature?expand=1",
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := generateCompareURL(tt.args.r, tt.args.base, tt.args.head, tt.args.title, tt.args.body, tt.args.assignees, tt.args.labels, tt.args.projects, tt.args.milestones)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("generateCompareURL() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("generateCompareURL() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}