Repository: google/go-github
Pull request #1772 (2021-01-05 16:26:43)

Add client.BareDo to allow user to handle the body
This allows the user to also treat bodies as plain go streams.
For more flexibility.

~Before this, passing a nil body would do the request but close the~
~body. Now it is up to the caller to close resp.Body.~

~I mainly wanted to be able to do this to stream bodies and to re-use the same HTTP client in order to download release files.~

Edit:
This PR enables using the client as an http client with Github Specific error handling and throttling management.

codecov[bot]: # [Codecov](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=h1) Report
> Merging [#1772](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=desc) (e4cbdda) into [master](https://codecov.io/gh/google/go-github/commit/7081b5fe343920a77bb5601a68df93bcb6662663?el=desc) (7081b5f) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/google/go-github/pull/1772/graphs/tree.svg?width=650&height=150&src=pr&token=EgYjGjZlI0)](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master    #1772   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files          98       98           
  Lines        6373     6378    +5     
=======================================
+ Hits         6216     6221    +5     
  Misses         85       85           
  Partials       72       72           
```


| [Impacted Files](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [github/github.go](https://codecov.io/gh/google/go-github/pull/1772/diff?src=pr&el=tree#diff-Z2l0aHViL2dpdGh1Yi5nbw==) | `96.61% <100.00%> (+0.05%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=footer). Last update [7081b5f...e4cbdda](https://codecov.io/gh/google/go-github/pull/1772?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

gmlewis: Hmmm... I'm concerned that this change might be too disruptive for users of this package.  If I understand correctly, it would force all users to add code to manually close the body.

willnorris: I had the same thought... this seems like a pretty risky change.
azr: Yes, that is correct ! Is that an issue even for v34 ?
azr: To be precise it would force users that do nothing with the response's body.
gmlewis: > Yes, that is correct ! Is that an issue even for v34 ?

Yes. While bumping version numbers is indeed used for breaking API changes, this change I feel is too disruptive for the majority of users of this package to be generally useful.

I'm happy to leave this issue/PR open for discussion, but if there is not much interest in it, I think we will most likely close it.

Alternatively, if you could come up with a PR that would default to existing behavior but as an option people could enable your proposed new behavior, that might be more palatable for inclusion in this repo.
azr: Hey @gmlewis,

> this change I feel is too disruptive for the majority of users of this package to be generally useful.

Gotcha, I understand 🙂 . The api doesn't change so unless you read the changelog... Its an easy trap.

> but if there is not much interest in it, I think we will most likely close it.

After some thinking I still would like to reuse the same client to download binaries from Github. In case they start adding api limitations or something.  So I still would like to make this work. Also I like to handle the body like if I was doing a plain http request.

I was thinking of adding a new function that looks like so:

```go
// ___ Is just like Do but will let you handle the body.
// Make sure to close the Response Body.
func (c *Client) ___(ctx context.Context, req *http.Request) (*Response, error) {
```

In terms of name I was thinking of something like `BareDo` ? `DoWithContext` ( meh ).

I'm also happy to implement something with your idea:

So allowing to call:

```go
client.Do(ctx, req, github.LeaveBody)
```

Ah naming is hard :D Do you have a suggestion there ?

( I like my suggestion a tad more, because it feels simple and sort of godoc friendly, I will implement it to gain some time but feel free to make me change it ).
azr: Hello there, commenting here just to say that this works for me. I had a tiny issue where the auth token from github was bein set again to a request done to aws which was 400'ing because it want only one auth token, I was able to fix this using the following code:

```go
fuunc something() {
	ts := oauth2.StaticTokenSource(
		&oauth2.Token{AccessToken: tk},
	)
	tc = &http.Client{
		Transport: &HostSpecificTokenAuthTransport{
			TokenSources: map[string]oauth2.TokenSource{
				"api.github.com": ts,
			},
		},
	}
}

type HostSpecificTokenAuthTransport struct {
	TokenSources map[string]oauth2.TokenSource

	// Transport is the underlying HTTP transport to use when making requests.
	// It will default to http.DefaultTransport if nil.
	Transport http.RoundTripper

	Base http.RoundTripper
}

// RoundTrip authorizes and authenticates the request with an
// access token from Transport's Source.
func (t *HostSpecificTokenAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	source, found := t.TokenSources[req.Host]
	if found {
		reqBodyClosed := false
		if req.Body != nil {
			defer func() {
				if !reqBodyClosed {
					req.Body.Close()
				}
			}()
		}

		if source == nil {
			return nil, errors.New("transport's Source is nil")
		}
		token, err := source.Token()
		if err != nil {
			return nil, err
		}

		token.SetAuthHeader(req)

		// req.Body is assumed to be closed by the base RoundTripper.
		reqBodyClosed = true
	}

	return t.base().RoundTrip(req)
}


func (t *HostSpecificTokenAuthTransport) base() http.RoundTripper {
	if t.Base != nil {
		return t.Base
	}
	return http.DefaultTransport
}
```

This seems to be related with #804 and #246 would you like me to contribute this change here ? 
gmlewis: I'm not sure I fully understand how you expect `BareDo` to be used since most (all?) endpoints in this repo call `Do` directly.

Do you expect users of this library to call it directly from their own code?
azr: That's what I'd like to do, yes. I need all the nice error handling and throttling management but would like to handle the bodies myself, just like if I was using an plain http.Client that knows a little more about the Github API. It makes this library even more powerful to me. Because it'd be sort of replaceable.

azr: Okay I updated the PR Thanks for reviewing 🙂 
azr: Good idea, I added a func to test this 🙂 

Patch:
diff --git a/github/github.go b/github/github.go
index 957ac0c7..0a32a53c 100644
--- a/github/github.go
+++ b/github/github.go
@@ -512,16 +512,15 @@ func parseRate(r *http.Response) Rate {
 	return rate
 }
 
-// Do sends an API request and returns the API response. The API response is
-// JSON decoded and stored in the value pointed to by v, or returned as an
-// error if an API error has occurred. If v implements the io.Writer
-// interface, the raw response body will be written to v, without attempting to
-// first decode it. If rate limit is exceeded and reset time is in the future,
-// Do returns *RateLimitError immediately without making a network API call.
+// BareDo sends an API request and lets you handle the api response. If an error
+// or API Error occurs, the error will contain more information. Otherwise you
+// are supposed to read and close the response's Body. If rate limit is exceeded
+// and reset time is in the future, BareDo returns *RateLimitError immediately
+// without making a network API call.
 //
-// The provided ctx must be non-nil, if it is nil an error is returned. If it is canceled or times out,
-// ctx.Err() will be returned.
-func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) {
+// The provided ctx must be non-nil, if it is nil an error is returned. If it is
+// canceled or times out, ctx.Err() will be returned.
+func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, error) {
 	if ctx == nil {
 		return nil, errors.New("context must be non-nil")
 	}
@@ -558,8 +557,6 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res
 		return nil, err
 	}
 
-	defer resp.Body.Close()
-
 	response := newResponse(resp)
 
 	c.rateMu.Lock()
@@ -568,6 +565,7 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res
 
 	err = CheckResponse(resp)
 	if err != nil {
+		defer resp.Body.Close()
 		// Special case for AcceptedErrors. If an AcceptedError
 		// has been encountered, the response's payload will be
 		// added to the AcceptedError and returned.
@@ -581,27 +579,43 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res
 			}
 
 			aerr.Raw = b
-			return response, aerr
+			err = aerr
 		}
