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

Already on GitHub? Sign in to your account

Implicit extension API (i.e. HTTP client lib must be required before webmock) is confusing #72

Closed
myronmarston opened this Issue Feb 1, 2011 · 14 comments

Comments

Projects
None yet
6 participants
Collaborator

myronmarston commented Feb 1, 2011

Currently, WebMock decides to monkey patch (or not) the supported HTTP client libraries by detecting the presence or absence of the necessary constants. This works, but it is a confusing, implicit API because it relies upon the require order. If a project requires WebMock before the HTTP library, then the library will not be monkey patched by WebMock and it will not be able to stub requests made through it. This is even more of a problem when using something like Bundler since Bundler will require your gems, and there isn't an explicit way to make it require things in a particular order. I've had a couple VCR users open issues about how a particular HTTP library wasn't working with VCR and the problem was that they weren't requiring the HTTP library before VCR required WebMock. Wycats has also blogged about the problems with this approach.

I'd like to suggest that WebMock should move away from this implicit API. Instead, here's how I think it would ideally work:

  • WebMock would provide an explicit API like WebMock.stub_http_libs :net_http, :patron.
  • This would require the necessary WebMock monkey patches (each of which requires the necessary HTTP client gem) for the configured HTTP libraries; the various extension files would not all be required automatically, as they are now.
  • Switching directly to this API would essentially be a breaking change, so for now the existing way should continue to work with a deprecation. The best way I can come up to do this is to add some code to WebMock.stub_request so that the first time it is called, it checks to see if any http libs have been configured, and if not, it prints a deprecation warning and calls WebMock.stub_http_libs with the libraries that have already been loaded based on the presence of the constant. Whenever you release the next major release (WebMock 2.0.0) you could remove this deprecation code.

What do you think?

Contributor

phiggins commented Feb 1, 2011

I like this idea, but I don't understand why adding the new behavior has to mean removing the old. Couldn't webmock attempt to mock everything when it is require'd and provide the WebMock.stub_http_libs interface?

I suppose this could be made a little nicer by having the first call to WebMock check if any libs were explicitly loaded, and if not attempting to mock all of them.

Collaborator

myronmarston commented Feb 1, 2011

Couldn't webmock attempt to mock everything when it is require'd and provide the WebMock.stub_http_libs interface?

I guess it technically could, but that makes it too easy to unknowingly use the implicit interface (i.e. if you didn't even know about the explicit stub_http_libs interface) and may cause problems for you later on if require order on your project changes. The point of the explicit interface is to have a single authoritative way to tell WebMock which http libs to mock. Keeping the implicit interface goes against the entire purpose of the explicit interface.

Owner

bblimke commented Feb 3, 2011

I like the fact it's enough to require webmock in test environment to ensure tests
are isolated from network and I would like to avoid adding additional declarations.

I agree that it's annoying that stubbing doesn't work if libraries are required after webmock is required and it's very difficult to figure out, what's wrong.

If there is no better way, WebMock.stub_http_libs makes sense, but as you wrote in version 2.0.
There are some other things I would like to change in next major version.

I think the approach with mixing implicit and explicit interface will be confusing. If WebMock.stub_http_libs would be added, then I would current default behaviour should be removed imho.

Collaborator

myronmarston commented Feb 3, 2011

I like the fact it's enough to require webmock in test environment to ensure tests
are isolated from network and I would like to avoid adding additional declarations.

I like this fact, too, and if it always worked with no gotchas, I'd be in favor of keeping it how it is.

If there is no better way, WebMock.stub_http_libs makes sense, but as you wrote in version 2.0. There are some other things I would like to change in next major version.

If it's a change that will improve WebMock, and we can implement it in a backwards-compatible way (i.e. with the deprecation warning I mentioned above), then why do we need to wait until 2.0 to implement it?

I thought of another possible improvement that would work differently and should solve this problem: at the top of each of the files that monkey patches the HTTP libs, you could put something like this:

begin
  require 'httpclient'
rescue LoadError
  # the user must not be using httpclient
end

Basically, attempt to require each library before monkey patching it. This would solve the issue of require order mattering, and there would be no need to explicitly configure WebMock. The downside is that some users may have an HTTP client gem installed (i.e. for another project) and not want to use it on this project, and WebMock will load it, which will add bloat to the memory process. That said--I imagine that would be a pretty rare occurrence these days since most people use bundler and/or RVM gemsets to provide gem isolation, so if the gem is available, there's a good chance they want to use it.

Owner

bblimke commented Feb 3, 2011

I misread the thing about deprecation warning in your first comment, it makes sense now :)
I agree both possibilities seem fine for WebMock 1.x
I'm not sure which option is better yet, but I think I prefer the second option. It should't cause any problems in 98% of projects.

Contributor

benpickles commented Feb 4, 2011

One of the features of WebMock is that it allows you to test behaviour not implementation and having to explicitly tell WebMock to stub particular libraries is taking a small step away from this in my opinion.

How about storing an array of the successfully stubbed libraries, then when stub_request is called (either for the first time or every time) it compares present libraries that haven't been stubbed against this array. If a library is present that hasn't been stubbed then output a warning - and stop the tests if connections have been disabled with WebMock.disable_net_connect!.

phoet commented Aug 2, 2011

please have a look at this issue for one of the gotchas: https://github.com/myronmarston/vcr/issues/41#issuecomment-1709357

Owner

bblimke commented Aug 2, 2011

Performing any kind of checks on first WebMock.stub_request would be a good solution if webmock was only about stub_request, but it's also registering executed requests, verifying expectations (of real, unstubbed requests too), enabling and disabling network connections and invoking callbacks (so VCR recording still wouldn't work unless same check is performed when a new callback is registered)
Only solution that would currently work for all use cases of webmock is this one:

begin
  require 'httpclient'
rescue LoadError
  # the user must not be using httpclient
end

for each adapter.

Collaborator

myronmarston commented Aug 2, 2011

@bblimke: My preference would still be an explicit API, rather than having WebMock attempt to require the http client libs, but I know some WebMock users may not like that.

Owner

bblimke commented Aug 3, 2011

@myronmarston: Advantages of explicit API are clear to me. Making this change in WebMock 1.x is to risky
as people may not know about this change to explicit API plus implicit api is very convenient in many cases.

I'm thinking whether to use "force require" idea in each http adapter in version 1.x or it will create problems.

Owner

bblimke commented Aug 4, 2011

I'll add force require in version 1.7.0, and if it causes problem to people, I'm going to add configuration to switch it off in the next release.

Owner

bblimke commented Aug 8, 2011

Added in master.

@bblimke bblimke closed this Aug 8, 2011

pboling commented Nov 30, 2011

I believe this change broke my usage of WebMock + VCR for versions >= 1.7. We record SOAP interactions. With WebMock 1.6.4 we used this as our spec/support/vcr.rb (simplified):

# This needs to be loaded before webmock is loaded, soap4r uses httpclient
require 'soap/streamHandler'   
# Configure VCR
VCR::Config.stub_with :webmock

Any way around this impasse?

Reference to my issue: https://github.com/myronmarston/vcr/issues/109

Owner

bblimke commented Dec 2, 2011

No idea why 1.7.0 would break it. If soap4r already loads httpclient, then this change shouldn't matter as webmock
would not load it twice.
If you discover any more details please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment