Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Update to work with the latest version of em-http-request (green specs) #111

Merged
merged 7 commits into from

5 participants

@sdhull

I've done a lot of work to get WebMock working with the latest version of em-http-request on my fork.

The internal structure of em-http-request has changed substantially since WebMock was made to work with it. I recommend that you either pull this into a separate branch (eg, "em-http-request-edge" or similar), or spin off a branch for legacy em-http-request support (eg, "em-http-request-legacy").

It's also worth noting that the response.message is no longer supported by em-http-request. It's hardcoded to "unknown". So I added a hack to skip those specs for em-http-request.

Also worth noting is that I've fixed a "double resume" exception when being used in conjunction with em-synchrony (complete with spec to prove it ;)

I noticed that EM::HttpClient#build_request_signature was not idempotent, and it was causing issues for me. I really didn't feel like figuring out how to make it idempotent so I put it behind a memoized method and called it a day.

Last, but not least, I prevented EventMachine from connecting to the remote server at all when there is a matching stub in the stub registry. I'm not sure whether or not this was the case before, but specs were passing even when it was permitting connections to remote servers (which didn't seem ideal to me).

-Steve

@phiggins

Is there a reason this was added to the Gemfile, or just left over from your development process?

Left over from development. Is there some reason to remove it? I guess it isn't technically necessary, but it is certainly useful during development.

I don't use it, and it doesn't make sense to me to add dependencies that aren't necessary.

Fair enough. What makes the most sense? Should I add a commit to remove it?

Yeah, another commit removing it is probably best.

@phiggins

Is this a limitation of WebMock, of em-http-request, of the techniques used by WebMock to mock em-http-request, or something else? Do separate bugs need to be filed for this?

It's a limitation of em-http-request. See this line/method:

https://github.com/igrigorik/em-http-request/blob/master/lib/em-http/client.rb#L210

Wow, you're right, this must be a regression in em-http-request.

I submitted a bug to em-http-request: igrigorik/em-http-request#118

@sdhull

@phiggins: are you waiting for implementation of response.reason to accept this pull request, or simply still reviewing?

@phiggins

@sdhull: I don't have commit rights on WebMock, I'm just a contributor. I would really like to see this make it into a release soon though.

@jcf, @myronmarston, @bblimke: What do you guys think?

@myronmarston
Collaborator

I'm in favor of keeping webmock working with all the HTTP client it supports, but I'm a very occasional committer to webmock--I mostly just keep it working with VCR.

With my current work load at work and the other open source projects I'm involved in, I don't have the bandwidth to properly review and/or merge this in, so I'll leave it up to @jcf or @bblimke to do so.

@phiggins

@sdhull: I've been playing around with your em-http support this weekend. Nice work!

I noticed that using em-http's middleware wasn't supported. Here's some failing test cases:

https://gist.github.com/ad47b30880b6311f5c0b

@sdhull

Nice test cases. I'll try and find some time to make those pass in the next few days. I'll also take out the hack that skipped the response.reason test cases (Ilya has fixed that in em-http-request master).

@phiggins

Had a minute of downtime in between festivities to take a crack at this. The specs I posted before were busted, so I fixed those and the response middleware spec passed out of the box. The request middleware spec was a little trickier, but I got it to pass as well: https://gist.github.com/ad47b30880b6311f5c0b

@sdhull

Awesome! I applied your diffs -- I assume that since you linked to diff files, you wanted me to apply them and add a commit to this. So I've done that, except I haven't pushed yet. Just let me know if that was your intention and I'll push that commit.

As for this other issue -- response.reason -- I was looking at the implementation in em-http-request (that was recently committed) and it turns out that Ilya opted to resolve the reason string from the status code instead of parsing it out of the response. This means it is (probably) still incompatible with standard webmock response reasons (since I believe you can stub response code and reason separately).

@phiggins

Awesome! I applied your diffs -- I assume that since you linked to diff files, you wanted me to apply them and add a commit to this. So I've done that, except I haven't pushed yet. Just let me know if that was your intention and I'll push that commit.

Yeah, applying the diffs should be fine. I couldn't figure out how to send a pull request to your branch without deleting mine first and didn't want to spend all day figuring it out.

As for this other issue -- response.reason -- I was looking at the implementation in em-http-request (that was recently committed) and it turns out that Ilya opted to resolve the reason string from the status code instead of parsing it out of the response. This means it is (probably) still incompatible with standard webmock response reasons (since I believe you can stub response code and reason separately).