+	}
+	return response, err
+}
 
-		return response, err
+// Do sends an API request and returns the API response. The API response is
+// JSON decoded and stored in the value pointed to by v, or returned as an
+// error if an API error has occurred. If v implements the io.Writer interface,
+// the raw response body will be written to v, without attempting to first
+// decode it. If v is nil, and no error hapens, the response is returned as is.
+// If rate limit is exceeded and reset time is in the future, Do returns
+// *RateLimitError immediately without making a network API call.
+//
+// The provided ctx must be non-nil, if it is nil an error is returned. If it
+// is canceled or times out, ctx.Err() will be returned.
+func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) {
+	resp, err := c.BareDo(ctx, req)
+	if err != nil {
+		return resp, err
 	}
+	defer resp.Body.Close()
 
-	if v != nil {
-		if w, ok := v.(io.Writer); ok {
-			io.Copy(w, resp.Body)
-		} else {
-			decErr := json.NewDecoder(resp.Body).Decode(v)
-			if decErr == io.EOF {
-				decErr = nil // ignore EOF errors caused by empty response body
-			}
-			if decErr != nil {
-				err = decErr
-			}
+	switch v := v.(type) {
+	case nil:
+	case io.Writer:
+		_, err = io.Copy(v, resp.Body)
+	default:
+		decErr := json.NewDecoder(resp.Body).Decode(v)
+		if decErr == io.EOF {
+			decErr = nil // ignore EOF errors caused by empty response body
+		}
+		if decErr != nil {
+			err = decErr
 		}
 	}
-
-	return response, err
+	return resp, err
 }
 
 // checkRateLimitBeforeDo does not make any network calls, but uses existing knowledge from
diff --git a/github/repos_commits.go b/github/repos_commits.go
index 707e5cb1..b49a5afe 100644
--- a/github/repos_commits.go
+++ b/github/repos_commits.go
@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"net/url"
 	"time"
 )
 
@@ -224,7 +225,10 @@ func (s *RepositoriesService) GetCommitSHA1(ctx context.Context, owner, repo, re
 //
 // GitHub API docs: https://docs.github.com/en/free-pro-team@latest/rest/reference/repos/#compare-two-commits
 func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo string, base, head string) (*CommitsComparison, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head)
+	escapedBase := url.QueryEscape(base)
+	escapedHead := url.QueryEscape(head)
+
+	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, escapedBase, escapedHead)
 
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
@@ -248,7 +252,11 @@ func (s *RepositoriesService) CompareCommits(ctx context.Context, owner, repo st
 //
 // GitHub API docs: https://docs.github.com/en/free-pro-team@latest/rest/reference/repos/#compare-two-commits
 func (s *RepositoriesService) CompareCommitsRaw(ctx context.Context, owner, repo, base, head string, opts RawOptions) (string, *Response, error) {
-	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, base, head)
+	escapedBase := url.QueryEscape(base)
+	escapedHead := url.QueryEscape(head)
+
+	u := fmt.Sprintf("repos/%v/%v/compare/%v...%v", owner, repo, escapedBase, escapedHead)
+
 	req, err := s.client.NewRequest("GET", u, nil)
 	if err != nil {
 		return "", nil, err


Test patch:
diff --git a/github/github_test.go b/github/github_test.go
index e95c2d00..db50b465 100644
--- a/github/github_test.go
+++ b/github/github_test.go
@@ -1560,3 +1560,38 @@ func TestAddOptions_QueryValues(t *testing.T) {
 		t.Error("addOptions err = nil, want error")
 	}
 }
+
+func TestBareDo_returnsOpenBody(t *testing.T) {
+
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	expectedBody := "Hello from the other side !"
+
+	mux.HandleFunc("/test-url", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		fmt.Fprint(w, expectedBody)
+	})
+
+	ctx := context.Background()
+	req, err := client.NewRequest("GET", "test-url", nil)
+	if err != nil {
+		t.Fatalf("client.NewRequest returned error: %v", err)
+	}
+
+	resp, err := client.BareDo(ctx, req)
+	if err != nil {
+		t.Fatalf("client.BareDo returned error: %v", err)
+	}
+
+	got, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("ioutil.ReadAll returned error: %v", err)
+	}
+	if string(got) != expectedBody {
+		t.Fatalf("Expected %q, got %q", expectedBody, string(got))
+	}
+	if err := resp.Body.Close(); err != nil {
+		t.Fatalf("resp.Body.Close() returned error: %v", err)
+	}
+}
diff --git a/github/repos_commits_test.go b/github/repos_commits_test.go
index 0deeeef5..f2fde498 100644
--- a/github/repos_commits_test.go
+++ b/github/repos_commits_test.go
@@ -9,6 +9,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"net/url"
 	"reflect"
 	"strings"
 	"testing"
