An unmade request can cause tests with stubs to spuriously pass

[jkakar]

I'm using the stub functionality built in to Excon and I really like
it. Having this functionality be a first-class citizen is super
awesome, especially in this kind of library. One small issue I have
is that, in some cases, tests can spuriously pass if an expected
request is not made.

For example, consider this application code that might represent a
client library that makes fire-and-forget calls to an API:

def make_request()
  connection = Excon.new('http://example.com')
  connection.post(path: '/path')
end

And the accompanying test logic:

def setup
  super
  Excon.stubs.clear
  Excon.defaults[:mock] = true
end

def test_make_request
  Excon.stub(method: :post) do |request|
    assert_equal('/path', request[:path])
    {status: 200}
  end
  make_request
end

I expect this test to fail if the code in make_request is commented
out completely, but it doesn't. The Excon.stub block simply never
gets called. My workaround, for now, is to add an Excon.stubs.pop
call to the test:

def test_make_request
  Excon.stub(method: :post}) do |request|
    assert_equal('/path', request[:path])
    Excon.stubs.pop
    {status: 200}
  end
  make_request
end

And then, in teardown, ensure that all stubs have been consumed:

def teardown
  assert Excon.stubs.empty?
  super
end

This is okay, but it's also error-prone because I may forget a call to
Excon.stubs.pop and I also have to be careful to pop from the front
of the Excon.stubs array and not the end because of the way it works
internally. It'd be nice if there was a method to confirm that all
expected requests were made, something like:

def teardown
  Excon.verify
  super
end

[geemus]

@jkakar - interesting. I'll have to give this further thought in the general case, for now a couple work arounds come to mind. You could set variables inside the excon request (outside of the stubs) and then check these. I do this, for instance, in heroku.rb as a way of tracking that you have made an app (for instance) so that subsequent dependent calls can look that app up. In this case you could simply set a class/global in the before, change it in the stub and check/reset it in the after. Similarly you could have a counter that incremented (instead of a boolean or something) so you could specify that it had been called a certain number of times. Anyway, this still isn't easy to access/use but at least it would allow it. I'll give the more general case/usage additional thought...

[geemus]

I wonder if pop/deleting the stub should be the normal behavior. Since they exist as an array, you could always just add the same stub multiple times if needed. Then you could more explicitly ask if a particular stub still existed instead of just checking for empty also. What do you think?

[jkakar]

The main thing is that when I stub a call I expect it to be invoked.
I don't care so much about the implementation as much as I want a very
clear contract that Excon will tell me when something I stubbed wasn't
invoked. I also definitely don't want to manipulating the internal
stub infrastructure in Excon in my tests as I have to now.

So yeah, popping stubbed calls when they're invoked and expecting the
user to call a method like Excon.verify in teardown seems like a
good path forward.

[geemus]

I did add #unstub I believe. So you could pass the params given inside a stub to unstub to have something remove itself, which is at least a step forward (albeit not a default). How should verify work? I'm not sure checking for an empty stub list is really correct. Maybe you could just check Excon.stub_for(...) and see if it is nil/false?

[jkakar]

I think verify should check for an empty stub list and raise an
exception if that's not the case. The question to answer is this: is
it a failure if I stub a request and it never gets called? I think
the answer is yes, otherwise I wouldn't have stubbed it; therefore,
any uncalled stubs left over are a sign of failure and there should be
a simple way (that doesn't involve me as a test writer understanding
how Excon internally handles stubs) to make sure that the condition of
success (all stubs successfully called) has been met.

[geemus]

Fair. I guess my concern was not well explained. Since stubs are global, if you are using something else which ships with excon mocks as part of your testing, you may not want to clear things out (lest you break the provided mocks). This may go back to an earlier ideas, providing namespaces for stubs (by default they would work the same, but people that ship mocks could namespace it to their gem to avoid this issue).

[jkakar]

Ah, right, I could see that. But even then, if you're using something
else that's mocking calls the ordering matters, so you still need to
be aware of what's going on behind the covers.

[stale]

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

[geemus]

Closing as duplicate of #434.