Replaced defer statements with t.Cleanup in docs

[alvii147]

Hi there,

I just replaced the defer statements with t.Cleanup in the README, as it's generally not a good idea to use defer in tests. Lemme explain why.

Consider this test:

package main

import (
    "fmt"
    "net/http"
    "testing"

    "github.com/jarcoal/httpmock"
)

func TestHTTPGet(t *testing.T) {
    httpmock.Activate()
    defer httpmock.DeactivateAndReset()

    httpmock.RegisterResponder(
        "GET",
        "https://mock.httpstatus.io/400",
        httpmock.NewStringResponder(200, `{}`),
    )

    httpmock.RegisterResponder(
        "GET",
        "https://mock.httpstatus.io/404",
        httpmock.NewStringResponder(200, `{}`),
    )

    for _, url := range []string{"https://mock.httpstatus.io/400", "https://mock.httpstatus.io/404"} {
        t.Run(fmt.Sprintf("URL %s", url), func(t *testing.T) {
            resp, err := http.Get(url)
            if err != nil {
                t.Fatal("SendRequest failed", err)
            }

            if resp.StatusCode != 200 {
                t.Fatal("SendRequest returned non-200 status code", resp.StatusCode)
            }
        })
    }
}

I'm using the httpstatus service as mock URLs to send requests to. Sending requests to https://mock.httpstatus.io/{status_code} always returns a response with the specified status code.

The test above is using test tables to send GET requests to the following two endpoints:

• https://mock.httpstatus.io/400
• https://mock.httpstatus.io/404

Normally https://mock.httpstatus.io will respond with 400 and 404 respectively, but the test is mocking both to respond with 200. If we run this test, it does exactly that, and passes with no issues:

$ go test
PASS
ok      github.com/alvii147/httpmock-playground 0.550s

The problem shows up when we add the t.Parallel() directive to make the two test cases run in parallel:

package main

import (
    "fmt"
    "net/http"
    "testing"

    "github.com/jarcoal/httpmock"
)

func TestHTTPGet(t *testing.T) {
    httpmock.Activate()
    defer httpmock.DeactivateAndReset()

    httpmock.RegisterResponder(
        "GET",
        "https://mock.httpstatus.io/400",
        httpmock.NewStringResponder(200, `{}`),
    )

    httpmock.RegisterResponder(
        "GET",
        "https://mock.httpstatus.io/404",
        httpmock.NewStringResponder(200, `{}`),
    )

    for _, url := range []string{"https://mock.httpstatus.io/400", "https://mock.httpstatus.io/404"} {
        t.Run(fmt.Sprintf("URL %s", url), func(t *testing.T) {
            t.Parallel() // Parallelize test cases
            resp, err := http.Get(url)
            if err != nil {
                t.Fatal("SendRequest failed", err)
            }

            if resp.StatusCode != 200 {
                t.Fatal("SendRequest returned non-200 status code", resp.StatusCode)
            }
        })
    }
}

When we run this, we get:

$ go test
--- FAIL: TestHTTPGet (0.00s)
    --- FAIL: TestHTTPGet/URL_https://mock.httpstatus.io/400 (0.21s)
        main_test.go:36: SendRequest returned non-200 status code 400
    --- FAIL: TestHTTPGet/URL_https://mock.httpstatus.io/404 (0.21s)
        main_test.go:36: SendRequest returned non-200 status code 404
FAIL
exit status 1
FAIL    github.com/alvii147/httpmock-playground 0.468s

The logged output is telling us that https://mock.httpstatus.io actually responded with 400 and 404 respectively, which means we actually sent a request to https://mock.httpstatus.io, and httpmock failed to intercept it. But why?

The answer is the defer statement actually runs before the individual test cases when the test cases are parallelized. This is an important feature of Go's testing library, you can read more about it here.

For this purpose exactly, Go's testing library offers the t.Cleanup(). Any function passed into t.Cleanup() will only execute after all sub tests have executed and returned. If we replace the defer statement with t.Cleanup(), this issue no longer persists, and we are able to run tests in parallel properly:

package main

import (
    "fmt"
    "net/http"
    "testing"

    "github.com/jarcoal/httpmock"
)

func TestHTTPGet(t *testing.T) {
    httpmock.Activate()
    t.Cleanup(httpmock.DeactivateAndReset)

    httpmock.RegisterResponder(
        "GET",
        "https://mock.httpstatus.io/400",
        httpmock.NewStringResponder(200, `{}`),
    )

    httpmock.RegisterResponder(
        "GET",
        "https://mock.httpstatus.io/404",
        httpmock.NewStringResponder(200, `{}`),
    )

    for _, url := range []string{"https://mock.httpstatus.io/400", "https://mock.httpstatus.io/404"} {
        t.Run(fmt.Sprintf("URL %s", url), func(t *testing.T) {
            t.Parallel() // Parallelize test cases
            resp, err := http.Get(url)
            if err != nil {
                t.Fatal("SendRequest failed", err)
            }

            if resp.StatusCode != 200 {
                t.Fatal("SendRequest returned non-200 status code", resp.StatusCode)
            }
        })
    }
}

$ go test
PASS
ok      github.com/alvii147/httpmock-playground 0.507s

[maxatome]

Hi,

You are right, we always should use Cleanup instead of defer but this method appeared in 1.14 and httpmock is announced to run on 1.13, that's why the docs still use defer instead of Cleanup.

I plan to remove this restriction targeting at least 1.16 (3.5 year old), and use new features in internal code. During this switch, httpmock.Activate() signature could be changed to httpmock.Activate(...testing.TB) to automatically call Cleanup on first arg if passed without breaking existing code.

[alvii147]

@maxatome, ahh that makes sense, thanks for the clarification. If the switch from httpmock.Activate() to httpmock.Activate(...testing.TB) is planned soon, I'm happy to help out on that too!