Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expects.never doesn't handle stubs methods correctly #678

Open
ducmtran opened this issue Nov 6, 2024 · 6 comments · May be fixed by #679
Open

expects.never doesn't handle stubs methods correctly #678

ducmtran opened this issue Nov 6, 2024 · 6 comments · May be fixed by #679
Assignees
@floehopper
Labels
bug-report

Comments

@ducmtran
Copy link

ducmtran commented Nov 6, 2024

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
Copy link
Member

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!

@floehopper floehopper self-assigned this Nov 7, 2024
@zenspider
Copy link
Contributor

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 added a commit that referenced this issue Nov 9, 2024
@floehopper
Fail fast if invocation matches never expectation
fc5898a 
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

This was happening because neither the `if` condition was `true`,
because the "never" expectation was not returned by
`ExpectationList#match_allowing_invocation`, but the other expectation
allowing expectations was returned. Thus `Expectation#invoke` was called
on the latter and `Mock#raise_unexpected_invocation_error` was not
called.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.
@floehopper floehopper linked a pull request Nov 9, 2024 that will close this issue
Open
3 tasks
@floehopper
Copy link
Member

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 added a commit that referenced this issue Nov 9, 2024
@floehopper
Fail fast if invocation matches never expectation
ca5bf20 
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

This was happening because neither the `if` condition was `true`,
because the "never" expectation was not returned by
`ExpectationList#match_allowing_invocation`, but the other expectation
allowing expectations was returned. Thus `Expectation#invoke` was called
on the latter and `Mock#raise_unexpected_invocation_error` was not
called.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.
@floehopper
Copy link
Member

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
Copy link
Author

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
Copy link
Member

@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

floehopper added a commit that referenced this issue Nov 13, 2024
@floehopper
Fail fast if invocation matches never expectation
9ed84cd 
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

This was happening because neither the `if` condition was `true`,
because the "never" expectation was not returned by
`ExpectationList#match_allowing_invocation`, but the other expectation
allowing expectations was returned. Thus `Expectation#invoke` was called
on the latter and `Mock#raise_unexpected_invocation_error` was not
called.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.
floehopper added a commit that referenced this issue Nov 24, 2024
@floehopper
Fail fast if invocation matches never expectation
f832c55 
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

This was happening because neither the `if` condition was `true`,
because the "never" expectation was not returned by
`ExpectationList#match_allowing_invocation`, but the other expectation
allowing expectations was returned. Thus `Expectation#invoke` was called
on the latter and `Mock#raise_unexpected_invocation_error` was not
called.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.
floehopper added a commit that referenced this issue Nov 24, 2024
@floehopper
Fail fast if invocation matches never expectation
b98a6dc 
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

This was happening because neither the `if` condition was `true`,
because the "never" expectation was not returned by
`ExpectationList#match_allowing_invocation`, but the other expectation
allowing expectations was returned. Thus `Expectation#invoke` was called
on the latter and `Mock#raise_unexpected_invocation_error` was not
called.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.
floehopper added a commit that referenced this issue Nov 24, 2024
@floehopper
Fail fast if invocation matches never expectation
15985bc 
Previously when an invocation matched an expectation which did not allow
invocations (i.e. `Expectation#never` had been called on it), but the
invocation also matched another expectation which did allow invocations,
then the test would not fail with an unexpected invocation error.

After #681 a deprecation warning was displayed in this scenario, but now
an unexpected invocation error is reported.

This behaviour was confusing and had led to a number of issues being
raised over the years: #44, #131, #490 & most recently #678. Previously
I had thought this was a symptom of the JMock v1 dispatch behaviour
(which _might_ still be true) and thus would be best addressed by
adopting the JMock v2 dispatch behaviour (#173). However, having
considered this specific scenario involving a "never" expectation, I've
decided to try to fix it with the changes in this commit.

Also the new behaviour will bring the behaviour into line with what is
already the case for mocks where `Mock#stub_everything` has been called
as per this acceptance test [1] that was introduced in this commit [2].

Now a test like this will fail with an unexpected invocation error:

    mock = mock('mock')
    mock.stubs(:method)
    mock.expects(:method).never
    mock.method

    unexpected invocation: #<Mock:mock>.method()
    unsatisfied expectations:
    - expected never, invoked once: #<Mock:mock>.method(any_parameters)
    satisfied expectations:
    - allowed any number of times, invoked never: #<Mock:mock>.method(any_parameters)

Closes #678. Also addresses #490, #131 & #44.

[1]: https://github.com/freerange/mocha/blob/f2fa99197f35e2d2ce9554db63f6964057e29ce0/test/acceptance/expected_invocation_count_test.rb#L202-L214
[2]: d358377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@floehopper floehopper

Labels
bug-report
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fail fast if invocation matches never expectation freerange/mocha
3 participants
@floehopper @zenspider @ducmtran