- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 159
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
Expectations are not respected when a method is stubbed out #44
Comments
This is the intended behaviour. The important thing to realise is that In this case What is the behaviour you actually want to test? Perhaps there is another way of doing it? I hope you don't mind, but I reformatted the code in your initial post to make it a bit clearer what's going on. This should make it more useful to other people as a reference. |
Thanks for the quick response (and thanks for fixing up the code, definitely much easier to read now!) I think I understand your reasoning for why the current behavior exists, but not sure I know the motivation behind the current behavior. The use case here I have is that I have some module A around which I have tests, and A calls into methods on B. For most tests on A, I simply want to stub out behavior on B so I can unit test the behavior on A given known returns from B (the typical use case for stubbing). As a result, I simply stub out a lot of the behavior on B in a setup method. But then for certain tests for A, I need to test the behavior in A by checking to see if it actually calls into B. So, I end up in situations where B.foo is stubbed out but then I have a specific test in the module A tests that wants to make sure that B.foo never gets called. I think my conceptual model (which may be completely wrong) is:
In this model, stubs and expectations don't affect each other and serve separate purposes. What is the motivation to treat stubs and expectations as part of the same system as opposed to having stubs simply be a way to modify the behavior of a called method and expectations be a way to simply determine whether or not a method is called? Again, thanks so much for the response and being patient with me =). I should add that I'd be willing to look into modifying the current behavior if we actually agree to change it. |
I think a clearer/more concise way to put this -- I think my issue here may be simply with the fact that in certain cases, all expectations seem to need to pass for a test to pass, but in others, it seems like certain expectations can fail but the overall test will still pass. I believe in the first part of that last sentence because, for example, the following code will throw an exception: Foo.expects(:a).twice
Foo.expects(:a).once
Foo.a('bye') Here, though the second expectation is satisfied, the first is not. A failure here makes sense to me. Similarly, if I do: Foo.stubs(:a)
Foo.expects(:a).once This will fail if I never call Foo.a because the second expectation is never satisfied. This model of "every expectation that is set up should be satisfied" seems like a reasonable statement to make. As a result, I think the following code should fail: Foo.stubs(:a)
Foo.expects(:a).never
Foo.a Here, even though Foo.a satisfies the first expectation, it does not satisfy the second. Similarly, the following code will pass: Foo.stubs(:a)
Foo.expects(:a).once
Foo.a
Foo.a Here, the second expectation should fail because Foo.a was called twice, not once. The first expectation is satisfied though. The issue here seems to be specific to what happens when extraneous calls to a method is made in the case of multiple expectations being set up. Basically, in the case that I just have Foo.expects(:a).once; Foo.a; Foo.a, the expectation fails, so similarly, I believe that expectation should always fail if multiple calls to Foo are made even if other expectations are set up on Foo.a. Thanks again for taking the time to look and respond to these. If the above logic makes sense to you, I'd be happy to write a pull request to fix this issue (otherwise, I can just workaround this in my code). This results in a failure (correctly). |
In the meantime, I've added a workaround for this in my test suite with the following monkeypatch: https://gist.github.com/1963537 The idea here is that on every invocation, we simply find all matching methods and and invoke them. Then, we just check at the end of a test whether or not the number of invocations violates our expectations. We don't treat never expectations differently -- they just get checked at the end of the method the same way everything else does (so no more immediate UnexpectedInvocation). This allows the examples above to do the right thing. It will also end up breaking some tests that would previously pass. For example, before, the following would create a passing test: Foo.expects(:a).once
Foo.expects(:a).twice
Foo.a
Foo.a
Foo.a The expectations above, however, seem to me to be stating that there are two conflicting expectations of the number of times 'a' should be invoked (and neither is saying a should be invoked three times). So breaking in this case now seems like it may be the correct thing to do. The only thing that is a bit unclear with this patch is if I set up multiple expectations that are all matching but that each specify different return values. So, let us take the following example: Foo.expects(:a).twice.returns(1)
Foo.expects(:a).twice.returns(2)
Foo.a
Foo.a Previously, this would return 2 followed by 1, whereas now it returns 2 twice. I also thought this may be an ok tradeoff in that, I think, the previous behavior was not really defined. In most cases when people set up multiple expectations like this that will all match, there isn't (or shouldn't be) a lot of logic about the order of returns for multiple invocations. To set up ordering of return values for multiple invocations, the proper thing to do should be: Foo.expects(:a).twice.returns(1).then.returns(2) Anyway, I am happy keeping this workaround not upstreamed, but if this seems like a reasonable patch to add to mocha, I'd be happy to make a pull request with tests around the new behavior. Let me know what you think, and thanks for bearing with my giant essays of comments =). |
Thanks for all your comments. This aspect of Mocha was modelled on the method dispatch behaviour of jMock v1. For some time I have wanted to change it to be modelled on the method dispatch behaviour of jMock v2, but this would be such a significant change in behaviour that it would require a change in major version and I just haven't managed to find the time to do it. I do see that there is an apparent inconsistency in the existing behaviour and would be interested to see whether there is a similar inconsistency if we were to change to use the jMock v2 approach. I'm not sure whether your approach would be more or less intuitive to most people. I need to do some serious thinking about this when I can find the time! It'd be great if you could read the two jMock links above and let me know what you think. In response to the actual problem you are trying to solve, this problem has come up quite a few times over the years. One option is to use Mocha's state machine mechanism. I've dug out this bit of code (which may be a bit out-of-date) as an example. I know it's a bit unwieldy, but it might give us more inspiration. |
I don't plan on incorporating this any time soon, so I'm going to mark this issue as closed. Thanks for your input. Cheers, James. |
Previously, agents always cached their catalog in $client_datadir/catalog/<host>.json, even when running in noop mode. If the agent failed to download a catalog during the next run, e.g. puppet server was stopped or overloaded, etc, the agent would fall back to the cache, due to the usecacheonfailure setting, and apply the previously cached catalog. This could lead to surprising results if you were just testing out the noop run and never meant for those changes to be deployed in production. This commit also changes the apply application, so it behaves consistently. The agent_spec explicitly unstubs `cached_class=`, because mocha behaves oddly when setting a `never` expectation on a previously stubbed method[1]. This was noticed, because the test wasn't failing as expected when run against the old code. [1] freerange/mocha#44 (comment)
Previously, agents always cached their catalogs in $client_datadir/catalog/<host>.json, even when running in noop mode. If the agent failed to download a catalog during the next run, e.g. puppet server was stopped or overloaded, etc, the agent would fall back to the cache, due to the usecacheonfailure setting, and apply the previously cached catalog. This could lead to surprising results if you were just testing out the noop run and never meant for those changes to be deployed in production. This commit will set the catalog's cache to nil in noop mode so that the catalog is never cached. It also changes the apply application to behave the same. The agent_spec explicitly unstubs `cached_class=`, because mocha behaves unintuitively when setting a `never` expectation on a previously stubbed method[1]. This was noticed, because the test wasn't failing as expected when run against the old code. [1] freerange/mocha#44 (comment)
Previously, agents always cached their catalogs in $client_datadir/catalog/<host>.json, even when running in noop mode. If the agent failed to download a catalog during the next run, e.g. puppet server was stopped or overloaded, etc, the agent would fall back to the cache, due to the usecacheonfailure setting, and apply the previously cached catalog. This could lead to surprising results if you were just testing out the noop run and never meant for those changes to be deployed in production. This commit will set the catalog's cache to nil in noop mode so that the catalog is never cached. It also changes the apply application to behave the same. The agent_spec explicitly unstubs `cached_class=`, because mocha behaves unintuitively when setting a `never` expectation on a previously stubbed method[1]. This was noticed, because the test wasn't failing as expected when run against the old code. [1] freerange/mocha#44 (comment)
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.
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.
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.
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.
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.
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
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)
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)
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)
A fix for this was released in v2.7.0 - better late than never?! |
If you stub a method on a class and then set an expectation on that method, the expectation is not actually respected.
Reproduction:
The text was updated successfully, but these errors were encountered: