Expectation with never cardinality should display deprecation warning #681
Conversation
|
we found several failing tests, here's one on mocha main branch, it only produces one print statement i couldn't find the time to debug all the failing cases so there might be other issues - we are pretty busy with other work right now so responses might be delayed, but thanks a lot for diving into this! cc @zenspider |
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.
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
force-pushed
the
expectation-with-never-cardinality-should-display-deprecation-warning
branch
from
094ea8f
to
065fe3d
Compare
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)
|
Thanks for running your test suite against the branch with the deprecation warnings. I assume that the "failure" in the the example test is because in reality you have tests relying on def test_stub_with
Klass.stubs(:foo).with { |_| p 'inside stub' }
Klass.foo
endIf so, while this was never documented behaviour to be relied upon, I've changed the code to avoid multiple invocations of the matcher block. 😓 Could you try running your test suite against the updated branch when you have time? 🙏 |
hi, i tried the latest commit again - the above is the original issue we reported and the latest commit is failing it instead of giving a deprecation warning 😅 cc @zenspider |
|
Hmm. That's very odd. I have an acceptance test which is pretty much exactly that test and it is passing in the branch. To double-check, I just ran the test below and got the expected output, i.e. the test passed but a deprecation warning was reported. Can you double-check that you were using the correct branch? i.e. # klass_test.rb
require "bundler/inline"
gemfile do
source "https://rubygems.org"
gem "minitest"
gem "mocha", git: "https://github.com/freerange/mocha.git", branch: "expectation-with-never-cardinality-should-display-deprecation-warning"
end
require "minitest/autorun"
require "mocha/minitest"
class TestKlass < Minitest::Test
class Klass
def self.foo; end
end
def test_foo_never_called
Klass.stubs(:foo)
Klass.expects(:foo).never
Klass.foo
end
end |
|
@ducmtran Have you had a chance to look at this? Otherwise I'll probably just release it! |
|
@floehopper apologies for the late response. Yes Github UI for "referenced this commit" confused me and i accidentally used the commit from #679 🤦 . With the correct commit, everything passed for us, with warning printed locally (we have an issue with our CI setup not printing the warning but i doubt it's relevant to this PR). Thanks! |
Great - thanks for letting me know. 👍 |
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. 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]. [1]: https://github.com/freerange/mocha/blob/f2fa99197f35e2d2ce9554db63f6964057e29ce0/test/acceptance/expected_invocation_count_test.rb#L202-L214 [2]: d358377
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
force-pushed
the
expectation-with-never-cardinality-should-display-deprecation-warning
branch
from
065fe3d
to
e0b6dae
Compare
|
I've tweaked the text of the deprecation warning slightly, tweaked the name of the acceptance test, and expanded the main commit note to explain that the new behaviour will bring the behaviour into line with what is already the case for mocks where mocha/test/acceptance/expected_invocation_count_test.rb Lines 202 to 214 in f2fa991 |
floehopper
deleted the
expectation-with-never-cardinality-should-display-deprecation-warning
branch
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
Currently when an invocation matches an expectation which does not allow invocations (i.e.
Expectation#neverhas 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 PR means we display a deprecation warning instead to give users a chance to update their code before the actual change is made.