@@ -366,12 +367,28 @@ func TestRepositoriesService_TrailingPercent_GetCommitSHA1(t *testing.T) {
 }
 
 func TestRepositoriesService_CompareCommits(t *testing.T) {
-	client, mux, _, teardown := setup()
-	defer teardown()
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
+	}
 
-	mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) {
-		testMethod(t, r, "GET")
-		fmt.Fprintf(w, `{
+	for _, sample := range testCases {
+		client, mux, _, teardown := setup()
+
+		base := sample.base
+		head := sample.head
+		escapedBase := url.QueryEscape(base)
+		escapedHead := url.QueryEscape(head)
+
+		pattern := fmt.Sprintf("/repos/o/r/compare/%v...%v", base, head)
+
+		mux.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) {
+			testMethod(t, r, "GET")
+			fmt.Fprintf(w, `{
 		  "base_commit": {
 		    "sha": "s",
 		    "commit": {
@@ -398,46 +415,28 @@ func TestRepositoriesService_CompareCommits(t *testing.T) {
 		    }
 		  ],
 		  "files": [ { "filename": "f" } ],
-		  "html_url":      "https://github.com/o/r/compare/b...h",
+		  "html_url":      "https://github.com/o/r/compare/%[1]v...%[2]v",
 		  "permalink_url": "https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17",
-		  "diff_url":      "https://github.com/o/r/compare/b...h.diff",
-		  "patch_url":     "https://github.com/o/r/compare/b...h.patch",
-		  "url":           "https://api.github.com/repos/o/r/compare/b...h"
-		}`)
-	})
-
-	ctx := context.Background()
-	got, _, err := client.Repositories.CompareCommits(ctx, "o", "r", "b", "h")
-	if err != nil {
-		t.Errorf("Repositories.CompareCommits returned error: %v", err)
-	}
+		  "diff_url":      "https://github.com/o/r/compare/%[1]v...%[2]v.diff",
+		  "patch_url":     "https://github.com/o/r/compare/%[1]v...%[2]v.patch",
+		  "url":           "https://api.github.com/repos/o/r/compare/%[1]v...%[2]v"
+		}`, escapedBase, escapedHead)
+		})
+
+		ctx := context.Background()
+		got, _, err := client.Repositories.CompareCommits(ctx, "o", "r", base, head)
+		if err != nil {
+			t.Errorf("Repositories.CompareCommits returned error: %v", err)
+		}
 
