Native Mocking a la FakeWeb

[mtodd]

Hey, just to continue the conversation somewhere where we can keep things organized!

You mentioned wanting to add mocking support (a la FakeWeb) directly into Excon. That sounds great to me!

I'd like to try my hand at writing the feature, but I'd like your input on how you'd like the API to look et al. Whatever kind of requirements you have I can write as specs and begin implementing.

[qrush]

+1!

[geemus]

Alright, I just researched available stuff and did a naive first pass here: 2e5bbec

Basic interface is Excon.stub(request_params, response_params)

It is assumed that if you omit a request param it will match any value, so rather than specifying :method => :any or something you just don't specify any at all. Also the stubs are added to an array, so when matching it will return stubbed responses in a FIFO manner (so you could, for instance, stub a series of progressively less restrictive things to catch more stuff, ie the last stub could be {} to match all requests).

Also, I omitted functionality to explicitly disable real requests (but it should be easy enough to add). I think this is a pretty reasonable implementation and should cover most use cases, but I'm certainly interested to hear if I missed anything. Once we have it ironed out I can reach out to fakeweb/webmock/vcr/etc to give them integration points as needed.

Sorry for the delay on this, I had thought I would work on it at MountainWest if nothing else, but I'm blocked on some of my other tasks so I figured now was as good a time as any. That said, I'd love some feedback when you get a chance so we can solidify and move toward releasing something.

[geemus]

Also, I didn't do anything to allow for regex as segment. This is counter to the way other services implement it. This would slow down the comparison stuff, but probably worth it for the flexibility.

And, rather than matching :headers hash vs :headers hash it should perhaps do a sub-match to just compare specified headers vs existing (so repeat the top level process at the header level and assume omitted ones match anything). This should probably be done prior to a release (as without it matching on headers would be a pain in the butt).

[atmos]

It seems weird to me to stub it at the top level instead of the instance level. Something like the following seems a little more sane to me.

connection = Excon.new('http://geemus.com')
connection.stub(request_params, response_params)
connection.request(:method => 'GET')

Using it on an instance seems slightly more sane to me but doesn't necessarily address some of the toplevel methods that excon exposes. What's there now is basically doing the same thing as mocha's any_instance method on connections but seems like those stubs would exist on Excon for the life of the process. I might be missing something here though.

We do stuff similar to this now but end up reading responses from files since it tends to cut down on the chunking involved with understanding the context around what's being stubbed. Sane defaults for response status etc would be nice if you passed in a string for the response body.

connection = Excon.new('http://geemus.com')
connection.stub(request_params, File.read("/path/to/file"))
connection.request(:method => 'GET')

Also, Gemfile.lock doesn't belong in gems. :)

[geemus]

Yeah, top level vs not was on the fence for me (because of the top level requests). Connection level may be a better fit and it is perhaps reasonable to just say that if you want stubbing you have to use that interface.

Currently the stubs would exist for life of the process. Some other frameworks pop them off the list once they are used, but I wasn't sure I liked that. Otherwise there could be some way to unstub, or if it is at the instance level you just make sure to use a different connection for the stuff with different stubs (I think I like that notion best).

I hadn't considered the file/string side of response stuff (I wasn't sure if I was going to tackle this directly or just allow for the right stuff and defer to vcr or the like). I can certainly see how defaults for other parts of the response would be handy (maybe {:status => 200, :headers => { 'Location' => host }} or some such)

I thought with the Gemfile.lock you wanted it (for development) so that everybody would get the same resolution. It probably should not be distributed as part of the actual gem package though, which is perhaps what you meant.

Last but not least, thanks for the feedback!

[geemus]

Moved stubs to connection level, as that solves other issues and makes more sense:
f2decdd

[geemus]

Also redid the headers part as I mentioned above: 833b69f

[trevorturk]

Re: Gemfile.lock being in gems -- this is new to me, but Yehuda talks about it here:

http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

[geemus]

trevorturk: thanks for the info. I'll read over it and/or bug yehuda until I figure it out.

[trevorturk]

