Remember invocations #474
Remember invocations #474
Conversation
There was a problem hiding this comment.
Thanks so much for finding the time to investigate #473 and for submitting this PR - I really appreciate it!
I've added some review comments inline which I think it would be good to address before merging. Although I haven't had loads of time to look through the changes as carefully as I would like, so I apologise if I've misunderstood something.
I think it's probably outside the scope of this PR, but I wonder whether e.g. calling Expectation#at_most should affect the required number of calls. I think I'm right in saying that in the following code, the call to Expectation#at_most overrides the required number of calls set by the call to Expectation#at_least which doesn't seem quite right:
foo.expects(:bar).at_least(2).at_most(4)I suspect this may have always been the behaviour and I don't remember anyone complaining about it, but perhaps it would be good to tighten things up a bit. What do you think?
... in order to make a single Cardinality instance persist across updates to expectation cardinality. This include a 'times' method. To make space for it, the existing protected 'times' method has been renamed to 'count'.
Fixes freerange#473. Earlier, whenever you qualified an Expectation with a cardinality, a new Cardinality instance would replace the old one. The moving of invocations recording to Cardinality in 59454a8 then meant that the invocations recorded prior to updating cardinality would get discarded along with the Cardinality instance, effectively being forgotten. We now update the quantifiers of the same Cardinality instance for each Expectation in order to persist the recording of invocations across updates to cardinality.
The at_{least,most}_once methods simply delegate to at_least, and
at_least returns self. So, ending with a call to at_least means we don't
need to end with self in order to return self.
nitishr
force-pushed
the
remember-invocations
branch
from
a4a3d3c
to
5c876f4
Compare
There was a problem hiding this comment.
[...] I wonder whether e.g. calling
Expectation#at_mostshould affect the required number of calls. I think I'm right in saying that in the following code, the call toExpectation#at_mostoverrides the required number of calls set by the call toExpectation#at_leastwhich doesn't seem quite right:foo.expects(:bar).at_least(2).at_most(4)
You're right in your observation. And I think intuitively, the above code would seem to be equivalent to
foo.expects(:bar).times(2..4)However, our current rule is: Every cardinality-setting method defines both lower and upper bounds, thus replaces/overrides any earlier cardinality wholesale. This might even be worth documenting.
Making the above code respect intuition would mean complicating that rule, and I think it get very complicated very quickly. For instance, what's the outcome of the below?
foo.expect(:bar).exactly(2).at_most(4)Most such 'combinations' yield no sensible/intuitive outcome. So, I think rather than supporting 'combinations', we should keep it simple and stick with 'overrides'.
|
@nitishr Thanks again for identifying the problem in #473 and for taking the time to submit this PR and address my comments.
Agreed. I've opened #476 to address this.
Agreed. Thanks for considering my suggestion and for pointing out the flaws in it! I'll try to get this merged shortly. |
Creating an organisation causes a call to Whitehall::PublishingApi.patch_links due to Active Record callbacks. In Mocha 1.12.0 there was a change (presumably freerange/mocha#474) that causes the test to start failing in it's previous incantation. My understanding is that this fail occurs because the organisation is being created after the mock (`expects`) is applied to Whitehall::PublishingApi, which leads to the test failing because it thinks the `patch_links` method has been invoked twice. As per: ``` Error: patch_links::#organisations#test_patches_links_for_organisations: DRb::DRbRemoteError: unexpected invocation: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e620>, {:bulk_publishing => true}) unsatisfied expectations: - expected exactly once, invoked twice: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e530>, {:bulk_publishing => true}) satisfied expectations: - allowed any number of times, invoked never: #<User:0x3e490>.persisted?(any_parameters) - allowed any number of times, invoked never: #<User:0x3e490>.id(any_parameters) - allowed any number of times, invoked never: Services.asset_manager(any_parameters) (Minitest::Assertion) lib/tasks/publishing_api.rake:152:in `block (4 levels) in <top (required)>' lib/tasks/publishing_api.rake:149:in `each' lib/tasks/publishing_api.rake:149:in `each_with_index' lib/tasks/publishing_api.rake:149:in `block (3 levels) in <top (required)>' test/unit/tasks/publishing_api_test.rb:165:in `block (3 levels) in <class:PublishingApiRake>' ```
Creating an organisation causes a call to Whitehall::PublishingApi.patch_links due to Active Record callbacks. In Mocha 1.12.0 there was a change (presumably freerange/mocha#474) that causes the test to start failing in it's previous incantation. My understanding is that this fail occurs because the organisation is being created after the mock (`expects`) is applied to Whitehall::PublishingApi, which leads to the test failing because it thinks the `patch_links` method has been invoked twice. As per: ``` Error: patch_links::#organisations#test_patches_links_for_organisations: DRb::DRbRemoteError: unexpected invocation: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e620>, {:bulk_publishing => true}) unsatisfied expectations: - expected exactly once, invoked twice: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e530>, {:bulk_publishing => true}) satisfied expectations: - allowed any number of times, invoked never: #<User:0x3e490>.persisted?(any_parameters) - allowed any number of times, invoked never: #<User:0x3e490>.id(any_parameters) - allowed any number of times, invoked never: Services.asset_manager(any_parameters) (Minitest::Assertion) lib/tasks/publishing_api.rake:152:in `block (4 levels) in <top (required)>' lib/tasks/publishing_api.rake:149:in `each' lib/tasks/publishing_api.rake:149:in `each_with_index' lib/tasks/publishing_api.rake:149:in `block (3 levels) in <top (required)>' test/unit/tasks/publishing_api_test.rb:165:in `block (3 levels) in <class:PublishingApiRake>' ```
Creating an organisation causes a call to Whitehall::PublishingApi.patch_links due to Active Record callbacks. In Mocha 1.12.0 there was a change (presumably freerange/mocha#474) that causes the test to start failing in it's previous incantation. My understanding is that this fail occurs because the organisation is being created after the mock (`expects`) is applied to Whitehall::PublishingApi, which leads to the test failing because it thinks the `patch_links` method has been invoked twice. As per: ``` Error: patch_links::#organisations#test_patches_links_for_organisations: DRb::DRbRemoteError: unexpected invocation: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e620>, {:bulk_publishing => true}) unsatisfied expectations: - expected exactly once, invoked twice: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e530>, {:bulk_publishing => true}) satisfied expectations: - allowed any number of times, invoked never: #<User:0x3e490>.persisted?(any_parameters) - allowed any number of times, invoked never: #<User:0x3e490>.id(any_parameters) - allowed any number of times, invoked never: Services.asset_manager(any_parameters) (Minitest::Assertion) lib/tasks/publishing_api.rake:152:in `block (4 levels) in <top (required)>' lib/tasks/publishing_api.rake:149:in `each' lib/tasks/publishing_api.rake:149:in `each_with_index' lib/tasks/publishing_api.rake:149:in `block (3 levels) in <top (required)>' test/unit/tasks/publishing_api_test.rb:165:in `block (3 levels) in <class:PublishingApiRake>' ```
Creating an organisation causes a call to Whitehall::PublishingApi.patch_links due to Active Record callbacks. In Mocha 1.12.0 there was a change (presumably freerange/mocha#474) that causes the test to start failing in it's previous incantation. My understanding is that this fail occurs because the organisation is being created after the mock (`expects`) is applied to Whitehall::PublishingApi, which leads to the test failing because it thinks the `patch_links` method has been invoked twice. As per: ``` Error: patch_links::#organisations#test_patches_links_for_organisations: DRb::DRbRemoteError: unexpected invocation: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e620>, {:bulk_publishing => true}) unsatisfied expectations: - expected exactly once, invoked twice: Whitehall::PublishingApi.patch_links(#<Organisation:0x3e530>, {:bulk_publishing => true}) satisfied expectations: - allowed any number of times, invoked never: #<User:0x3e490>.persisted?(any_parameters) - allowed any number of times, invoked never: #<User:0x3e490>.id(any_parameters) - allowed any number of times, invoked never: Services.asset_manager(any_parameters) (Minitest::Assertion) lib/tasks/publishing_api.rake:152:in `block (4 levels) in <top (required)>' lib/tasks/publishing_api.rake:149:in `each' lib/tasks/publishing_api.rake:149:in `each_with_index' lib/tasks/publishing_api.rake:149:in `block (3 levels) in <top (required)>' test/unit/tasks/publishing_api_test.rb:165:in `block (3 levels) in <class:PublishingApiRake>' ```
Fixes #473. Earlier, whenever you qualified an Expectation with a cardinality, a new Cardinality instance would be replace the old one. 59454a8 moved the invocations recording to Cardinality, which then meant that the invocations recorded prior to updating cardinality would get discarded along with the Cardinality instance, effectively being forgotten.
We now update the quantifiers of the same Cardinality instance for each Expectation in order to persist the recording of invocations across updates to cardinality.