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

Fail fast if invocation matches never expectation #679

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

Fail fast if invocation matches never expectation #679

wants to merge 6 commits into from

Commits on Nov 13, 2024

  1. Extract local vars outside if/else in Mock#handle_method_call 

    I'm about to make some changes in this area and this will make it easier
to see what's going on.
    @floehopper
    floehopper committed Nov 13, 2024
    Configuration menu
    Copy the full SHA
    83987c7 View commit details
    Browse the repository at this point in the history

  2. Add Cardinality#invocations_never_allowed? 

    I'm planning to use this in a subsequent commit.
    @floehopper
    floehopper committed Nov 13, 2024
    Configuration menu
    Copy the full SHA
    c4e6efb View commit details
    Browse the repository at this point in the history

  3. Add Expectation#invocations_never_allowed? 

    I'm planning to use this in a subsequent commit.
    @floehopper
    floehopper committed Nov 13, 2024
    Configuration menu
    Copy the full SHA
    f84a4a7 View commit details
    Browse the repository at this point in the history

  4. Add ExpectationList#match_never_allowing_invocation 

    I'm planning to use this in a subsequent commit.
    @floehopper
    floehopper committed Nov 13, 2024
    Configuration menu
    Copy the full SHA
    c502471 View commit details
    Browse the repository at this point in the history

  5. Fail fast if invocation matches never expectation 

    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 committed Nov 13, 2024
    Configuration menu
    Copy the full SHA
    9ed84cd View commit details
    Browse the repository at this point in the history

  6. Avoid invoking matcher block multiple times 

    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
    floehopper committed Nov 13, 2024
    Configuration menu
    Copy the full SHA
    73c4ea6 View commit details
    Browse the repository at this point in the history