Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddMatch #20

Closed
wshelly opened this issue May 19, 2017 · 8 comments · Fixed by #55
Closed

AddMatch #20

wshelly opened this issue May 19, 2017 · 8 comments · Fixed by #55
Labels

Comments

@wshelly
Copy link

wshelly commented May 19, 2017

AddMatch only use once in a go program?

package main

import (
"fmt"
"net/http"
"regexp"
"gopkg.in/h2non/gock.v1"
)

func test_a() {
defer gock.Off()

gock.New("http://httpbin.org").Get("/").
	AddMatcher(func(req *http.Request, ereq *gock.Request) (bool, error) {
	matched, err := regexp.MatchString("/aa/[A-Za-z0-9]{6}/ii/[A-Za-z0-9]{6}/calculator", req.URL.Path)
	return matched && "GET" == req.Method, err
}).
	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"))

}

func test_b() {
defer gock.Off()
gock.New("http://httpbin.org").
Get("/application").
Reply(200).
SetHeader("Server", "gockbin")

res, err := http.Get("http://httpbin.org/application")
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"))

}

func main() {
test_a()
test_b()
}

@wshelly
Copy link
Author

wshelly commented May 19, 2017

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"))

}`

@h2non h2non added the question label Jul 18, 2017
@JusbeR
Copy link

JusbeR commented Aug 14, 2018

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
Copy link
Owner

h2non commented Aug 14, 2018

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

@JusbeR
Copy link

JusbeR commented Aug 15, 2018

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?

@JusbeR
Copy link

JusbeR commented Aug 15, 2018

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.

@ods94065
Copy link

ods94065 commented Oct 13, 2018

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
Copy link
Owner

h2non commented Oct 14, 2018

@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
Copy link

Funcan commented Nov 16, 2018

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants