Add RequestStub helper methods for accessing request signatures

[twolfson]

In #544 @brettlangdon submitted a proposal to add requests to RequestStub instances. It was declined for legitimate concerns of overlapping requests. We have naively been using the forked version in our own repo, finally ran into the issue, and found a way to solve the concerns.

We have added a new test to guarantee separate request signatures are not combined via matches? (i.e. "should not combine identical requests").

This solution is more/less the same as our #544 (use the same approach as times(n) matcher) but instead of losing request ordering and cardinality by storing request counts on a hash. We moved to storing request signatures on an array.

I'm pretty confident we have no overlap issues now since any issues would mean that times(n) has those same issues.

In this PR:

• Revived #544
  • Addition of requests, first_request, and last_request to request stubs
• Added test for request overlap
• Patched request overlap by adding array for HashCounter

Notes:

The initial revision of this PR is more of a proposal than a final product. There need to be some things that will be changed if accepted (e.g. rename HashCounter). But the concepts are here.

Reference code used from times(n):

https://github.com/bblimke/webmock/blob/v1.22.6/lib/webmock/rspec/matchers/request_pattern_matcher.rb#L18-L21

https://github.com/bblimke/webmock/blob/v1.22.6/lib/webmock/request_execution_verifier.rb#L15-L18

https://github.com/bblimke/webmock/blob/v1.22.6/lib/webmock/request_registry.rb#L16-L20

https://github.com/bblimke/webmock/blob/v1.22.6/lib/webmock/util/hash_counter.rb#L6-L18

https://github.com/bblimke/webmock/blob/v1.22.6/lib/webmock/http_lib_adapters/net_http.rb#L75-L77

[bblimke]

@twolfson thank you for submitting this pull request and for an attempt to improve the previous solution.

Unfortunately your solutions still has the same problems as the previous one.

First of all HashCounter is a HashCounter, single responsibility. You can increment counter for key, nothing more. There is no place for array there :) If you would like to keep an array of request signatures, it would have to be a different object or we would have to change the name.

There are two problems with your solution, that I mentioned already in the previous pull requests are:

1. RequestStub is a simple data object. RequestStub should not have access to RequestRegistry or be coupled with anything else. RequestStub by design is not allowed to know it's own data, nothing else.
2. requests method still returns requests that have not been handled by exatly the same stub. Perhaps I wasn't clear on this in the first pull requests, so let me show an example:

WebMock.allow_net_connect!
RestClient.get 'http://example.com/'
stub1 = stub_request(:get, "www.example.com).to_return(body: "abc")
RestClient.get 'http://example.com/'
stub2 = stub_request(:get, "www.example.com).to_return(body: "def")
RestClient.get 'http://example.com/'

stub2.requests.size #will return 3

now if you ask stub2 for request signatures it will return 3 request signatures from all 3 requests, because it matches them, despite it only handled the last request.

or another example:

stub1 = stub_request(:post, "www.example.com).to_return(body: "def")
stub2 = stub_request(:post, "www.example.com", body: {:param1 => 'one'}.to_json).to_return(body: "abc")
RestClient.post 'http://example.com/', {:param1 => 'one'}.to_json
RestClient.post 'http://example.com/' {:param1 => 'two'}.to_json

stub1.requests.size #will return 2, despite it only handled the last request

[twolfson]

I agree that HashCounter is currently acting as a stand-in on this PR -- we can rename it once we decide if this is good to land or not.

With respect to your counter examples, these are currently issues as well with the times matcher as well (also referenced in #445). I copied/edited the examples provided and got them running:

https://github.com/underdogio/webmock/blob/c8b48f9e8f98d6b68980b20d956873da6820c078/spec/unit/request_stub_spec.rb#L46-L71

When we add a times(n) assertion to these tests at the same locations as requests.size, we run into failing expectations:

https://github.com/underdogio/webmock/blob/829b274e46f366881089460e4ba00f5c5c2e0ef0/spec/unit/request_stub_spec.rb#L46-L75

Failures:

  1) WebMock::RequestStub requests test1
     Failure/Error: expect(stub2).to(have_been_requested.once())

       The request GET http://www.google.com/ was expected to execute 1 time but it executed 3 times

       The following requests were made:

       GET http://www.google.com/ with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'User-Agent'=>'Ruby'} was made 3 times

       ============================================================
     # ./spec/unit/request_stub_spec.rb:56:in `block (3 levels) in <top (required)>'

  2) WebMock::RequestStub requests test2
     Failure/Error: expect(stub1).to(have_been_requested.once())

       The request POST http://www.google.com/ was expected to execute 1 time but it executed 2 times

       The following requests were made:

       POST http://www.google.com/ with body 'param1=one' with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'application/x-www-form-urlencoded', 'Host'=>'www.google.com', 'User-Agent'=>'Ruby'} was made 1 time
       POST http://www.google.com/ with body 'param1=two' with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'application/x-www-form-urlencoded', 'Host'=>'www.google.com', 'User-Agent'=>'Ruby'} was made 1 time

       ============================================================
     # ./spec/unit/request_stub_spec.rb:71:in `block (3 levels) in <top (required)>'

I believe that in expected usage scenarios these edge cases don't come up. People will likely only set up stubs once per it block before requests are made as well as only provide 1 stub over a specific URL (e.g. http://example.com/). If they did, then we would see #445 and its variations more frequently =/

With respect to RequestStub, we are open to suggestions at other locations to put these accessors (e.g. RequestRegistry[stub]?).

[bblimke]

Yes, people usually set only one stub per example - usually. The code needs to be deterministic though.

You are right that times matcher has exactly the same issue if you use expect(stub1).to(have_been_requested.once()) syntax.

The problem is not with how it works, but how it reads though.

Initially you had to write:

stub1 = stub_request(:post, "www.example.com)
RestClient.post 'http://example.com/'
expect(a_request(:post, "www.example.com").to(have_been_requested.once())

to avoid duplication in declaring request signatures, support for matching directly on request stub was added:

stub1 = stub_request(:post, "www.example.com)
RestClient.post 'http://example.com/'
expect(stub1).to(have_been_requested.once())

Therefore expect(stub1).to(have_been_requested.once()) doesn't mean that a request stubbed with stub1 was executed once. It means that a request which request signature matches stub1 was requested once, but not necessarily handled by stub1

I agree it's confusing and it's worth considering changing the WebMock API to make it clear. I.e expect(request_matching(stub1)).to(have_been_requested.once())

stub1.requests is even more confusing though.

Perhaps we could add a method to WebMock's API. i.e. something like executed_requests(requests_matching(stub1))

in above examples requests_matching(stub) would be a method that takes RequestStub as an argument and returns RequestPattern.

[twolfson]

Ah, I see. Thanks for clarifying -- I think introducing request_matching(stub1) would resolve #445's main complaint as well.

Another solution seems to be using the request_stub.request_pattern method/attribute:

https://github.com/bblimke/webmock/blob/v1.22.6/lib/webmock/request_stub.rb#L7

expect(stub1.request_pattern).to(have_been_requested.once())
puts executed_requests(stub1.request_pattern)

Or maybe we rename executed_requests to matching_requests so we keep the same match syntax (and since we know that "execution" is a misnomer now =/)