rspec / rspec-expectations

Provides a readable API to express expected outcomes of a code example
https://rspec.info
MIT License
1.26k stars 396 forks source link

`Has` doesn't respect keyword arguments #1480

Closed romanbsd closed 1 week ago

romanbsd commented 3 weeks ago

Subject of the issue

When working with capybara, the following fails:

expect(page).to have_field('object_title', with: 'some name')

Your environment

Steps to reproduce

Using capybara do expect(page).to have_field('object_title', with: 'some name')

Expected behavior

It delegates to has_field?('object_title', with: 'some name')

Actual behavior

It calls has_field?('object_title', { with: 'some name' })

I made a following monkey patch to workaround the problem:

module RSpec
  module Matchers
    module BuiltIn
      class Has
        private

        def predicate_result
          if @args.last.is_a?(Hash)
            # Extract the last element of @args and pass it as keyword arguments
            args_without_last = @args[0...-1]
            last_hash = @args.last
            actual.__send__(predicate_method_name, *args_without_last, **last_hash, &@block)
          else
            # Fallback to the default behavior if there's no hash at the end of @args
            actual.__send__(predicate_method_name, *@args, &@block)
          end
        end
      end
    end
  end
end
pirj commented 3 weeks ago

Capybara provides their implementation of have_ methods.

Which one do you call? Why you don’t call the Capybara’s one if they provide it? Does it suffer from the same issue?

If it’s a configuration issue and Capybara matchers were not included, can you please leave a note on what was the culprit for the future researchers?

romanbsd commented 3 weeks ago

I'll check with the team, thanks!

JonRowe commented 3 weeks ago

We actually have tests for passing keyword arguments through a have matcher which pass, so it must not be as simple as you say, but there is a possibility there are still keywork argument bugs around as most of RSpec predates keyword arguments. We can't simply add ** behaviour until we release RSpec 4 due to our supported Rubies.

Can you provide a pure Ruby test that fails?

romanbsd commented 3 weeks ago

Interestingly enough, I cannot reproduce it in a simple case. The capybara code (as I checked in debugger callstack) is from https://github.com/teamcapybara/capybara/blob/master/lib/capybara/session.rb#L774 and then https://github.com/teamcapybara/capybara/blob/master/lib/capybara/node/matchers.rb#L421 However, for this simple test case it passes:

class TestModel
  class Internal
    def has_field?(locator = nil, **options, &optional_filter_block)
      true
    end
  end

  def initialize
    @internal = Internal.new
  end

  def has_field?(...)
    @internal.has_field?(...)
  end
end

RSpec.describe TestModel, type: :model do
  let(:instance) { described_class.new }

  it 'passes keyword arguments' do
    expect(instance).to have_field('secret', with: 'something')
  end
end

I'm puzzled.

JonRowe commented 3 weeks ago

I did some more investigation and at a guess, whats happening is that the capture of the arguments up in the DynamicPrediate parent class is somehow being overriden, so that these hashes are not being marked as ruby2_keywords hashes, by default we use:

        def initialize(method_name, *args, &block)
          @method_name, @args, @block = method_name, args, block
        end
        ruby2_keywords :initialize if respond_to?(:ruby2_keywords, true)

Which captures the keywords as the hash but marks them as ruby2 style which allows the splat to work.

So we're going to need some more information from you as to what else you have that might be monkey patching over the top, can you check the source location of RSpec::Matchers::BuiltIn::DynamicMatcher#initialize in your broken suite?

romanbsd commented 3 weeks ago

I'm having hard time finding the source location:

(byebug) RSpec::Matchers::BuiltIn::DynamicPredicate.method(:initialize).source_location
nil
(byebug) RSpec::Matchers::BuiltIn::DynamicPredicate.method(:new).source_location
nil
JonRowe commented 3 weeks ago

RSpec::Matchers::BuiltIn::DynamicPredicate.instance_method(:initialize).source_location

romanbsd commented 3 weeks ago

.rvm/gems/ruby-3.2.4/gems/rspec-expectations-3.13.1/lib/rspec/matchers/built_in/has.rb", 10

JonRowe commented 3 weeks ago

In your monkey patch, what is the value of Hash.ruby2_keywords_hash(last_hash)

romanbsd commented 2 weeks ago

it's for example:

{:with=>"Beta experiment"}
pirj commented 2 weeks ago

I think what is interesting is the result of the predicate method Hash.ruby2_keywords_hash?(last_hash) which would be true or false.

What tools do you use for debugging, @romanbsd ?

JonRowe commented 2 weeks ago

Sorry yes thats what I meant

romanbsd commented 2 weeks ago

I use byebug, but can try anything else.

pirj commented 2 weeks ago

Can you please check what Hash.ruby2_keywords_hash?(last_hash) returns?

romanbsd commented 1 week ago
(byebug) Hash.ruby2_keywords_hash?(last_hash)
false
JonRowe commented 1 week ago

Ok I think this is likely something else interferring with the ruby2 keyword hash stuff then, in our vanilla implementation it returns true and everything works as expected. I'd recommend trying to eliminate things from your stack (temporarily) until you find what gem / code is interfering with this.

I'm closing this for now but if you can come up with a reproduction please re-open.

romanbsd commented 1 week ago

Any idea what it could be, or what code patterns should I search for?

JonRowe commented 1 week ago

You could try searching your code/dependencies for things monkey patching has's initializer,

romanbsd commented 1 week ago

The Has initializer is not monkey patched as far as I can tell. Is there some mechanism of ruby2_keywords that can be in effect here? How some external library can have an effect on this code?

JonRowe commented 1 week ago

Anything which intefers with the vanilla behaviour of the ruby2_keywords flag or our class and its methods could be at fault, you should also search for anything thats reopening the class (including er including itself into the class form example).

As I mentioned eliminating one gem at a time might be the best way to debug this