I just changed the way carrierwave works to gitignore Gemfile.lock -- but I'm not 100% sure if it's better.

I had a hard time with pull request merge conflicts due to Gemfile.lock a few times, so I figure that's enough of a reason to err that way.

[geemus]

Alright, after a good deal of feedback and thought I did some rework of this yesterday. I'm liking the new format pretty well and it seems to fit in well and be powerful. Here is an overview of things as they stand.

Mock for a connection means that it will either match a stub and return that value or raise a StubNotFound error. The main way to turn this on/off is at the global level:

Excon.mock = true # default all new connections to mocked
Excon.mock = false # this is the starting state, and allows connection to be normal

You can also override the default at the connection level:

connection = Excon.new('http://www.example.com', :mock => true)
connection = Excon.new('http://www.example.com', :mock => true)

Or even at the request level if you need that granularity:

connection.request(:method => :get, :mock => true)
connection.request(:method => :get, :mock => false)

Now then, as for defining stubs. These are at the global level again. The first argument is a hash of request params to match against (currently does string matching, but regex is likely to be added at some point). Any values omitted from this hash are considered to match all (ie if you omit :method it would match all methods or if you pass an empty hash it will match all requests).

Defining the response value happens in one of two ways, first is by returning a hash to build the response from as the second argument:

Excon.stub({}, {:body => '', :headers => {}, :status => 200})

The other option is to pass a block (which will receive the request params that were passed in).

Excon.stub({}) do |request_params|
  # do stuff with request_params and then return hash for response
  {:body => '', :headers => {}, :status => 200}
end

I think that sums up the changes currently and I believe does a good job of covering what I understand to be the main use cases while remaining flexible for some less common cases when needed. Would love any additional feedback to help polish anything that has rough edges and then once it seems good I'll write up some docs and release.

Thanks!

[geemus]

Calling this good, given the relative lack of further questions/concerns. If they do appear please open new more focused issues. Thanks!

[mattsnyder]

@geemus Would be great if the above documentation was added to the README. If it wasn't for Google, I would have missed out on Excon.mock = true (immensely useful!) In the README it only states it can be set by passing :mock => true to the connection or the request.

[geemus]

@mattsnyder - good call. The even more up to date method would be Excon.defaults[:mock] = true. You can actually set any option there and it will be overridden if there is a different connection value which will in turn be overridden if there is a request option. That way you have (hopefully) the most flexibility possible. Would you be interested in a quick pull request to add this info for us? Thanks!

[mattsnyder]

@geemus Just discovered Excon.defaults thanks to the deprecation message. Sure thing on the PR!

[geemus]

@mattsnyder - awesome, just let me know if you have any questions or anything. The readme could definitely use much more love.

[mattsnyder]

@geemus Actually I do, how accurate is the current info on stubbing? In other words I could append the info above to the existing documentation on stubbing or replace. Thoughts?

[geemus]

@mattsnyder - Good question. I think it could probably be clearer and more detailed, but it does not appear to be inaccurate. Hope that answers the question. Somebody with more perspective than me could probably write it much better in general, so if you want to take a stab at replacing I'd welcome it. But if you prefer to just append/prepend it where it seems like it should fit that would also be great.

[mattsnyder]

@geemus I just appended to the doc for the now and referred back to this PR. That should help for now.

As I work more with speccing Excon I'll put together another PR with updated information.

For now here is the pull request.

Thanks again for all your work on this project, it's been very helpful!

[geemus]

@mattsnyder - my pleasure, thanks for pointing out what isn't clear enough. Should definitely help others get done what they need to.

[contentfree]

It's still not clear how to successfully mock Excon calls. I've added the rspec config to my spec_helper.rb and now my specs all fail with Excon::Errors::StubNotFound. Obviously I'm missing something with Excon.stub(), but I just want to make a global stub (maybe...?) Can there be one more round of clarification on stubbing?

[geemus]

@contentfree - sure. I'll do my best. Could you provide a bit more detail on what you are mocking and how you are calling it that is erroring so I can try to make sure I know what is unclear? Thanks!

