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 all 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
1 change: 1 addition & 0 deletions lib/mocha/invocation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ def initialize(mock, method_name, *arguments, &block)
@yields = []
@result = nil
end
ruby2_keywords(:initialize) if RUBY_V3_PLUS

def call(yield_parameters = YieldParameters.new, return_values = ReturnValues.new)
yield_parameters.next_invocation.each do |yield_args|
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
1 change: 1 addition & 0 deletions lib/mocha/stubbed_method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ def define_new_method
stub_method_owner.send(:define_method, method_name) do |*args, &block|
self_in_scope.mock.method_missing(method_name_in_scope, *args, &block)
end
stub_method_owner.send(:ruby2_keywords, method_name) if RUBY_V3_PLUS
retain_original_visibility(stub_method_owner)
end

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