expects.never doesn't handle stubs methods correctly

[ducmtran]

We found that expects.never doesn't fail a test when the method is stubbed with stubs. Reproduce example below, with the test_foo_never_called case. I don't think this is intended?

require 'rubygems'
gem 'mocha'
require 'minitest/autorun'
require 'mocha/minitest'

class TestMeme < Minitest::Test
  class Klass
    def self.foo; end
  end

  def test_foo_never_called # should fail, but doesn't
    Klass.stubs(:foo)
    Klass.expects(:foo).never
    Klass.foo
  end

  def test_foo_called_once # correctly passes
    Klass.stubs(:foo)
    Klass.expects(:foo).once
    Klass.foo
  end

  def test_no_stubs # correctly fails
    Klass.expects(:foo).never
    Klass.foo
  end
end

output

➜ sandbox ruby mocha_stub.rb
Run options: --seed 21862

# Running:

F

Failure:
TestMeme#test_no_stubs [mocha_stub.rb:25]:
unexpected invocation: TestMeme::Klass.foo()
unsatisfied expectations:
- expected never, invoked once: TestMeme::Klass.foo(any_parameters)



rails test mocha_stub.rb:23

..

Finished in 0.000907s, 3307.6069 runs/s, 2205.0713 assertions/s.
3 runs, 2 assertions, 1 failures, 0 errors, 0 skips

cc @zenspider

[floehopper]

This is working as expected and is effectively a duplicate of #490, #131 & #44. However, I recognize that the behaviour is confusing and, having revisited my responses to those issues, I'm now more sympathetic to trying to come up with a fix.

Previously I'd hoped that fixing #173 would solve this problem, but I now recognize that I'm unlikely to implement that any time soon.

So to that end I've spiked on a fix in #679 which would generate this extra test failure for your example:

  1) Failure:
TestMeme#test_foo_never_called [foo.rb:11]:
not all expectations were satisfied
unsatisfied expectations:
- expected never, invoked once: TestMeme::Klass.foo(any_parameters)
satisfied expectations:
- allowed any number of times, invoked never: TestMeme::Klass.foo(any_parameters)

Is that the kind of failure you are expecting?

Now I just need to find the time to:

1. Work through the test failures in the spike to see if this fix causes collateral damage elsewhere
2. Work out how to roll this out, i.e. deprecation warnings, etc

Unfortunately I don't have loads of time to spend on this at the moment, so it might take me a while, but I'll do my best!

[zenspider]

personally I think that failure above is fine. I do find the double entry for foo to be confusing but I know enough to know why it is there.

Some of your failure messages are changing in ways that will be less explanatory, eg:

  1) Failure:
UnexpectedInvocationTest#test_avoid_recursion_when_unexpected_invocation_exception_message_depends_on_uninspectable_object [test/acceptance/unexpected_invocation_test.rb:21]:
--- expected
+++ actual
@@ -1 +1 @@
-"unexpected invocation: inspect(1, 2, foo)"
+"not all expectations were satisfied"


  2) Failure:
FailureMessageTest#test_should_include_unexpected_invocation_in_unsatisfied_expectation_message [test/acceptance/multiple_expectations_failure_message_test.rb:21]:
--- expected
+++ actual
@@ -1 +1 @@
-["unexpected invocation: #<Mock:mock>.method_one()", "unsatisfied expectations:", "- expected exactly once, invoked twice: #<Mock:mock>.method_one(any_parameters)"]
+["not all expectations were satisfied", "unsatisfied expectations:", "- expected exactly once, invoked twice: #<Mock:mock>.method_one(any_parameters)"]

[floehopper]

personally I think that failure above is fine. I do find the double entry for foo to be confusing but I know enough to know why it is there.

Thanks for your feedback. I wasn't really asking about the specifics of the error message; I was more interested in double-checking this was the failure that @ducmtran was expecting when they added the "should fail, but doesn't" comment to the test_foo_never_called method in their original example.

Making any improvements to the specific of the error message is out-of-scope and would be tackled separately.

Some of your failure messages are changing in ways that will be less explanatory, eg:

These other tests were failing due to shortcomings in my original spike. I have now come up with a solution that doesn't break any other tests in #679.

@ducmtran @zenspider Would either of you be able to run this branch of Mocha against your test suite to give me some more confidence I haven't broken anything else? Thanks.

[floehopper]

I've created a PR which implements a deprecation warning in #681.

@ducmtran @zenspider It would be great if you could try that branch out too if you have time.

[ducmtran]

Thanks for the quick responses and the background on this. I see why it is the way it is now, but i'm definitely stumped when running into the test case above

We will try the deprecation branch and see how it goes

[floehopper]

@ducmtran

We will try the deprecation branch and see how it goes

Thanks - let me know how it goes so I can get it released