rspec / rspec-expectations

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

Ruby 3.0.3 and keyword arguments #1350

Open enthusiasmus opened 2 years ago

enthusiasmus commented 2 years ago

Subject of the issue

When using ruby 3.0.3 manually defined chained rspec matchers with keyword parameters don't forwards the arguments correctly

Your environment

Description

Checkout the file: https://github.com/rspec/rspec-expectations/blob/main/lib/rspec/matchers/dsl.rb Go to row 304 and 305, they currently look like this:

          define_user_override(method_name, definition) do |*args, &block|
            super(*args, &block)

and accordingly to this explanation for ruby 3 they should look like

          define_user_override(method_name, definition) do |*args, **kwargs, &block|
            super(*args, **kwargs, &block)

Am I overseeing something? Thanks in advance! Best regards, Lukas

pirj commented 2 years ago

don't forwards the arguments correctly

Can you please provide a reproducible example?

and accordingly to this explanation for ruby 3

Check this an this for more insight.

JonRowe commented 2 years ago

Ruby keyword arguments are tricky, there is no correct way to capture them and maintain compatibility with Ruby 2, the best approach is to add ruby2_keywords to methods that might be passed keyword arguments, which I thought we'd mostly done, to retain the older behaviour that exposed them as a hash, using **kwargs internally isn't possible for us with this version of RSpec as we support older versions of Ruby that don't understand that syntax, and in any case doesn't replicate the ruby 2 behaviour entirely (we've tried conditionally applying that syntax and it doens't behave the same).

In any case can you provide a snippet of how you encountered this issue (not our code, but your code that triggered it) because in many cases if you're creating a custom matcher or using blocks you can work around this by simply capturing the keywords using **kwargs yourself

dblock commented 2 years ago

Could this be the cause of https://github.com/rspec/rspec-mocks/issues/1505?

13k commented 2 years ago

_spec/support/kwargs_chainmatcher.rb

RSpec::Matchers.define :work_with_ruby3_kwargs do
  match do |actual|
    true
  end

  chain :but do |value, does: true|
    true
  end
end

_spec/spechelper.rb

require_relative "support/kwargs_chain_matcher"
# ...

_spec/kwargs_chainspec.rb

RSpec.describe "ruby 3 kwargs chain matcher" do
  it "should work" do
    expect("kwargs chain matcher").to work_with_ruby3_kwargs.but("it", does: :not)
  end
end
bundle exec rspec spec/kwargs_chain_spec.rb
F

Failures:

  1) ruby 3 kwargs chain matcher should work
     Failure/Error:
       chain :but do |value, does: true|
         true
       end

     ArgumentError:
       wrong number of arguments (given 2, expected 1)
     # ./spec/support/kwargs_chain_matcher.rb:8:in `block (2 levels) in <top (required)>'
     # ./spec/kwargs_chain_spec.rb:5:in `block (2 levels) in <top (required)>'

Finished in 0.0034 seconds (files took 0.13517 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/kwargs_chain_spec.rb:4 # ruby 3 kwargs chain matcher should work
13k commented 2 years ago

capturing **kwargs in the proc also doesn't work:

  chain :also_capturing_but do |value, **kwargs|
    true
  end
  context "when capturing kwargs" do
    it "should work" do
      expect("kwargs chain matcher").to work_with_ruby3_kwargs.also_capturing_but("it", does: :not)
    end
  end
  2) ruby 3 kwargs chain matcher when capturing kwargs should work
     Failure/Error:
       chain :also_capturing_but do |value, **kwargs|
         true
       end

     ArgumentError:
       wrong number of arguments (given 2, expected 1)
     # ./spec/support/kwargs_chain_matcher.rb:12:in `block (2 levels) in <top (required)>'
     # ./spec/kwargs_chain_spec.rb:10:in `block (3 levels) in <top (required)>'
johncarney commented 1 year ago

FWIW, I'm encountering this problem as I am trying to migrate a number of apps to Ruby 3 due to the imminent EOL of Ruby 2. In the short term, I'm probably going to monkey-patch around it as follows, but it's not ideal, obviously.

module RSpec
  module Matchers
    module DSL
      module Macros
        def chain(method_name, *attr_names, &definition)
          unless block_given? ^ attr_names.any?
            raise ArgumentError, "You must pass either a block or some attribute names (but not both) to `chain`."
          end

          definition = assign_attributes(attr_names) if attr_names.any?

          define_user_override(method_name, definition) do |*args, **kwargs, &block|
            super(*args, **kwargs, &block)
            @chained_method_clauses.push([method_name, [*args, kwargs]])
            self
          end
        end
      end
    end
  end
end

I haven't properly tested the above, so it might break on certain use-cases.

pirj commented 1 year ago

A quick fix for both examples with chain above is to add this at dsl.rb:310:

            define_user_override(method_name, definition) do |*args, &block|
              super(*args, &block)
              @chained_method_clauses.push([method_name, args])
              self
            end
+           ruby2_keywords method_name if respond_to?(:ruby2_keywords, true)
          end

A PR is welcome with this and a spec covering the case with chain.

mathieujobin commented 1 year ago

@JonRowe

... internally isn't possible for us with this version of RSpec as we support older versions of Ruby that don't understand that syntax, ...

it should be able to support it if we drop support for 2.6 and below, keeping 2.7 for another year.

I'm not sure those are still all needed. https://github.com/rspec/rspec-expectations/blob/8a468eea0a2ee456db4d6e2dccccd66ff269b862/.github/workflows/ci.yml#L48-L52

Also maybe its time to bump https://github.com/rspec/rspec-expectations/blob/8a468eea0a2ee456db4d6e2dccccd66ff269b862/rspec-expectations.gemspec#L29

JonRowe commented 1 year ago

@mathieujobin our plans are to do just that in the next major version of RSpec, but theres a few things to organise to get that onde.