Having em-http specific code isn't terrible, in my opinion, but should probably be avoided where we can.

@sdhull

Having em-http specific code isn't terrible, in my opinion, but should probably be avoided where we can.

I was thinking that we wouldn't be able to support stubbing of the response "reason" because the "reason" isn't parsed out of the response text, but rather looked up from a hash (ie, 200 => "OK"). So if I set the response text to something like:

HTTP/1.1 200 ROCKIN

em-http-request will say the response.status is 200 and the response.reason is "OK" (because they are mapped from the status code instead of parsed from the response).

See this file in em-http-request to see what I mean.

But I realized today that we can update the mapping hash temporarily, but the problem with this is two-fold: (1) it's not threadsafe, and (2) it would simulate a behavior that would not occur in real operation. That is, if a real webserver returned "200 ROCKIN", em-http-request will still say the response "reason" is "OK" -- not "ROCKIN". So to support stubbing of response reason in WebMock for em-http-request (without further changes to em-http-request) would be somewhat dishonest.

That said, I have code to make it work locally. I just don't think it should be committed. I think it's time to fork and fix http_parser.rb so that it parses the reason out of the response text, so that Ilya can do it right in em-http-request.

@phiggins

Having more complicated code in WebMock to properly mimic the http library in use is my preference, but as I said before I'm not a maintainer so take my opinion with a grain of salt.

If this means having (pseudo) code similar to:

if http_lib_in_use == :em_http
  text.should == 'foo'
else
  text.should == 'bar'
end

... then I think that's what should be done. WebMock already has to be pretty intrusive to get the desired functionality, but temporarily chaging constants in the mocked library goes a bit far for my taste.

@qrush

does this work with em-http-request 1.0?

Tested and working against 1.0.0.beta.4, as you can see from the gemspec:

s.add_development_dependency 'em-http-request', '>= 1.0.0.beta.4'
@bblimke
Owner

Just had some time to through all your commits @sdhull. Wow, that's an amazing work!
Looks like you and @phiggins found all the possible use cases.
I'd like to release it asap.

I'd like to merge it into master but temporarily still be compatible with em-http-request 0.3.0.
I'm going to keep 2 adapters for now, em-http-request adapter and em-http-request_lt_10 adapter, depending
on which constants are defined if that's possible.

@bblimke bblimke merged commit 4527962 into bblimke:master
@sdhull

