Reduce duplication in & between Mock & ObjectMethods #431

nitishr · 2019-12-02T17:40:44Z

Reduce duplication between {Mock,ObjectMethods}#{expects,stubs}
Reduce surface area by making on_stubbing{_method_unnecessarily} private (followed by a rename)

Also closes #468

This will allow us to keep the code DRY by extracting a method with the identical bits

Array will return a single-element array in case the arg is a single value, or a Hash converted to an array in case the arg is a Hash. We then yield the single element or the key value pair, and return the result of the last yield

Mock#{expects,stubs} already handle methods_vs_return_values/method_name_or_hash Note that this change isn't 100% behavior preserving due to the separation of Mockery#stub_method call from the Mock#{expects,stubs} call. It's possible that midway through argument iteration, stub_method raises a StubbingError, which would leave some methods stubbed without having an expectation set for them. However, I think that's still OK, since you wouldn't expect anything useful to happen after a StubbingError anyway. We're changing undocumented/unpublished behavior in a way that shouldn't matter.

Keep the stubba_ prefix to avoid accidental name clash with other valid methods

floehopper · 2020-02-10T16:04:53Z

lib/mocha/mock.rb

+    def anticipates(method_name_or_hash, backtrace = nil, object = Mock.new(@mockery), &block)
+      Array(method_name_or_hash).map do |*args|
+        args = args.flatten
+        method_name = args.shift
+        Mockery.instance.stub_method(object, method_name) unless object.is_a?(Mock)
+        ensure_method_not_already_defined(method_name)
+        expectation = Expectation.new(self, method_name, backtrace)
+        expectation.returns(args.shift) unless args.empty?
+        yield expectation if block
+        @expectations.add(expectation)
+      end.last


Although I appreciate that this reduces the duplication, I'm not convinced that the resultant code is any easier to follow. In fact I think, if anything, it's a bit harder to follow! Let me have a think about how we might better remove the duplication.

Thanks for your review, @floehopper. Would you mind elaborating what aspects of this change you aren't convinced about and/or which ones you think make it harder to follow? If I understand your concerns more specifically, we might be able to come up with a better solution together.

To start with, I can share a bit more of my thinking here...

If it's the anticipates abstraction, I'll admit that I was initially unsure of it as well (and still am, but a lot less than I initially was). The reason I was unsure was I wondered if the need for me to look up (and cite the dictionary definitions in the PR description) was a sign of the concept being too smart/cute/nuanced.

