Report: bug in TestTeamsClientSend re handling of response error

[atc0005]

Bug report

Dave Cheney's response to a golang/go GH issue I filed:

There is a bug in your test

2020/08/27 09:18:25 RoundTripper returned a response & error; ignoring response

This is the fix

(~/devel/go-teams-notify) % git diff
diff --git a/send_test.go b/send_test.go
index 6a1d6d1..8824f67 100644
--- a/send_test.go
+++ b/send_test.go
@@ -116,6 +116,9 @@ func TestTeamsClientSend(t *testing.T) {
                client := NewTestClient(func(req *http.Request) (*http.Response, error) {
                        // Test request parameters
                        assert.Equal(t, req.URL.String(), test.reqURL)
+                       if test.resError != nil {
+                               return nil, test.resError
+                       }
                        return &http.Response{
                                StatusCode: test.resStatus,

@@ -124,7 +127,7 @@ func TestTeamsClientSend(t *testing.T) {

                                // Must be set to non-nil value or it panics
                                Header: make(http.Header),
-                       }, test.resError
+                       }, nil
                })
                c := &teamsClient{httpClient: client}

I confirmed that the RoundTripper returned a response & error; ignoring response test output is present all the way back to v1.2.0.

References

This project (and original)

• Remote API error handling: expose response string #43
• 2ce144f
• 6db6217

Guides

• http://hassansin.github.io/Unit-Testing-http-client-in-Go

Official/upstream

• Panic with Go versions < 1.15.0 when reading (http.Response).Body golang/go#41071
• net/http: checks for nil Response or Response.Body in Client.send causing some internal Google tests to fail golang/go#38095
  • golang/go@2d77d33
  • golang/go@12d02e7
• https://godoc.org/net/http#RoundTripper
• https://godoc.org/net/http#Response
  • https://godoc.org/net/http#Response.Body

[atc0005]

These 4 contributed lines gave me no end of grief as I went back & forth over existing notes and official documentation. In the end, it was simpler than I was making it out to be.

Below are some typed paper notes in case they should prove useful to someone reading this later, perhaps landing here after a search query of some kind.

---

Dave Cheney provides fixes for a Go test to allow the test to complete without unexpected output. This goes back to when dasrick first implemented the test (v1.2.0?).

Dave's fix was:

• to check for a response error provided by the test table and if non-nil, return a nil http.Request and the response error provided by the test table
• to change the unconditional response error provided by the test table to a nil error value

But why?

The test code creates a new http.Client with the Transport field set to a custom RoundTripFunc type. This type accepts a http.Request and returns a http.Response and error type. Note: The custom type is a function type.

The method signature for RoundTripFunc.RoundTrip satisfies the http.RoundTripper interface. The implementation calls the receiver and passes it the http.Request argument.

So, a new http.Client is created for each table test entry. As part of creating the http.Client, the anonymous function is executed which returns a http.Response and the error type.

Before Dave's changes, a http.Response was returned unconditionally along with whatever error was defined for that table test entry.

The RoundTripper documentation notes that nil must be returned as the error value if a response is received. A non-nil error should be returned for failure to obtain a response.

To me this means:

• Failure to obtain a response is shown by returning a nil http.Response
• A non-nil error is paired with that nil http.Response

Thanks to Dave, the changes (once applied) properly implements RoundTripper behavior.

---

Oh, regarding the Go 1.14 vs Go 1.15 behavior, the Go documentation for http.Response notes:

The http Client and Transport guarantee that Body is always non-nil, even on responses without a body or responses with a zero-length body.

It sounds like Go 1.15 does the right thing by setting the http.Response Body field to empty if not already set. On the surface this means that Go 1.14 should do the same thing, but based on light reading it sounds like the "bug" isn't severe/important enough to warrant a backport.