with_arguments adds non-documented arguments to delegate method call

[Burgestrand]

Sorry about raising an issue without a PR, I had bin/setup issues with Puma/bundler.

The attached code, more or less copied from the documentation does not run successfully. It fails with an error. Code example at the bottom of this issue.

It's not clear to me if the documentation is wrong, or if the documentation is intended to work as it is written.

Implementation details

The reason is that the delegated arguments are not only used in the matcher, they're also added to the delegated method call (see line 423):

shoulda-matchers/lib/shoulda/matchers/independent/delegate_method_matcher.rb

Lines 419 to 425 in 64376a0

	def call_delegating_method_with_delegate_method_returning(value)
	register_subject_double_collection_to(value)
	Doublespeak.with_doubles_activated do
	subject.public_send(delegating_method, *delegated_arguments)
	end
	end

The test suite doesn't catch this because the method under test has its' arguments defined as * (line 267):

shoulda-matchers/spec/unit/shoulda/matchers/independent/delegate_method_matcher_spec.rb

Lines 266 to 274 in 64376a0

	define_class('PostOffice') do
	def deliver_mail(*)
	mailman.deliver_mail('221B Baker St.', hastily: true)
	end
	def mailman
	Mailman.new
	end
	end

I tried to dig a bit in the git history to figure out why it accepts any arguments, but I didn't find anything conclusive.

Reproducible code

Save to a file, run it using Ruby, e.g. ruby example.rb.

Click here for error output

Run options: --seed 62691

# Running:

E

Finished in 0.000794s, 1259.4459 runs/s, 0.0000 assertions/s.

  1) Error:
CourierTest#test_: Courier should delegate #deliver_package to the #post_office object passing arguments [{:expedited=>true}]. :
ArgumentError: wrong number of arguments (given 1, expected 0)
    isolated.rb:31:in `deliver_package'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-matchers-4.4.1/lib/shoulda/matchers/independent/delegate_method_matcher.rb:422:in `public_send'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-matchers-4.4.1/lib/shoulda/matchers/independent/delegate_method_matcher.rb:422:in `block in call_delegating_method_with_delegate_method_returning'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-matchers-4.4.1/lib/shoulda/matchers/doublespeak/world.rb:29:in `with_doubles_activated'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/2.7.0/forwardable.rb:235:in `with_doubles_activated'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-matchers-4.4.1/lib/shoulda/matchers/independent/delegate_method_matcher.rb:421:in `call_delegating_method_with_delegate_method_returning'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-matchers-4.4.1/lib/shoulda/matchers/independent/delegate_method_matcher.rb:384:in `subject_delegates_to_delegate_object_correctly?'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-matchers-4.4.1/lib/shoulda/matchers/independent/delegate_method_matcher.rb:204:in `matches?'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/assertions.rb:55:in `assert_accepts'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:62:in `block in should'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:70:in `instance_exec'
    /Users/kim/.asdf/installs/ruby/2.7.1/lib/ruby/gems/2.7.0/gems/shoulda-context-2.0.0/lib/shoulda/context/context.rb:70:in `block in create_test_from_should_hash'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

# frozen_string_literal: true

require "bundler/inline"
require "minitest/autorun"

gemfile do
  source "https://rubygems.org/"

  gem "shoulda-matchers", "~> 4.0"
  gem "shoulda-context"
end

Shoulda::Matchers.configure do |config|
  config.integrate do |with|
    with.test_framework :minitest
  end
end

class Courier
  class PostOffice
    def deliver_package(expedited:)
    end
  end

  attr_reader :post_office

  def initialize
    @post_office = PostOffice.new
  end

  def deliver_package # NOTE: this method accepts no arguments, which is the problem
    post_office.deliver_package(expedited: true)
  end
end

class CourierTest < Minitest::Test
  should delegate_method(:deliver_package).
    to(:post_office).
    with_arguments(expedited: true)
end

[mcmire]

Hey @Burgestrand, thanks for raising this issue. I think the intent here to be able to test that if you have a situation where you are manually delegating to another object, if arguments are passed to the delegating method, they get successfully passed on to the target method. So I think the documentation is incorrect. As for the tests, I checked the history and they are very old. So it's very possible that they're poorly written and that the fact that they don't represent reality very well has never been noticed. But my feeling is that in order to reflect the intent, Courier#deliver_package should take an expedited keyword argument so that it can pass it on to PostOffice#deliver_package, and that the tests should reflect this more accurately.

[vsppedro]

Sorry about raising an issue without a PR, I had bin/setup issues with Puma/bundler.

Hi, @Burgestrand, I'm sorry to hear that. Can you give me more details about the problem, please? I'll take a look later.

When you have time, please take a look at this PR and see if it solves the problem you faced: #1373

Thanks!

EDIT: You can test on master now! 😄

[Burgestrand]

But my feeling is that in order to reflect the intent, Courier#deliver_package should take an expedited keyword argument so that it can pass it on to PostOffice#deliver_package, and that the tests should reflect this more accurately.

I don't have a strong opinion. I can see both being useful, but given this issue hasn't been raised before that's an argument against it 😄

---

When I came across this I was trying to use it as documented. Here's roughly what I intended to test:

def ignored?
  repository.ignored?(order_id: @id)
end

I expected it to be testable as such:

should delegate_method(:ignored?).to(:repository).with_arguments(order_id: "5")

[Burgestrand]

[…]
Hi, @Burgestrand, I'm sorry to hear that. Can you give me more details about the problem, please? I'll take a look later.
[…]
EDIT: You can test on master now! 😄

@vsppedro I tried master now, fresh install of Ruby 2.7.2 with asdf.

• I ran into an issue due to bundler version mismatch. It appears it does gem install bundler which for me installs 2.1.4. It later fails the bundle check/install due to the Gemfile.lock using version 1.17.3 and I don't have that installed.
• I manually install bundler gem install bundler -v 1.17.3
• Re-run bin/setup — it takes a while, eventually fails for rails_6_0.gemfile: puma. I believe I run into ctype.h missing from puma_http11.c puma/puma#2304
• I manually edit the gemfile.lock for Rails 6 to use puma version 4.3.7 instead of 4.3.1
• bin/setup now succeeds all the way to Setup complete!

[vsppedro]

So, we need to update some dependencies so that it doesn't happen again! 😄

Thanks!

[Burgestrand]

So, we need to update some dependencies so that it doesn't happen again! 😄

Thanks!

Happy to help 😊