Skip to content

Commit

Permalink
Avoid invoking matcher block multiple times
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
floehopper committed Nov 13, 2024
1 parent 9ed84cd commit 73c4ea6
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 7 deletions.
2 changes: 0 additions & 2 deletions lib/mocha/expectation_list.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,6 @@ def +(other)
self.class.new(to_a + other.to_a)
end

private

def matching_expectations(invocation, ignoring_order: false)
@expectations.select { |e| e.match?(invocation, ignoring_order: ignoring_order) }
end
Expand Down
13 changes: 8 additions & 5 deletions lib/mocha/mock.rb
Original file line number Diff line number Diff line change
Expand Up @@ -322,14 +322,17 @@ def handle_method_call(symbol, arguments, block)
check_responder_responds_to(symbol)
invocation = Invocation.new(self, symbol, arguments, block)

matching_expectation_allowing_invocation = all_expectations.match_allowing_invocation(invocation)
matching_expectation_never_allowing_invocation = all_expectations.match_never_allowing_invocation(invocation)
matching_expectation_ignoring_order = all_expectations.match(invocation, ignoring_order: true)
matching_expectations = all_expectations.matching_expectations(invocation)
matching_expectation_allowing_invocation = matching_expectations.detect(&:invocations_allowed?)
matching_expectation_never_allowing_invocation = matching_expectations.detect(&:invocations_never_allowed?)

if matching_expectation_allowing_invocation && !matching_expectation_never_allowing_invocation
matching_expectation_allowing_invocation.invoke(invocation)
elsif matching_expectation_ignoring_order || (!matching_expectation_ignoring_order && !@everything_stubbed)
raise_unexpected_invocation_error(invocation, matching_expectation_ignoring_order)
else
matching_expectation_ignoring_order = all_expectations.match(invocation, ignoring_order: true)
if matching_expectation_ignoring_order || (!matching_expectation_ignoring_order && !@everything_stubbed)
raise_unexpected_invocation_error(invocation, matching_expectation_ignoring_order)
end
end
end

Expand Down
11 changes: 11 additions & 0 deletions test/acceptance/parameter_matcher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -373,4 +373,15 @@ def test_should_not_match_parameters_when_values_do_not_add_up_to_ten
end
assert_failed(test_result)
end

def test_should_only_call_matcher_block_once
test_result = run_as_test do
number_of_invocations = 0
mock = mock()
mock.stubs(:method).with { number_of_invocations += 1; true }
mock.method
assert_equal 1, number_of_invocations
end
assert_passed(test_result)
end
end

0 comments on commit 73c4ea6

Please sign in to comment.