So, I let it mull for a few days in my head and reached the conclusion that anticipates is probably a better abstraction covering both an expectation and an allowance (stub, in mocha's terminology). I've always found expectation to be too strong a term to represent an allowance. (Anticipation might be too strong a term, too - but it does seem less strong than expectation.) So, in the long term, I would even consider/suggest considering renaming, for instance, the class Expectation to Anticipation (and maybe introduce subclasses - Expectation and Allowance). I have of course not fully analyzed that chain of reasoning, and certainly not considered it from an API compatibility point of view. This was only trying to clarify the concepts and their nuances for myself.

Having said that, I'm not a native English speaker and may well have misinterpreted the shades of meanings of the various terms. So, I'd appreciate any corrections to my understanding or thinking.

Sorry for the slow reply. I'll try to explain my reservations:

Although the Mock#anticipates method has removed all the duplication between ObjectMethods#expects, ObjectMethods#stubs, Mock#expects & Mock#stubs (which is great BTW!), it's now really quite a long and "dense" method and, what's more, three of the lines are only executed conditionally. Coming at the final version of this method definition cold, I found it quite hard to follow.

I'm not completely convinced that the inlining of the ArgumentIterator is a net gain. Encapsulating the common processing of the arguments for the four API methods in a single place still makes sense to me, even though its exact abstraction may not be quite right. And having to work out what the combination of calls to Kernel#Array, Array#map, Array#flatten, Array#shift & Array#last is doing adds quite a lot of cognitive overhead to understanding the method definition.

The naming of Mock#anticipates doesn't bother me too much, although I don't think it is quite right. I wonder whether it would make more sense if we made an Expectation have a cardinality of Cardinality.at_least(0) (i.e. stubbing behaviour) by default, inline Mock#anticipates into Mock#stubs, and call Mock#stubs from Mock#expects passing a block setting the expectation cardinality to Cardinality.exactly(1) (i.e. default expecting behaviour). It feels as if the expecting behaviour is a superset of the stubbing behaviour. What do you think...?

Does make sense? I'm sorry I've struggled a bit to explain my thinking!

I've had time to go through the commits one-by-one in more detail this afternoon and I'd like to mention that I found them very easy to follow - so thank you for taking so much care to curate the commits and write useful commit messages.

Having looked through the commits in more detail, I'm inclined to have a go at merging a version of this PR, albeit with some changes as per my comments above. Is that OK with you?

That's very helpful, @floehopper. I now understand your concerns much better.

I like the suggestion of expects constraining the Expectation, rather than stubs loosening it. Part of the reason I extracted anticipates rather than reuse expects, BTW, was to avoid changing the public API of the existing methods, even though the changes would be fully backward compatible (i.e. accepting but not expecting a block or optional arguments). So, that's a consideration.

I'd be happy with you merging a modified version of the PR with the changes you feel comfortable with. We could iterate on top of that for the more controversial/difficult aspects.

@floehopper, I'm planning to push another commit which I think will alleviate some of your concerns, while at the same time, improving mocha's behavior with expectation chaining by making it more consistent across single-method and multiple-method stubs/expects.

The code in that commit or even the approach could possibly be improved, but I'd like to convey the general idea/direction before I go too far.

Is that OK? If you'd rather that I wait for you to finish merging (a version of) this PR, that's fine, too. Just let me know.

BTW, was to avoid changing the public API of the existing methods, even though the changes would be fully backward compatible (i.e. accepting but not expecting a block or optional arguments). So, that's a consideration.

Good point! 😄

Is that OK? If you'd rather that I wait for you to finish merging (a version of) this PR, that's fine, too. Just let me know.

Yes, that's fine - please go ahead!

Done.

I'm completely fine taking any or all of the newly added commits to a different PR so they can be discussed and reviewed on their own (or altogether dropping any/all of them). The newly introduced ExpectationSetting may need some redesign, but I hope the changes convey the general concept and approach.

I like the suggestion of expects constraining the Expectation, rather than stubs loosening it.

Looking at the code after my last few updates, I think the current form expresses the intent better than the change we were thinking of.

def stubs(stubbed_methods_vs_return_values) expects(stubbed_methods_vs_return_values).at_least(0) end

seem more intention-revealing and logical than

def expects(stubbed_methods_vs_return_values) stubs(stubbed_methods_vs_return_values).exactly(1) end

@floehopper, I'm planning to push another commit which I think will alleviate some of your concerns, while at the same time, improving mocha's behavior with expectation chaining by making it more consistent across single-method and multiple-method stubs/expects.

The code in that commit or even the approach could possibly be improved, but I'd like to convey the general idea/direction before I go too far.

Is that OK? If you'd rather that I wait for you to finish merging (a version of) this PR, that's fine, too. Just let me know.

Done.

I'm completely fine taking any or all of the newly added commits to a different PR so they can be discussed and reviewed on their own (or altogether dropping any/all of them). The newly introduced ExpectationSetting may need some redesign, but I hope the changes convey the general concept and approach.

Sorry for the slow reply. Thanks for adding the extra commit - I'm going to take a look at it shortly.

floehopper · 2020-02-10T16:11:00Z

I've added a comment inline relating to the most significant change in this PR. While I agree that removing the duplication is good, I wonder whether there are some better abstractions we could use so the resultant code is easier to understand.

When calling expects/stubs with a hash (method_names_vs_return_values), only the last expectation would get returned. Any further 'expectation setting' methods chained to that call would, therefore, get called only on the last expectation. This seems arbitrary, and neither evident nor intuitive. We now 'extract' the expectation setting methods into a _virtual_ interface, 'implemented' by both Expectation and ExpectationSetting, and return ExpectationSetting instead of Expectation from the expects/stubs methods. This allows us to pass any further expectation setting method calls on to _each_ of the multiple expectations, rather than just the _last_ (or some other arbitrary) single expectation.

nitishr · 2020-02-21T12:56:31Z

Please merge this to stop me pushing more on this branch :-)

Mock wasn't the right place for calling it. The new yield mechanism is more general and less ugly, and allocates responsibilities more appropriately.

Closes freerange#468 and sets us up to reduce the surface area of the internal API exposed to users, by inlining anticipates into expects and using that from stubs

This reduces the surface area of the internal API exposed to users as suggested in freerange#467

Given that the public API of Expectation consists solely of expectation setting methods, and that the matching, verifying and other misc methods aren't part of the public API, it makes sense to equate the Expectation protocol implicitly to the ExpectationSetting protocol. If we were to segregate interfaces, we might wanna have other internal-facing interfaces like ExpectationMatching, etc. By naming this CompositeExpectation, we don't need (as much) to expose it or to include it in/update the documentation because as far as clients are concerned, they still get back an Expectation (technically, something that quacks like an Expectation).

nitishr · 2020-02-21T15:28:52Z

lib/mocha/composite_expectation.rb

