Skip to content

allow expectations to be set on the stub itself #103

Merged
merged 4 commits into from Jul 30, 2011

4 participants

@afeld
afeld commented Jun 6, 2011

This replaces issue #83 - see the revised README for the new syntax to set RSpec expectations on the stub itself, which saves having to retype the URL, matching parameters, etc. twice for each spec.

I basically mirrored the assert_requested specs for starters, though more could certainly be added. The one issue I'm still having is that the error message being thrown shows the mock request instance rather than the request string - hence all the commented out fail_with messages in webmock_shared.rb. I'm sure it's something small I'm missing.

@bronson
bronson commented Jun 8, 2011

This is awesome. I was just about to file an issue asking how to DRY up my tests because the duplication suggested by webmock's README just smells terrible. Added your branch to the bundle file, removed the duplication, and life is sooo much better.

  gem 'webmock', :git => 'https://github.com/afeld/webmock.git', :branch => "expectations_on_stubs"
@bronson
bronson commented Jun 8, 2011

One thought: it would be nice if worked like rspec's should_receive call. For instance, instead of:

stub = stub_request(:post, "https://bla").with(:body => "values").to_return(:body => {}.to_json)
@mytest.send_request
stub.should have_been_requested

That last line is somewhat unfortunate. What about this, which should be exactly equivalent?

should_request(:post, "https://bla").with(:body => "values").to_return(:body => {}.to_json)
@mytest.send_request
@afeld
afeld commented Jun 8, 2011

@bronson Yeah, I was trying to do what you mentioned with something like

stub_request(:get, "http://www.example.com").should be_requested.once
# ... make a request ...

Unfortunately I don't think the should be_requested syntax will work, since it would basically need to defer checking that expectation until the end of the example. A new syntax (i.e. the one you proposed) or execution of requests within a block might make it possible, but in either case it would be far less trivial of a change. Also, the advantage of this syntax if that multiple checks can be done within a single test - see my latest commit for an example.

@jcf
Collaborator
jcf commented Jun 11, 2011

The failure message can be fixed using the following code in RequestStub.

def to_s
  request_pattern.to_s
end

You have pended a spec that doesn't work in webmock_shared.rb, and until that spec is working I can't merge this change.

@afeld
afeld commented Jun 13, 2011

@jcf Thanks for that failure message fix. I ended up removing the spec you are referring to because it was throwing the NetConnectNotAllowedError, which (in my understanding) is the expected behavior.

stub = stub_request(:post, "www.example.com").with { |req| req.body == "wadus" }
http_request(:post, "http://www.example.com/", :body => "abc")  # raises NetConnectNotAllowedError
# doesn't matter what the next line is, since an exception has already been raised

Since the request did not match the stub, it is appropriate that the above error is thrown, but it doesn't test the new functionality so it isn't useful as part of this pull request.

@bblimke
Owner

This spec makes sense. I'd just add stub_request(:any, /.+/) at the beginning, to stub "any other" request which would prevent NetConnectNotAllowedError error.

@bblimke
Owner
bblimke commented Jul 29, 2011

@afeld nicely done! This will indeed remove lot of duplication from specs. Thank's for the pull request.
I'm going to merge it into master.

@bronson I wanted to have stubbing code separated from expectations code and avoided expect-run-verify in webmock for purpose. In my opinion it has advantages but different people like different styles, it's a matter of taste.

@bblimke bblimke merged commit 76c01cf into bblimke:master Jul 30, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.