-	want := &CommitsComparison{
-		BaseCommit: &RepositoryCommit{
-			SHA: String("s"),
-			Commit: &Commit{
-				Author:    &CommitAuthor{Name: String("n")},
-				Committer: &CommitAuthor{Name: String("n")},
-				Message:   String("m"),
-				Tree:      &Tree{SHA: String("t")},
-			},
-			Author:    &User{Login: String("l")},
-			Committer: &User{Login: String("l")},
-			Parents: []*Commit{
-				{
-					SHA: String("s"),
-				},
-			},
-		},
-		Status:       String("s"),
-		AheadBy:      Int(1),
-		BehindBy:     Int(2),
-		TotalCommits: Int(1),
-		Commits: []*RepositoryCommit{
-			{
+		want := &CommitsComparison{
+			BaseCommit: &RepositoryCommit{
 				SHA: String("s"),
 				Commit: &Commit{
-					Author: &CommitAuthor{Name: String("n")},
+					Author:    &CommitAuthor{Name: String("n")},
+					Committer: &CommitAuthor{Name: String("n")},
+					Message:   String("m"),
+					Tree:      &Tree{SHA: String("t")},
 				},
 				Author:    &User{Login: String("l")},
 				Committer: &User{Login: String("l")},
@@ -447,109 +446,171 @@ func TestRepositoriesService_CompareCommits(t *testing.T) {
 					},
 				},
 			},
-		},
-		Files: []*CommitFile{
-			{
-				Filename: String("f"),
+			Status:       String("s"),
+			AheadBy:      Int(1),
+			BehindBy:     Int(2),
+			TotalCommits: Int(1),
+			Commits: []*RepositoryCommit{
+				{
+					SHA: String("s"),
+					Commit: &Commit{
+						Author: &CommitAuthor{Name: String("n")},
+					},
+					Author:    &User{Login: String("l")},
+					Committer: &User{Login: String("l")},
+					Parents: []*Commit{
+						{
+							SHA: String("s"),
+						},
+					},
+				},
 			},
-		},
-		HTMLURL:      String("https://github.com/o/r/compare/b...h"),
-		PermalinkURL: String("https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17"),
-		DiffURL:      String("https://github.com/o/r/compare/b...h.diff"),
-		PatchURL:     String("https://github.com/o/r/compare/b...h.patch"),
-		URL:          String("https://api.github.com/repos/o/r/compare/b...h"),
-	}
+			Files: []*CommitFile{
+				{
+					Filename: String("f"),
+				},
+			},
+			HTMLURL:      String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v", escapedBase, escapedHead)),
+			PermalinkURL: String("https://github.com/o/r/compare/o:bbcd538c8e72b8c175046e27cc8f907076331401...o:0328041d1152db8ae77652d1618a02e57f745f17"),
+			DiffURL:      String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v.diff", escapedBase, escapedHead)),
+			PatchURL:     String(fmt.Sprintf("https://github.com/o/r/compare/%v...%v.patch", escapedBase, escapedHead)),
+			URL:          String(fmt.Sprintf("https://api.github.com/repos/o/r/compare/%v...%v", escapedBase, escapedHead)),
+		}
 
-	if !reflect.DeepEqual(got, want) {
-		t.Errorf("Repositories.CompareCommits returned \n%+v, want \n%+v", got, want)
-	}
+		if !reflect.DeepEqual(got, want) {
+			t.Errorf("Repositories.CompareCommits returned \n%+v, want \n%+v", got, want)
+		}
 
-	const methodName = "CompareCommits"
-	testBadOptions(t, methodName, func() (err error) {
-		_, _, err = client.Repositories.CompareCommits(ctx, "\n", "\n", "\n", "\n")
-		return err
-	})
+		const methodName = "CompareCommits"
+		testBadOptions(t, methodName, func() (err error) {
+			_, _, err = client.Repositories.CompareCommits(ctx, "\n", "\n", "\n", "\n")
+			return err
+		})
 
-	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
-		got, resp, err := client.Repositories.CompareCommits(ctx, "o", "r", "b", "h")
-		if got != nil {
-			t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
-		}
-		return resp, err
-	})
+		testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+			got, resp, err := client.Repositories.CompareCommits(ctx, "o", "r", base, head)
+			if got != nil {
+				t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got)
+			}
+			return resp, err
+		})
+
+		teardown()
+	}
 }
 
 func TestRepositoriesService_CompareCommitsRaw_diff(t *testing.T) {
-	client, mux, _, teardown := setup()
-	defer teardown()
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
+	}
+
+	for _, sample := range testCases {
+		client, mux, _, teardown := setup()
+
+		base := sample.base
+		head := sample.head
+		pattern := fmt.Sprintf("/repos/o/r/compare/%v...%v", base, head)
+		const rawStr = "@@diff content"
+
+		mux.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) {
+			testMethod(t, r, "GET")
+			testHeader(t, r, "Accept", mediaTypeV3Diff)
+			fmt.Fprint(w, rawStr)
+		})
+
+		ctx := context.Background()
+		got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", base, head, RawOptions{Type: Diff})
+		if err != nil {
+			t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
+		}
+		want := rawStr
+		if got != want {
+			t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
+		}
 