[contentfree]

I'm using Excon indirectly via Mandrill's API gem (I got a lot of
emails... Heh)

I'll probably open an issue with them because they ought to handle test
environments better.

However, for now, where I use their gem - which throws the errors when I
set mock = true - I'm rescuing the StubNotFound error.

It would be nice to have a config option similar to the mock option that
would result in no exceptions being raised (maybe just returning a fake OK
response or even just return whatever the option is set to?)

On Aug 14, 2013, at 7:39 PM, Wesley Beary notifications@github.com wrote:

@contentfree https://github.com/contentfree - sure. I'll do my best.
Could you provide a bit more detail on what you are mocking and how you are
calling it that is erroring so I can try to make sure I know what is
unclear? Thanks!

—
Reply to this email directly or view it on
GitHubhttps://github.com//issues/29#issuecomment-22681878
.

[mattsnyder]

@contentfree Can you provide some sample code on how you are calling it so we can help you figure out how to mock it?

[geemus]

@contentfree - you could stub "the rest", ie if none of the existing stubs match, simply return this (instead of erroring). Anything you don't specify in a stub is assumed to match, so you could do this:

Excon.stub({}, { :body => 'fallback', :status => 200 })

Existing stubs will be gone through first (stubs are checked in order), but once it gets through them it will hit this and return the simple 200/fallback message. FWIW. That said, if we understand what is up a bit better we can probably tailor the solution and/or help fix the mandrill gem (afraid I don't have much familiarity with mandrill).

[contentfree]

As an aside, that paragraph and example would be a great addition to the
docs.

I'll look at the Mandrill gem to see if they're open to suggestions.

On Aug 15, 2013, at 7:22 AM, Wesley Beary notifications@github.com wrote:

@contentfree https://github.com/contentfree - you could stub "the rest",
ie if none of the existing stubs match, simply return this (instead of
erroring). Anything you don't specify in a stub is assumed to match, so you
could do this:

Excon.stub({}, { :body => 'fallback', :status => 200 })

Existing stubs will be gone through first (stubs are checked in order), but
once it gets through them it will hit this and return the simple
200/fallback message. FWIW. That said, if we understand what is up a bit
better we can probably tailor the solution and/or help fix the mandrill gem
(afraid I don't have much familiarity with mandrill).

—
Reply to this email directly or view it on
GitHubhttps://github.com//issues/29#issuecomment-22705846
.

[geemus]

@contentfree - yeah, definitely looking to improve docs. So far in https://github.com/geemus/excon/blob/master/README.md#stubs the 3rd example is meant to (more or less) map to what I explained here (it is a bit narrower in that it limits to get method stuff). Would just removing the get method make it clearer? Is some of the wording there ambiguous or confusing that I can improve? Just trying to make sure I make the right fix. Thanks!

[contentfree]

@geemus - I think that might make it clearer, but more useful would be a mention nearer the Rspec example: Add the "fallback" stub after the defaults[:mock] = true line (along with a comment about adding more specific stubs where needed for specific tests).

I could always go click the Edit button on that page…

[geemus]

@contentfree - yeah. I had somewhat assumed that error was preferable to default stub (so that you could figure out what you still needed to stub). I would certainly welcome a pull request on this if you are willing.

[contentfree]

I made a pull request for the doc addition

[contentfree]

@geemus - In light of your last comment, should a further note be added to the docs regarding the price of a global stub? (That is, losing potential insight to where & what might need proper stubbing.)

A global stub is probably useful when Excon is a dependency of a dependency and I'm not using it directly. However, my intuition is that my project's dependency (in my specific case, the Mandrill API gem) ought to provide a method to be put into "test" mode...

[geemus]

@contentfree - ah, understanding more now. Yeah, it shouldn't be your responsibility to write mocks for dependencies that far out (in my opinion anyway). If you decide to try and make mandrill do the right thing and need help, just let me know. Happy to help and thanks again for the pull!

[geemus]

@contentfree - yeah, clarifying that seems of value.