Sweeeet! Glad you got around to reviewing this and pulling it in. Good stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  Gemfile
@@ -5,6 +5,7 @@ gemspec
group :development do
gem 'guard-rspec'
gem 'rb-fsevent'
+ gem 'em-synchrony', '0.3.0.beta.1', :require => false
end
platforms :jruby do
View
168 lib/webmock/http_lib_adapters/em_http_request.rb
@@ -1,59 +1,98 @@
-if defined?(EventMachine::HttpRequest)
+if defined?(EventMachine::HttpClient)
module EventMachine
- OriginalHttpRequest = HttpRequest unless const_defined?(:OriginalHttpRequest)
+ OriginalHttpClient = HttpClient unless const_defined?(:OriginalHttpClient)
+ OriginalHttpConnection = HttpConnection unless const_defined?(:OriginalHttpConnection)
+
+ if defined?(Synchrony)
+ # have to make the callbacks fire on the next tick in order
+ # to avoid the dreaded "double resume" exception
+ module HTTPMethods
+ %w[get head post delete put].each do |type|
+ class_eval %[
+ def #{type}(options = {}, &blk)
+ f = Fiber.current
+
+ conn = setup_request(:#{type}, options, &blk)
+ conn.callback { EM.next_tick { f.resume(conn) } }
+ conn.errback { EM.next_tick { f.resume(conn) } }
+
+ Fiber.yield
+ end
+ ]
+ end
+ end
+ end
- class WebMockHttpRequest < EventMachine::HttpRequest
+ class WebMockHttpConnection < HttpConnection
+ def webmock_activate_connection(client)
+ request_signature = client.request_signature
- include HttpEncoding
+ if WebMock::StubRegistry.instance.registered_request?(request_signature)
+ conn = HttpStubConnection.new rand(10000)
+ post_init
- class WebMockHttpClient < EventMachine::HttpClient
+ @deferred = false
+ @conn = conn
- def setup(response, uri, error = nil)
- @last_effective_url = @uri = uri
- if error
- on_error(error)
- fail(self)
- else
- EM.next_tick do
- receive_data(response)
- succeed(self)
- end
- end
- end
+ conn.parent = self
+ conn.pending_connect_timeout = @connopts.connect_timeout
+ conn.comm_inactivity_timeout = @connopts.inactivity_timeout
- def unbind
+ finalize_request(client)
+ @conn.set_deferred_status :succeeded
+ elsif WebMock.net_connect_allowed?(request_signature.uri)
+ real_activate_connection(client)
+ else
+ raise WebMock::NetConnectNotAllowedError.new(request_signature)
end
+ end
+ alias_method :real_activate_connection, :activate_connection
+ alias_method :activate_connection, :webmock_activate_connection
+ end
- def close_connection
- end
+ class WebMockHttpClient < EventMachine::HttpClient
+ include HttpEncoding
+
+ def uri
+ @req.uri
end
- def send_request_with_webmock(&block)
- request_signature = build_request_signature
+ def setup(response, uri, error = nil)
+ @last_effective_url = @uri = uri
+ if error
+ on_error(error)
+ fail(self)
+ else
+ @conn.receive_data(response)
+ succeed(self)
+ end
+ end
+ def send_request_with_webmock(head, body)
WebMock::RequestRegistry.instance.requested_signatures.put(request_signature)
if WebMock::StubRegistry.instance.registered_request?(request_signature)
webmock_response = WebMock::StubRegistry.instance.response_for_request(request_signature)
- WebMock::CallbackRegistry.invoke_callbacks(
- {:lib => :em_http_request}, request_signature, webmock_response)
- client = WebMockHttpClient.new(nil)
- client.on_error("WebMock timeout error") if webmock_response.should_timeout
- client.setup(make_raw_response(webmock_response), @uri,
- webmock_response.should_timeout ? "WebMock timeout error" : nil)
- client
+ on_error("WebMock timeout error") if webmock_response.should_timeout
+ WebMock::CallbackRegistry.invoke_callbacks({:lib => :em_http_request}, request_signature, webmock_response)
+ EM.next_tick {
+ setup(make_raw_response(webmock_response), @uri,
+ webmock_response.should_timeout ? "WebMock timeout error" : nil)
+ }
+ self
elsif WebMock.net_connect_allowed?(request_signature.uri)
- http = send_request_without_webmock(&block)
- http.callback {
+ send_request_without_webmock(head, body)
+ callback {
if WebMock::CallbackRegistry.any_callbacks?
- webmock_response = build_webmock_response(http)
+ webmock_response = build_webmock_response
WebMock::CallbackRegistry.invoke_callbacks(
- {:lib => :em_http_request, :real_request => true}, request_signature,
+ {:lib => :em_http_request, :real_request => true},
+ request_signature,
webmock_response)
end
}
- http
+ self
else
raise WebMock::NetConnectNotAllowedError.new(request_signature)
end
@@ -62,55 +101,63 @@ def send_request_with_webmock(&block)
alias_method :send_request_without_webmock, :send_request
alias_method :send_request, :send_request_with_webmock
+ def request_signature
+ @request_signature ||= build_request_signature
+ end
private
- def build_webmock_response(http)
+ def build_webmock_response
webmock_response = WebMock::Response.new
- webmock_response.status = [http.response_header.status, http.response_header.http_reason]
- webmock_response.headers = http.response_header
- webmock_response.body = http.response
+ webmock_response.status = [response_header.status, response_header.http_reason]
+ webmock_response.headers = response_header
+ webmock_response.body = response
webmock_response
end
def build_request_signature
- if @req
- options = @req.options
- method = @req.method
- uri = @req.uri
- else
- options = @options
- method = @method
- uri = @uri
+ headers, body = @req.headers, @req.body
+
+ @conn.middleware.select {|m| m.respond_to?(:request) }.each do |m|
+ headers, body = m.request(self, headers, body)
end
- if options[:authorization] || options['authorization']
- auth = (options[:authorization] || options['authorization'])
+ method = @req.method
+ uri = @req.uri
+ auth = @req.proxy[:authorization]
+ query = @req.query
+
+ if auth
userinfo = auth.join(':')
userinfo = WebMock::Util::URI.encode_unsafe_chars_in_userinfo(userinfo)
- options.reject! {|k,v| k.to_s == 'authorization' } #we added it to url userinfo
+ if @req
+ @req.proxy.reject! {|k,v| t.to_s == 'authorization' }
+ else
+ options.reject! {|k,v| k.to_s == 'authorization' } #we added it to url userinfo
+ end
uri.userinfo = userinfo
end
- uri.query = encode_query(@req.uri, options[:query]).slice(/\?(.*)/, 1)
+ uri.query = encode_query(@req.uri, query).slice(/\?(.*)/, 1)
WebMock::RequestSignature.new(
method.downcase.to_sym,
uri.to_s,
- :body => (options[:body] || options['body']),
- :headers => (options[:head] || options['head'])
+ :body => body,
+ :headers => headers
)
end
-
def make_raw_response(response)
response.raise_error_if_any
status, headers, body = response.status, response.headers, response.body
+ headers ||= {}
response_string = []
response_string << "HTTP/1.1 #{status[0]} #{status[1]}"
+ headers["Content-Length"] = body.length unless headers["Content-Length"]
headers.each do |header, value|
value = value.join(", ") if value.is_a?(Array)
@@ -128,17 +175,20 @@ def make_raw_response(response)
end
def self.activate!
- EventMachine.send(:remove_const, :HttpRequest)
- EventMachine.send(:const_set, :HttpRequest, WebMockHttpRequest)
+ EventMachine.send(:remove_const, :HttpConnection)
+ EventMachine.send(:const_set, :HttpConnection, WebMockHttpConnection)
+ EventMachine.send(:remove_const, :HttpClient)
+ EventMachine.send(:const_set, :HttpClient, WebMockHttpClient)
end
def self.deactivate!
- EventMachine.send(:remove_const, :HttpRequest)
- EventMachine.send(:const_set, :HttpRequest, OriginalHttpRequest)
+ EventMachine.send(:remove_const, :HttpConnection)
+ EventMachine.send(:const_set, :HttpConnection, OriginalHttpConnection)
+ EventMachine.send(:remove_const, :HttpClient)
+ EventMachine.send(:const_set, :HttpClient, OriginalHttpClient)
end
end
end
- EventMachine::WebMockHttpRequest.activate!
-
+ EventMachine::WebMockHttpClient.activate!
end
View
83 spec/em_http_request_spec.rb
@@ -10,6 +10,52 @@
it_should_behave_like "WebMock"
+ it "should work with request middleware" do
+ stub_http_request(:get, "www.example.com").with(:body => 'bar')
+
+ middleware = Class.new do
+ def request(client, head, body)
+ [{}, 'bar']
+ end
+ end
+
+ EM.run do
+ conn = EventMachine::HttpRequest.new('http://www.example.com/')
+
+ conn.use middleware
+
+ http = conn.get(:body => 'foo')
+
+ http.callback do
+ WebMock.should have_requested(:get, "www.example.com").with(:body => 'bar')
+ EM.stop
+ end
+ end
+ end
+
+ it "should work with response middleware" do
+ stub_http_request(:get, "www.example.com").to_return(:body => 'foo')
+
+ middleware = Class.new do
+ def response(resp)
+ resp.response = 'bar'
+ end
+ end
+
+ EM.run do
+ conn = EventMachine::HttpRequest.new('http://www.example.com/')
+
+ conn.use middleware
+
+ http = conn.get
+
+ http.callback do
+ http.response.should be == 'bar'
+ EM.stop
+ end
+ end
+ end
+
it "should work with streaming" do
stub_http_request(:get, "www.example.com").to_return(:body => "abc")
response = ""
@@ -35,6 +81,43 @@
http_request(:get, "http://www.example.com/?x=3", :query => "a[]=b&a[]=c").body.should == "abc"
end
+ # not pretty, but it works
+ describe "with synchrony" do
+ let(:webmock_em_http) { File.expand_path(File.join(File.dirname(__FILE__), "../lib/webmock/http_lib_adapters/em_http_request.rb")) }
+
+ before(:each) do
+ # need to reload the webmock em-http adapter after we require synchrony
+ EM::WebMockHttpClient.deactivate!
+ $".delete webmock_em_http
+ require 'em-synchrony'
+ require 'em-synchrony/em-http'
+ require webmock_em_http
+ end
+
+ it "should work" do
+ stub_request(:post, /.*.testserver.com*/).to_return(:status => 200, :body => 'ok')
+ lambda {
+ EM.run do
+ fiber = Fiber.new do
+ http = EM::HttpRequest.new("http://www.testserver.com").post :body => "foo=bar&baz=bang", :timeout => 60
+ EM.stop
+ end
+ fiber.resume
+ end
+ }.should_not raise_error
+ end
+
+ after(:each) do
+ EM.send(:remove_const, :Synchrony)
+ EM.send(:remove_const, :HTTPMethods)
+ EM::WebMockHttpClient.deactivate!
+ $".reject! {|path| path.include? "em-http-request"}
+ $".delete webmock_em_http
+ require 'em-http-request'
+ require webmock_em_http
+ end
+ end
+
describe "mocking EM::HttpClient API" do
before { stub_http_request(:get, "www.example.com/") }
subject do
View
11 spec/em_http_request_spec_helper.rb
@@ -6,17 +6,19 @@ def failed
end
def http_request(method, uri, options = {}, &block)
+ @http = nil
+ head = options[:headers] || {}
response = nil
error = nil
uri = Addressable::URI.heuristic_parse(uri)
EventMachine.run {
- request = EventMachine::HttpRequest.new("#{uri.omit(:userinfo).normalize.to_s}")
- http = request.send(:setup_request, method, {
+ request = EventMachine::HttpRequest.new("#{uri.normalize.to_s}")
+ http = request.send(method, {
:timeout => 10,
:body => options[:body],
:query => options[:query],
- 'authorization' => [uri.user, uri.password],
- :head => options[:headers]}, &block)
+ :head => head.merge('authorization' => [uri.user, uri.password])
+ }, &block)
http.errback {
error = if http.respond_to?(:errors)
http.errors
@@ -34,6 +36,7 @@ def http_request(method, uri, options = {}, &block)
})
EventMachine.stop
}
+ @http = http
}
raise error if error
response
View
29 spec/webmock_shared.rb
@@ -532,7 +532,11 @@ class MyException < StandardError; end;
it "should return declared status message" do
stub_http_request(:get, "www.example.com").to_return(:status => [500, "Internal Server Error"])
- http_request(:get, "http://www.example.com/").message.should == "Internal Server Error"
+ response = http_request(:get, "http://www.example.com/")
+ # not supported by em-http-request, it always returns "unknown" for http_reason
+ unless @http.is_a?(EventMachine::WebMockHttpClient)
+ response.message.should == "Internal Server Error"
+ end
end
it "should return default status code" do
@@ -542,7 +546,11 @@ class MyException < StandardError; end;
it "should return default empty message" do
stub_http_request(:get, "www.example.com")
- http_request(:get, "http://www.example.com/").message.should == ""
+ response = http_request(:get, "http://www.example.com/")
+ # not supported by em-http-request, it always returns "unknown" for http_reason
+ unless @http.is_a?(EventMachine::WebMockHttpClient)
+ response.message.should == ""
+ end
end
it "should return body declared as IO" do
@@ -640,7 +648,10 @@ def call(request)
end
it "should return recorded status message" do
- @response.message.should == "OK"
+ # not supported by em-http-request, it always returns "unknown" for http_reason
+ unless @http.is_a?(EventMachine::WebMockHttpClient)
+ @response.message.should == "OK"
+ end
end
it "should ensure file is closed" do
@@ -676,7 +687,10 @@ def call(request)
end
it "should return recorded status message" do
- @response.message.should == "OK"
+ # not supported by em-http-request, it always returns "unknown" for http_reason
+ unless @http.is_a?(EventMachine::WebMockHttpClient)
+ @response.message.should == "OK"
+ end
end
end
@@ -1491,8 +1505,11 @@ def call(request)
end
it "should pass response with status and message" do
- @response.status[0].should == 302
- @response.status[1].should == "Found"
+ # not supported by em-http-request, it always returns "unknown" for http_reason
+ unless @http.is_a?(EventMachine::WebMockHttpClient)
+ @response.status[0].should == 302
+ @response.status[1].should == "Found"
+ end
end
it "should pass response with headers" do
View
2  webmock.gemspec
@@ -20,7 +20,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'rspec', '>= 2.0.0'
s.add_development_dependency 'httpclient', '>= 2.1.5.2'
s.add_development_dependency 'patron', '>= 0.4.9'
- s.add_development_dependency 'em-http-request', '>= 0.2.14'
+ s.add_development_dependency 'em-http-request', '>= 1.0.0.beta.4'
s.add_development_dependency 'curb', '>= 0.7.8'
s.files = `git ls-files`.split("\n")
Something went wrong with that request. Please try again.