Skip to content
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

Show proof of concept for keyword arg matching #534

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ Style/Alias:
Style/Documentation:
Enabled: false

# Useful for distinuishing between positional & keyword arguments
Style/BracesAroundHashParameters:
Enabled: false

# Enumerable#each_with_object only available since Ruby v1.9
Style/EachWithObject:
Enabled: false
Expand Down
2 changes: 2 additions & 0 deletions lib/mocha/expectation.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'mocha/ruby_version'
require 'mocha/method_matcher'
require 'mocha/parameters_matcher'
require 'mocha/expectation_error'
Expand Down Expand Up @@ -222,6 +223,7 @@ def with(*expected_parameters, &matching_block)
@parameters_matcher = ParametersMatcher.new(expected_parameters, &matching_block)
Copy link
Contributor Author

@wasabigeek wasabigeek Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, since expected_parameters can be matchers as well, would that break matchers for keyword arguments? e.g. the has_value matcher

object = mock()
object.expects(:method_1).with(has_value(1)) # has_value probably interpreted as a positional argument
object.method_1('key_1' => 1, 'key_2' => 2) # does not match anymore?

I'll check a test case.

Copy link
Contributor Author

@wasabigeek wasabigeek Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current implementation it still works, this passes:

def test_has_value
  created_widget = Widget.new
  Widget.expects(:create).with(has_value('wombat')).returns(created_widget)
  assert_equal created_widget, Widget.create(:model => 'wombat')
end

However, I think it's because the keyword arg check only happens if the last expected param is a Hash, which in this case it isn't:

if (last_parameter = @expected_parameters.last).is_a?(Hash)

