gock icon indicating copy to clipboard operation
gock copied to clipboard

Allow multiple matchers

Open stevenmatthewt opened this issue 7 years ago • 10 comments

better diff view

I had a slightly confusing moment the other day when I was using gock. I had code very similar to the following:

gock.New("http://foo.com").
    Post("/bar").
    BodyString("bar").
    BodyString("baz").
    Reply(201).
    BodyString("intercepted")

res, err := http.Post("http://foo.com/bar", "application/json", bytes.NewBuffer([]byte(`{"baz":"foo"}`)))
// ...

To my surprise, gock did match the request. The source code makes the issue pretty clear: the second call to BodyString() just overwrote the first. This didn't seem like intuitive behavior to me.

This PR aims to remove this confusion by allowing multiple matchers (of the same or different types). I'm not necessarily finished with the PR; I think there might be a more generic/cleaner way of accomplishing this. But I wanted to go ahead and publish the PR so I could get some feedback on the general idea.

stevenmatthewt avatar Sep 20 '17 16:09 stevenmatthewt

This looks promising :)

tomas-fp avatar Sep 20 '17 17:09 tomas-fp

After a quick review, it looks great. Thanks for adding the test cases too.

My only concern is that this introduces a different behavior that can also affects to other matchers, not just the body, so I want to think about it before introduce this feature.

Thanks for the PR!

h2non avatar Sep 20 '17 22:09 h2non

In essence, I feel like the final goal of this implementation is introducing a different approach of what is currently implemented in gock, where request.go method calls would basically register a matcher function in the registry for later matching.

Overtime I realized that this approach would be more flexible, so I have implemented it in pook, a sort of Python port of gock.

I'm not close to this port this approach into gock if that makes it more flexible (perhaps in v2).

h2non avatar Sep 20 '17 22:09 h2non

I'm on vacation next week, so I'll probably have some time to do more work on this PR. I'm just going to mess around a bit and see if I can make some improvements (maybe even take a stab at implementing your concept of a deferred 'matcher function'). I'll keep you updated.

No worries if you don't want to merge. I'm having fun just messing around in some code that's not work related :)

stevenmatthewt avatar Sep 21 '17 12:09 stevenmatthewt

Cool, any help would be very welcome. Feel free to update me here with your ideas or approches, so we can converge somehow.

h2non avatar Sep 21 '17 12:09 h2non

Any updates here? Just curious :)

h2non avatar Sep 27 '17 07:09 h2non

Just tagging in here but I'am down to contribute as well =)! I like the your idea about registering matcher functions for later use.

edwinthinks avatar Sep 27 '17 14:09 edwinthinks

Finally getting some time to start working on the rehaul of the matcher functions. I'll be making a separate PR when I've got something to share.

stevenmatthewt avatar Sep 27 '17 15:09 stevenmatthewt

Any update here?

h2non avatar Dec 21 '17 12:12 h2non

Hey @h2non I looked again at this PR and had a few thoughts about how to go forward from here. I don't think we should be defining multiple body matchers in a single matcher and I don't think it should be overwriting the previous definition either. Ideally, gock.New() would only register a very specific HTTP request. I am not 100% sure how you handled it in pook

I'll take a swing at this soon :)

edwinthinks avatar Dec 25 '17 20:12 edwinthinks