+
+    %w[
+      times twice once never at_least at_least_once at_most at_most_once
+      with yields multiple_yields returns raises throws then when in_sequence


Remember to add with_block_given and with_no_block_given when merging

nitishr · 2020-02-21T16:16:28Z

lib/mocha/mock.rb

@@ -109,14 +109,15 @@ class Mock
    #   object.expects(:expected_method_one).returns(:result_one)
    #   object.expects(:expected_method_two).returns(:result_two)
    def expects(method_name_or_hash, backtrace = nil)
-      iterator = ArgumentIterator.new(method_name_or_hash)
-      iterator.each do |*args|
+      CompositeExpectation.new(Array(method_name_or_hash).map do |*args|


Looking at the documentation:

@return [Expectation] last-built expectation which can be further modified by methods on {Expectation}

This introduces a breaking change. So, even if we agree that the new behavior is preferable, we'd have to think about deprecating the old behavior.

There are no existing tests to check for the documented behavior, though.

make arg iteration similar in expects & stubs

22c594f

This will allow us to keep the code DRY by extracting a method with the identical bits

nitishr changed the title ~~Reduce duplication between Mock & ObjectMethods~~ Reduce duplication in & between Mock & ObjectMethods Dec 2, 2019

nitishr added 28 commits December 4, 2019 06:57

Refactor: extract add_expectation to DRY expects & stubs

ffac2ad

Refactor: inline temp - expectation

f700c36

Refactor: inline temp - iterator

da280b1

Refactor: extract anticipates to DRY expects,stubs

7579416

Refactor: inline temp - method

7cff8fa

Refactor: extract stub_method to be moved to Mockery

7494b2d

Refactor: extract & reuse stubbed_method

5f83af5

Refactor: move stub_method to Mockery from ObjectMethods

b9422ba

Refactor: make on_stubbing{,_method_unnecessarily} private

7a691d9

Refactor: rename on_stubbingx to check_stubbingx

bf32420

Refactor: no need for stateful obj for arg iter-n

73cf959

substitute algorithm for ArgumentIterator.each

b34726e

Array will return a single-element array in case the arg is a single value, or a Hash converted to an array in case the arg is a Hash. We then yield the single element or the key value pair, and return the result of the last yield

Refactor: extract anticipates to DRY expects,stubs

547a4d5

Refactor: inline add_expectation

bf7ef4e

no need to pass caller around

84e3ee7

ArgumentIterator.each returns the last result anyway

bfd20d8

Refactor: inline temp - mocker to restrict scope

9761be3

Refactor: prep to move by inlining temp - method

7e7a61b

Refactor: replace ending yield w/ call in sequence

b87733f

call stub_method alongside expectation setting for each method

2c4a5db

add expectation to expectations after fully set up

262ea4c

Refactor: rename stubbed_method->stubba_method_for

5a5ed50

Keep the stubba_ prefix to avoid accidental name clash with other valid methods

Refactor: move ArgumentIterator#each to Mock

5f1c5f4

Refactor: inline each_argument

365884e

Refactor: DRY up {PRE_RUBY_V19,RUBY_V19_AND_LATER}_EXCLUDED_METHODS

19004e7

Refactor: extract anticipates to DRY expects,stubs

201cfaa

Refactor: inline method error_if_frozen

2840411

nitishr force-pushed the mock-argument-iterator branch from 1da001a to 2840411 Compare December 4, 2019 06:57

floehopper added the refactoring label Dec 13, 2019

nitishr requested a review from floehopper January 16, 2020 08:56

floehopper reviewed Feb 10, 2020

View reviewed changes

floehopper self-assigned this Feb 10, 2020

nitishr force-pushed the mock-argument-iterator branch 3 times, most recently from 4214427 to 9bf000a Compare February 19, 2020 18:44

nitishr force-pushed the mock-argument-iterator branch 2 times, most recently from 74d8631 to 5222998 Compare February 20, 2020 09:25

nitishr mentioned this pull request Feb 21, 2020

removing the Spanish Inquisition special case :( #469

Closed

nitishr added 3 commits February 21, 2020 15:03

move Mockery#stub_method call to ObjectMethods

b80cb98

Mock wasn't the right place for calling it. The new yield mechanism is more general and less ugly, and allocates responsibilities more appropriately.

remove the Spanish Inquisition special case

8a7af66

Closes freerange#468 and sets us up to reduce the surface area of the internal API exposed to users, by inlining anticipates into expects and using that from stubs

Refactor: inline anticipates into expects

9930e5a

This reduces the surface area of the internal API exposed to users as suggested in freerange#467

nitishr force-pushed the mock-argument-iterator branch from caea3a3 to 9930e5a Compare February 21, 2020 15:08

nitishr commented Feb 21, 2020

View reviewed changes

floehopper changed the base branch from master to main July 24, 2020 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce duplication in & between Mock & ObjectMethods #431

Reduce duplication in & between Mock & ObjectMethods #431

nitishr commented Dec 2, 2019 •

edited

floehopper Feb 10, 2020 •

edited

nitishr Feb 11, 2020

floehopper Feb 17, 2020

nitishr Feb 17, 2020

floehopper Feb 17, 2020

nitishr Feb 19, 2020

floehopper Feb 19, 2020

nitishr Feb 19, 2020

nitishr Feb 21, 2020

floehopper Mar 21, 2020 •

edited

floehopper commented Feb 10, 2020

nitishr commented Feb 21, 2020

nitishr Feb 21, 2020

nitishr Feb 21, 2020

Reduce duplication in & between Mock & ObjectMethods #431

Are you sure you want to change the base?

Reduce duplication in & between Mock & ObjectMethods #431

Conversation

nitishr commented Dec 2, 2019 • edited

floehopper Feb 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floehopper Mar 21, 2020 • edited

Choose a reason for hiding this comment

floehopper commented Feb 10, 2020

nitishr commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishr commented Dec 2, 2019 •

edited

floehopper Feb 10, 2020 •

edited

floehopper Mar 21, 2020 •

edited