AddMatch about gock

[wshelly]

I use Filter solve this problem

`func filter(req *http.Request) bool {
matched, _ := regexp.MatchString("/aa/[A-Za-z0-9]{6}/ii/[A-Za-z0-9]{6}/calculator", req.URL.Path)
return matched && "GET" == req.Method
}

func test_a() {
defer gock.Off()

gock.New("http://httpbin.org").Filter(filter).
	Reply(204).
	SetHeader("Server", "gock")

res, err := http.Get("http://httpbin.org/aa/123456/ii/123456/calculator")
if err != nil {
	fmt.Errorf("Error: %s", err)
}

fmt.Printf("Status: %d\n", res.StatusCode)
fmt.Printf("Server header: %s\n", res.Header.Get("Server"))

}`

[JusbeR]

It does this probably because we modify the DefaultMathcer. I tried to add DefaultMatcher = NewMatcher() to OffAll, to check my theory and to me the behavior becomes more logical. But I speak only about this one case. Dunno if changing this breaks some other consistent behavior that I am not aware of.

[JusbeR]

Same here. So the matchers somehow live forever in your stack even if you call gock.Off()? Is this a feature or a bug? I could work around by using filters too.

[h2non]

@JusbeR It's intended. If you want to clean any unmatched request from the mock store, you should call gock.OffAll() instead.

[JusbeR]

Hmm, it feels like I am not getting something here now. If I run this:

package main

import (
	"log"
	"net/http"

	"gopkg.in/h2non/gock.v1"
)

func NewGockClient() *http.Client {
	c := &http.Client{Transport: &http.Transport{}}
	gock.InterceptClient(c)
	return c
}

func failingMatcher() {
	log.Println("Failing matcher")
	defer gock.OffAll()
	gock.New("https://example.com").
		Get("/api").
		AddMatcher(func(req *http.Request, ereq *gock.Request) (bool, error) { return false, nil }).
		Reply(200).
		BodyString("{}")

	c := NewGockClient()
	req, _ := http.NewRequest("GET", "https://example.com/api", nil)

	_, err := c.Do(req)
	if err != nil {
		log.Println("Failed:", err)
	} else {
		log.Println("Ok")
	}
}
func noMatcher() {
	log.Println("No matcher")
	defer gock.OffAll()
	gock.New("https://example.com").
		Get("/api").
		Reply(200).
		BodyString("{}")

	c := NewGockClient()
	req, _ := http.NewRequest("GET", "https://example.com/api", nil)

	_, err := c.Do(req)
	if err != nil {
		log.Println("Failed:", err)
	} else {
		log.Println("Ok")
	}
}
func main() {
	noMatcher()
	failingMatcher()
	noMatcher()
}

I get:

2018/08/15 08:47:34 No matcher
2018/08/15 08:47:34 Ok
2018/08/15 08:47:34 Failing matcher
2018/08/15 08:47:34 Failed: Get https://example.com/api: gock: cannot match any request
2018/08/15 08:47:34 No matcher
2018/08/15 08:47:34 Failed: Get https://example.com/api: gock: cannot match any request

Which should not happen after the OffAll call?

[ods94065]

I just ran into this, and agree that the problem is that AddMatcher() is modifying DefaultMatcher. This is because, if you call gock.New(), which calls NewMock() under the hood, you'll get a mock with matcher set to DefaultMatcher. Adding a matcher results in DefaultMatcher.Add(myFunc), which will modify DefaultMatcher by appending myFunc to its array of match functions. The effect will be felt by any subsequent tests that call gock.New().

The result: the presence of an AddMatcher() in one test is breaking subsequent tests, irrespective of whether gock.Off() or gock.OffAll() is used.

My workaround for now is to change this:

gock.New(someURL).
    Get(somePath).
    AddMatcher(someMatchFunc).
    Reply(...)

to this:

gock.New(someURL).
    Get(somePath).
    SetMatcher(gock.NewMatcher()).
    AddMatcher(someMatchFunc).
    Reply(...)

This assigns the mock a brand-new matcher object. The new matcher has the same battery of matching functions that DefaultMatcher does out of the box, which is what I want. Furthermore, because the mock now has its own local matcher object, AddMatcher only modifies that mock in isolation.

Though this seems to work, I am not happy with this solution – few programmers are going to expect AddMatcher to be modifying global state out of the box, and SetMatcher() is very easy to forget! Some options I'd like to put on the table to fix this:

1. Change NewMock() to call NewMatcher() instead of using DefaultMatcher. Downside: anybody who expected NewMock() to use DefaultMatcher prototypically would be broken.
2. Change NewMock() to clone DefaultMatcher – e.g. using DefaultMatcher.Get() to get the array of matching funcs, and passing those to a new MockMatcher. This means any changes you make to DefaultMatcher will apply to new mocks going forward, which preserves that existing behavior. This seems to me the least elegant, but perhaps the most backwards-compatible, solution.
3. Change the semantics of Matcher to make it immutable – Add(), Set(), and Flush() give you a new Matcher instead of modifying the existing one, a la append() and slices. This is, of course, a breaking API change.

Thoughts?

[h2non]

@ods94065 Thanks for the comment here. I review the issue and your points makes sense to me. I don't recall why I have decided to modify package-level state to be honest, but reviewing it now, I think it was a design mistake.

Options 1 and 2 seems reasonable to me and it's pretty much what I had in mind. Will push a fix soon. I might have some negative side-effects to some users, but I will not consider it a breaking change.

The good thing about the option 2 is that users that want to have globally shared matches can still rely on it by using gock.DefaultMatcher methods.

[Funcan]

@h2non Any progress on this? I just hit the same bug, and I'm happy to spend some time fixing it using one of the above suggestion if that helps