This might become problem when we want to refactor parameters in the future (see comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To discuss further in #534 (comment)

self
end
ruby2_keywords(:with) if RUBY_V3_PLUS

# Modifies expectation so that the expected method must be called with a block.
#
Expand Down
2 changes: 2 additions & 0 deletions lib/mocha/mock.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'mocha/ruby_version'
require 'mocha/expectation'
require 'mocha/expectation_list'
require 'mocha/invocation'
Expand Down Expand Up @@ -318,6 +319,7 @@ def method_missing(symbol, *arguments, &block) # rubocop:disable Style/MethodMis
raise_unexpected_invocation_error(invocation, matching_expectation)
end
end
ruby2_keywords(:method_missing) if RUBY_V3_PLUS

# @private
def respond_to_missing?(symbol, include_all)
Expand Down
26 changes: 26 additions & 0 deletions lib/mocha/parameter_matchers/last_positional_hash.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'mocha/parameter_matchers/base'
require 'mocha/ruby_version'

module Mocha
module ParameterMatchers
# @private
class LastPositionalHash < Base
def initialize(value)
@value = value
end

def matches?(available_parameters)
parameter = available_parameters.shift
if RUBY_V3_PLUS
return false unless Hash.ruby2_keywords_hash?(@value) == Hash.ruby2_keywords_hash?(parameter)
end

parameter == @value
end

def mocha_inspect
@value.mocha_inspect
end
end
end
end
7 changes: 6 additions & 1 deletion lib/mocha/parameters_matcher.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'mocha/inspect'
require 'mocha/parameter_matchers'
require 'mocha/parameter_matchers/last_positional_hash'

module Mocha
class ParametersMatcher
Expand Down Expand Up @@ -28,7 +29,11 @@ def mocha_inspect
end

def matchers
@expected_parameters.map(&:to_matcher)
if (last_parameter = @expected_parameters.last).is_a?(Hash)
@expected_parameters[0...-1].map(&:to_matcher) + [ParameterMatchers::LastPositionalHash.new(last_parameter)]
wasabigeek marked this conversation as resolved.
Show resolved Hide resolved
else
@expected_parameters.map(&:to_matcher)
end
end
end
end
1 change: 1 addition & 0 deletions lib/mocha/ruby_version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
module Mocha
RUBY_V2_PLUS = Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('2')
RUBY_V3_PLUS = Gem::Version.new(RUBY_VERSION.dup) >= Gem::Version.new('3')
end
4 changes: 2 additions & 2 deletions test/acceptance/stubba_example_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ def should_stub_two_different_class_methods
found_widgets = [Widget.new]
created_widget = Widget.new
Widget.expects(:find).with(:all).returns(found_widgets)
Widget.expects(:create).with(:model => 'wombat').returns(created_widget)
Widget.expects(:create).with({ :model => 'wombat' }).returns(created_widget)
assert_equal found_widgets, Widget.find(:all)
assert_equal created_widget, Widget.create(:model => 'wombat')
assert_equal created_widget, Widget.create({ :model => 'wombat' })
Copy link
Contributor Author

@wasabigeek wasabigeek Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, Mock#method_missing is not tagging the last hash as a ruby2_keywords_hash 🤔. Current suspicion is the way InstanceMethod#define_new_method passes arguments doesn't recognise the final arg as a kwarg:

stub_method_owner.send(:define_method, method_name) do |*args, &block|
  self_in_scope.mock.method_missing(method_name_in_scope, *args, &block)
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it help if you add **kwargs to the block params and the method_missing params? i.e.

stub_method_owner.send(:define_method, method_name) do |*args, **kwargs, &block|
  self_in_scope.mock.method_missing(method_name_in_scope, *args, **kwargs, &block)
end

Copy link
Contributor Author

@wasabigeek wasabigeek Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting ruby2_keywords on the methods through the flow works (see 96e71ad), but there probably is another way that doesn't implicitly couple code there 🤔 At least the acceptance test passes now.

Kwargs is another possibility, but I think we might need to refactor a lot more (most of the code expects an array of arguments instead of having the arguments split into positional and keyword args, plus we probably still need to have separate method definitions for other Ruby versions). Good reference: https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha! That blog post looks very helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was discussing with @chrisseaton (thanks!), and he suggested to try using ruby2_keywords only at the "entry points" (e.g. Mock#method_missing and StubbedMethod#define_new_method). At that point, we can already split between positional and keyword arguments, which should make subsequent logic cleaner instead of having to set/check for ruby2_keywords at multiple layers (e.g. 96e71ad).

It sounded like a great idea, so I started spiking a possible encapsulation of parameters here, but it's looking to be a pretty large and tricky change.

@floehopper, how do you feel about the refactor? If agreeable, do you think we should attempt to refactor first, or should I try to finish this current approach first (setting ruby2_keywords at multiple layers) before circling back?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@floehopper I've sketched out a refactor here [...]

@wasabigeek I'm a bit confused where "here" is, i.e. where I should be looking! Is it on the branch associated with this PR? And if so what's the best way for me to look at it - should I just look at the net diff of all the changes or each commit in turn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I was referencing another PR: #544, that has the proposed refactor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wasabigeek:

API-wise, I think there will be some ambiguity when hash matchers are involved e.g. is .with(has_value(“hello”)) matching a last positional Hash, or a kwarg? I think we can possibly:

  • try to allow hash matchers to check for both last positional Hash and kwargs, passing if either scenario matches (which would be ambiguous but shouldn't break existing test suites, and maybe we can phase this out later in favour of explicitness)

  • introduce a way to explicitly declare a kwarg matcher, e.g.:

    • .with(arg, kwargs(has_value(“hello”)))
    • .with(arg, kwargs: has_value(“hello”))

What do you think?

Ideally I'd like to provide as simple an upgrade path as possible given that this change might mean quite pervasive changes in a lot of codebases. I think that means having a backwardly compatible version of the code (ideally with deprecation warnings highlighting the ambiguities) and a new version of the code which requires explicitness. It would be nice if the two versions of the code could co-exist in the same release of Mocha behind a configuration option, but if that makes the code ridiculously complicated, it would be fine to have the two versions of the code in separate releases of Mocha (separated by a major version bump). In the latter case, it would be good to have finalised the new API by the time of the 1st release, so the deprecation warnings could explain how to fix them as clearly as possible.

I haven't had much time to think about the API, but of the two options you've proposed, I think I prefer .with(arg, kwargs(has_value(“hello”))) - it seems a bit more in keeping with the existing API. I guess another option might be to have a separate method on Expectation, e.g. with_kwargs.

Do you think we might need to make the positional argument matching more explicit too, i.e. .with(args(arg1, arg2), kwargs(has_value(“hello”)))...?

If we do use any of these approaches, I'd probably prefer more verbose names, i.e. keyword_arguments vs kwargs; maybe with aliases for the abbreviations...

Just to check my understanding, am I right in thinking the ambiguity mentioned above will eventually go away if/when we stop supporting Ruby v2? If that's the case, would it make any sense to have a configuration option (or code that detects the Ruby version) that allows a simpler Expectation#with signature (i.e. the same as the existing signature) when your using Ruby v3...? Apologies if I've misunderstood this!

I hope that helps. Thanks again for your work on this. Let me know if you have any more questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I forgot to respond to this!

I guess another option might be to have a separate method on Expectation, e.g. with_kwargs.
Do you think we might need to make the positional argument matching more explicit too, i.e. .with(args(arg1, arg2), kwargs(has_value(“hello”)))...?

I don't have a strong opinion at the moment, though it feels like args(...) wrapper changes the API without adding much. with_kwargs could be a more straightforward alternative 🤔

Just to check my understanding, am I right in thinking the ambiguity mentioned above will eventually go away if/when we stop supporting Ruby v2?

I don't think so, unless we make a breaking change in with's signature (i.e. the ambiguity is more from mocha's existing API than from Ruby itself). The tricky part is determining whether something like .expects(:foo).with(has_value(1)) is trying to exclusively match a last positional hash (e.g. .foo({ a: 1})) or a kwarg (e.g. .foo(a: 1)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wasabigeek

I guess another option might be to have a separate method on Expectation, e.g. with_kwargs.
Do you think we might need to make the positional argument matching more explicit too, i.e. .with(args(arg1, arg2), kwargs(has_value(“hello”)))...?

I don't have a strong opinion at the moment, though it feels like args(...) wrapper changes the API without adding much. with_kwargs could be a more straightforward alternative 🤔

Fair point.

Just to check my understanding, am I right in thinking the ambiguity mentioned above will eventually go away if/when we stop supporting Ruby v2?

I don't think so, unless we make a breaking change in with's signature (i.e. the ambiguity is more from mocha's existing API than from Ruby itself). The tricky part is determining whether something like .expects(:foo).with(has_value(1)) is trying to exclusively match a last positional hash (e.g. .foo({ a: 1})) or a kwarg (e.g. .foo(a: 1)).

Ah, I think I see what you mean now - thanks for explaining. 👍

end

def should_stub_instance_method_on_any_instance_of_a_class
Expand Down
23 changes: 23 additions & 0 deletions test/unit/expectation_test.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require File.expand_path('../../test_helper', __FILE__)
require 'mocha/ruby_version'
require 'mocha/expectation'
require 'mocha/invocation'
require 'mocha/sequence'
Expand Down Expand Up @@ -67,6 +68,28 @@ def test_should_not_match_calls_to_same_method_with_parameters_not_constrained_a
assert !expectation.match?(Invocation.new(:irrelevant, :expected_method, 1, 0, 3))
end

def test_should_match_last_positional_hash
expectation = new_expectation.with(1, { :a => 1 })
assert expectation.match?(Invocation.new(:irrelevant, :expected_method, 1, { :a => 1 }))
end

if RUBY_V3_PLUS
def test_should_match_keyword_args
expectation = new_expectation.with(1, Hash.ruby2_keywords_hash({ :a => 1 }))
assert expectation.match?(Invocation.new(:irrelevant, :expected_method, 1, Hash.ruby2_keywords_hash({ :a => 1 })))
end

def test_should_not_match_keyword_args_with_last_positional_hashes
expectation = new_expectation.with(1, Hash.ruby2_keywords_hash({ :a => 1 }))
assert !expectation.match?(Invocation.new(:irrelevant, :expected_method, 1, { :a => 1 }))
end

def test_should_not_match_last_positional_hashes_with_keyword_args
expectation = new_expectation.with(1, { :a => 1 })
assert !expectation.match?(Invocation.new(:irrelevant, :expected_method, 1, Hash.ruby2_keywords_hash({ :a => 1 })))
end
end
wasabigeek marked this conversation as resolved.
Show resolved Hide resolved

def test_should_allow_invocations_until_expected_invocation_count_is_one_and_actual_invocation_count_would_be_two
expectation = new_expectation.times(1)
assert expectation.invocations_allowed?
Expand Down