-	const rawStr = "@@diff content"
+		const methodName = "CompareCommitsRaw"
+		testBadOptions(t, methodName, func() (err error) {
+			_, _, err = client.Repositories.CompareCommitsRaw(ctx, "\n", "\n", "\n", "\n", RawOptions{Type: Diff})
+			return err
+		})
 
-	mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) {
-		testMethod(t, r, "GET")
-		testHeader(t, r, "Accept", mediaTypeV3Diff)
-		fmt.Fprint(w, rawStr)
-	})
+		testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+			got, resp, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", base, head, RawOptions{Type: Diff})
+			if got != "" {
+				t.Errorf("testNewRequestAndDoFailure %v = %#v, want ''", methodName, got)
+			}
+			return resp, err
+		})
 
-	ctx := context.Background()
-	got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "b", "h", RawOptions{Type: Diff})
-	if err != nil {
-		t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
+		teardown()
 	}
-	want := rawStr
-	if got != want {
-		t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
-	}
-
-	const methodName = "CompareCommitsRaw"
-	testBadOptions(t, methodName, func() (err error) {
-		_, _, err = client.Repositories.CompareCommitsRaw(ctx, "\n", "\n", "\n", "\n", RawOptions{Type: Diff})
-		return err
-	})
-
-	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
-		got, resp, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "b", "h", RawOptions{Type: Diff})
-		if got != "" {
-			t.Errorf("testNewRequestAndDoFailure %v = %#v, want ''", methodName, got)
-		}
-		return resp, err
-	})
 }
 
 func TestRepositoriesService_CompareCommitsRaw_patch(t *testing.T) {
-	client, mux, _, teardown := setup()
-	defer teardown()
-
-	const rawStr = "@@patch content"
-
-	mux.HandleFunc("/repos/o/r/compare/b...h", func(w http.ResponseWriter, r *http.Request) {
-		testMethod(t, r, "GET")
-		testHeader(t, r, "Accept", mediaTypeV3Patch)
-		fmt.Fprint(w, rawStr)
-	})
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
+	}
+
+	for _, sample := range testCases {
+		client, mux, _, teardown := setup()
+
+		base := sample.base
+		head := sample.head
+		pattern := fmt.Sprintf("/repos/o/r/compare/%v...%v", base, head)
+		const rawStr = "@@patch content"
+
+		mux.HandleFunc(pattern, func(w http.ResponseWriter, r *http.Request) {
+			testMethod(t, r, "GET")
+			testHeader(t, r, "Accept", mediaTypeV3Patch)
+			fmt.Fprint(w, rawStr)
+		})
+
+		ctx := context.Background()
+		got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", base, head, RawOptions{Type: Patch})
+		if err != nil {
+			t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
+		}
+		want := rawStr
+		if got != want {
+			t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
+		}
 
-	ctx := context.Background()
-	got, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "b", "h", RawOptions{Type: Patch})
-	if err != nil {
-		t.Fatalf("Repositories.GetCommitRaw returned error: %v", err)
-	}
-	want := rawStr
-	if got != want {
-		t.Errorf("Repositories.GetCommitRaw returned %s want %s", got, want)
+		teardown()
 	}
 }
 
 func TestRepositoriesService_CompareCommitsRaw_invalid(t *testing.T) {
-	client, _, _, teardown := setup()
-	defer teardown()
-
 	ctx := context.Background()
-	_, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", "s", "h", RawOptions{100})
-	if err == nil {
-		t.Fatal("Repositories.GetCommitRaw should return error")
+
+	testCases := []struct {
+		base string
+		head string
+	}{
+		{base: "b", head: "h"},
+		{base: "123base", head: "head123"},
+		{base: "`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+123base", head: "head123`~!@#$%^&*()_+-=[]\\{}|;':\",./<>?/*-+"},
 	}
-	if !strings.Contains(err.Error(), "unsupported raw type") {
-		t.Error("Repositories.GetCommitRaw should return unsupported raw type error")
+
+	for _, sample := range testCases {
+		client, _, _, teardown := setup()
+		_, _, err := client.Repositories.CompareCommitsRaw(ctx, "o", "r", sample.base, sample.head, RawOptions{100})
+		if err == nil {
+			t.Fatal("Repositories.GetCommitRaw should return error")
+		}
+		if !strings.Contains(err.Error(), "unsupported raw type") {
+			t.Error("Repositories.GetCommitRaw should return unsupported raw type error")
+		}
+		teardown()
 	}
 }