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

Fail fast if invocation matches never expectation #679

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

floehopper
Copy link
Member

@floehopper floehopper commented Nov 7, 2024

TODO

  • Come up with a version of this that emits a deprecation warning rather than actually changing the behaviour.
  • Ensure all the relevant documentation is updated, e.g. method dispatch in README, class docs for Mock, and docs for any relevant methods.
  • Update commit note(s) to reference being consistent with the behaviour described in the following test which was added in d358377

def test_should_fail_fast_if_method_is_never_expected_but_is_called_once_even_if_everything_is_stubbed
test_result = run_as_test do
stub = stub_everything('stub')
stub.expects(:method).never
1.times { stub.method }
end
assert_failed(test_result)
assert_equal [
'unexpected invocation: #<Mock:stub>.method()',
'unsatisfied expectations:',
'- expected never, invoked once: #<Mock:stub>.method(any_parameters)'
], test_result.failure_message_lines
end

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 more carefully, I've decided to try to fix it with the changes in this PR.

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 force-pushed the expectation-with-never-cardinality-should-fail-fast-if-invoked branch from 7caf33f to 5a214c0 Compare November 7, 2024 15:39
@floehopper floehopper force-pushed the expectation-with-never-cardinality-should-fail-fast-if-invoked branch from 5a214c0 to fc5898a Compare November 9, 2024 11:10
@floehopper floehopper changed the title WIP Fail fast if invocation matches never expectation Nov 9, 2024
floehopper added a commit that referenced this pull request Nov 9, 2024
Currently when an invocation matches an expectation which does not allow
invocations (i.e. `Expectation#never` has been called on it), but the
invocation also matches another expectation which does allow
invocations, then the test does not fail with an unexpected invocation
error.

In #679 I'm planning to change this behaviour so the test fails fast
with an unexpected invocation error. This commit displays a deprecation
warning instead to give users a chance to update their code before the
actual change is made.
@floehopper floehopper force-pushed the expectation-with-never-cardinality-should-fail-fast-if-invoked branch from fc5898a to ca5bf20 Compare November 9, 2024 14:32
I'm about to make some changes in this area and this will make it easier
to see what's going on.
I'm planning to use this in a subsequent commit.
I'm planning to use this in a subsequent commit.
I'm planning to use this in a subsequent commit.
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.
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
floehopper added a commit that referenced this pull request Nov 13, 2024
Currently when an invocation matches an expectation which does not allow
invocations (i.e. `Expectation#never` has been called on it), but the
invocation also matches another expectation which does allow
invocations, then the test does not fail with an unexpected invocation
error.

In #679 I'm planning to change this behaviour so the test fails fast
with an unexpected invocation error. This commit displays a deprecation
warning instead to give users a chance to update their code before the
actual change is made.
@floehopper floehopper force-pushed the expectation-with-never-cardinality-should-fail-fast-if-invoked branch from ca5bf20 to 73c4ea6 Compare November 13, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expects.never doesn't handle stubs methods correctly
1 participant