The `github.ErrorResponse.Error()` method panics

[sebnow]

The ErrorResponse.Error() implementation references the *http.Response which is not provided by go-github-mock. The examples use type assertions to work around this, but wrapping errors is a common pattern in Go (e.g. fmt.Errorf("failed: %w", userErr)). If you're testing code which wraps the error, it will panic.

diff --git a/src/mock/server_test.go b/src/mock/server_test.go
index c88b41f..25e0d58 100644
--- a/src/mock/server_test.go
+++ b/src/mock/server_test.go
@@ -118,6 +118,7 @@ func TestMockErrors(t *testing.T) {
                t.Fatal("user err is nil, want *github.ErrorResponse")
        }

+       t.Errorf("%s", userErr.Error())
        ghErr, ok := userErr.(*github.ErrorResponse)

        if !ok {

❯ go test ./...
?       github.com/migueleliasweb/go-github-mock    [no test files]
--- FAIL: TestMockErrors (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x80 pc=0x12e6a1a]

goroutine 50 [running]:
testing.tRunner.func1.2(0x1334920, 0x15b67d0)
    .../golang/1.16.3/go/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc0002b2000)
    .../golang/1.16.3/go/src/testing/testing.go:1146 +0x4b6
panic(0x1334920, 0x15b67d0)
    .../golang/1.16.3/go/src/runtime/panic.go:965 +0x1b9
github.com/google/go-github/v37/github.(*ErrorResponse).Error(0xc00007c5f0, 0x0, 0x16)
    .../go/pkg/mod/github.com/google/go-github/v37@v37.0.0/github/github.go:707 +0x3a
github.com/migueleliasweb/go-github-mock/src/mock.TestMockErrors(0xc0002b2000)
    .../go-github-mock/src/mock/server_test.go:121 +0x1e3
testing.tRunner(0xc0002b2000, 0x13b3ef8)
    .../golang/1.16.3/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
    .../golang/1.16.3/go/src/testing/testing.go:1238 +0x2b3
FAIL    github.com/migueleliasweb/go-github-mock/src/mock   0.133s
FAIL

[migueleliasweb]

Hi @sebnow , thanks for reporting this.

I was aware of this problem but still couldn't find a proper way to go around this limitation. The go-github SDK doesn't really seem to use error wrapping but it does implement the error interface in its error returns. Please, correct me if I'm wrong here. Have a look at github.Client.BareDo implementation and how the error response is treated.

There is a weird interaction in the way the SDK processes error responses. I traced it down to the logic inside the function in go-github/v37@v37.0.0/github/github.go.CheckResponse.

errorResponse := &ErrorResponse{Response: r}
data, err := ioutil.ReadAll(r.Body)
if err == nil && data != nil {
    json.Unmarshal(data, errorResponse)
}

For some reason the errorReponse is overwritten if there is any data in the body and this makes the return not to have the property Response filled. This is a problem when calling .Error() as you already mentioned because since the property is nil, the fmt.Sprintf panics.

func (r *ErrorResponse) Error() string {
    return fmt.Sprintf("%v %v: %d %v %+v",
        r.Response.Request.Method, sanitizeURL(r.Response.Request.URL),
        r.Response.StatusCode, r.Message, r.Errors)
}

If you know how to go around this, please submit a PR and I will be happy to review it with you. In the meantime I will dig a bit more to see if I can find a way.

In the meantine, the type casting will solve the problem as you will have access to the real error type. Just don't call .Error() for now :joy: .

:+1:

[sebnow]

The go-github SDK doesn't really seem to use error wrapping

Yeah, I was referring to the calling code. E.g. I'm using error wrapping to return errors from the Github SDK, and then I want to test my code using go-github-mock. Currently I'm working around this by using errors.As and checking the Github error, but it's a bit more white-box than I'd like.

Just don't call .Error() for now

This also precludes the use of error wrapping and other error reporting solutions.

For some reason the errorReponse is overwritten if there is any data in the body and this makes the return not to have the property Response filled.

I see. I guess this isn't a bug in go-github-mock but rather in the SDK then. It seems that the case of parsing the response body is tested though, so that's odd.

[migueleliasweb]

@sebnow that test case was a nice find. I'm gonna try to see how that works.

[migueleliasweb]

@sebnow , I just wanted to let you know this is now fixed. 😃