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.

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

Unexpected behaviour of expects.never on stub-ed method #490

Closed
BrentWheeldon opened this issue Sep 15, 2020 · 5 comments
Closed

Unexpected behaviour of expects.never on stub-ed method #490

BrentWheeldon opened this issue Sep 15, 2020 · 5 comments
Assignees

Comments

@BrentWheeldon
Copy link

I'm working on a test that is passing even when it shouldn't. I've simplified it down to a generic example:

setup do
  @mock = Mocha::Mock.new(nil)
  @mock.stubs(:a_method)
end

def test_a_method_never_called
  @mock.expects(:a_method).never

  @mock.a_method
end

That test passes, which I've found a bit unexpected. We use the setup because there are many test cases and most of them just need a simple stub, but in some we want to assert on how many times the method is called.

If I change my test to:

setup do
  @mock = Mocha::Mock.new(nil)
  @mock.stubs(:a_method)
end

def test_a_method_never_called
  @mock.unstub(:a_method)
  @mock.expects(:a_method).never

  @mock.a_method
end

It fails as expected... But that feels like a bit of a hack?

I wrote a failing test case for MockTest that demonstrates the issue:

  def test_expecting_stubbed_method_never_fails
    mock = build_mock
    mock.stubs(:a_method)
    mock.expects(:a_method).never

    mock.a_method

    assert !mock.__verified__?
  end

It's entirely possible I'm missing something here so any advice on alternative approaches would be appreciated! (Also, if this is a bug I'm happy to PR a fix if there's guidance on how this might be fixed!)

@floehopper
Copy link
Member

Hi @BrentWheeldon. Thanks for your question. I'm sorry for the slow reply - really busy with work right now and about to go on holiday.

I haven't looked at your examples in detail, but at first glance I think your scenario is somewhat similar to #171. Since that issue was reported, I've made some changes to the documentation:

From here:

Stubs and expectations are basically the same thing. A stub is just an expectation of zero or more invocations. The Expectation#stubs method is syntactic sugar to make the intent of the test more explicit.

When a method is invoked on a mock object, the mock object searches through its expectations from newest to oldest to find one that matches the invocation. After the invocation, the matching expectation might stop matching further invocations.

And from here:

However, there are some possible “gotchas” caused by this scheme:

  • if you create an expectation and then a stub for the same method, the stub will always override the expectation and the expectation will never be met.

  • if you create a stub and then an expectation for the same method, the expectation will match, and when it stops matching the stub will be used instead, possibly masking test failures.

  • if you create different expectations for the same method, they will be invoked in the opposite order than that in which they were specified, rather than the same order.

I hope this at least explains why you're seeing the behaviour you are seeing even if it doesn't suggest a solution. I'll try to find some time as soon as I can to suggest one or more alternative approaches to solve your issue. Sorry I don't have time right now. In the meantime it might be worth a quick look through closed issues to see if I've helped someone in this scenario before.

Otherwise in the long termer the aim is to resolve #173 and work on that is in progress in #448.

@BrentWheeldon
Copy link
Author

I'm sorry for the slow reply - really busy with work right now and about to go on holiday.

No worries at all! Enjoy your holiday!

I haven't looked at your examples in detail, but at first glance I think your scenario is somewhat similar to #171. Since that issue was reported, I've made some changes to the documentation:

Yep - it's totally the same issue. I don't know how I missed those docs! Sorry about that. I'll close this out 👍

@floehopper
Copy link
Member

Thanks! If you can think of a way to improve the docs to prevent other people having the same problem, I'd be really grateful. One thing I've been thinking about is having some kind of cookbook in the docs with some fuller examples than the API docs allow. Do you think that might have helped in this case?

@BrentWheeldon
Copy link
Author

I'm not sure, to be honest. I really don't know how I missed the existing docs - it was all right there. I think you can chalk this one up to user error!

floehopper added a commit that referenced this issue Nov 9, 2024

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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 9, 2024

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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 13, 2024

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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
floehopper added a commit that referenced this issue Dec 7, 2024

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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)

This commit also takes into account the fix in this commit [3]: In #681
I added logic to display a deprecation warning about a change I planned
to make in this PR. However, it turns out the logic in both was faulty
as demonstrated in this example [4] where the deprecation warning was
incorrectly displayed for the 2nd call to `Foo.create_if`:

    class Foo
      def self.create_if(condition)
        new if condition
      end
    end

    test "it creates only if condition is true" do
      Foo.expects(:new).never
      Foo.create_if(false)

      Foo.expects(:new).once
      Foo.create_if(true)
    end

This commit adds a new acceptance test to cover an example where the
logic was incorrect and updates the logic in `Mock#handle_method_call`
to fix the logic so this new test passes and none of the existing tests
fail. Unfortunately the implementation is now rather complicated and
hard to follow, but I think it will do for now. I have a couple of ideas
for ways to simplify it, but I don't have time to work on that now.

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
[3]: 01874f1
[4]: #679 (comment)
floehopper added a commit that referenced this issue Dec 7, 2024

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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)

This commit also takes into account the fix in this commit [3]: In #681
I added logic to display a deprecation warning about a change I planned
to make in this PR. However, it turns out the logic in both was faulty
as demonstrated in this example [4] where the deprecation warning was
incorrectly displayed for the 2nd call to `Foo.create_if`:

    class Foo
      def self.create_if(condition)
        new if condition
      end
    end

    test "it creates only if condition is true" do
      Foo.expects(:new).never
      Foo.create_if(false)

      Foo.expects(:new).once
      Foo.create_if(true)
    end

This commit adds a new acceptance test to cover an example where the
logic was incorrect and updates the logic in `Mock#handle_method_call`
to fix the logic so this new test passes and none of the existing tests
fail. Unfortunately the implementation is now rather complicated and
hard to follow, but I think it will do for now. I have a couple of ideas
for ways to simplify it, but I don't have time to work on that now.

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
[3]: 01874f1
[4]: #679 (comment)
floehopper added a commit that referenced this issue Dec 7, 2024

Verified

This commit was signed with the committer’s verified signature.
floehopper James Mead
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)

This commit also takes into account the fix in this commit [3]: In #681
I added logic to display a deprecation warning about a change I planned
to make in this PR. However, it turns out the logic in both was faulty
as demonstrated in this example [4] where the deprecation warning was
incorrectly displayed for the 2nd call to `Foo.create_if`:

    class Foo
      def self.create_if(condition)
        new if condition
      end
    end

    test "it creates only if condition is true" do
      Foo.expects(:new).never
      Foo.create_if(false)

      Foo.expects(:new).once
      Foo.create_if(true)
    end

This commit adds a new acceptance test to cover an example where the
logic was incorrect and updates the logic in `Mock#handle_method_call`
to fix the logic so this new test passes and none of the existing tests
fail. Unfortunately the implementation is now rather complicated and
hard to follow, but I think it will do for now. I have a couple of ideas
for ways to simplify it, but I don't have time to work on that now.

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
[3]: 01874f1
[4]: #679 (comment)
@floehopper
Copy link
Member

A fix was released in v2.7.0 - better late than never?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants