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

Compatibility of shared_examples with Ruby 3 #2674

Closed
WojciechKo opened this issue Dec 12, 2019 · 8 comments · Fixed by #2719
Closed

Compatibility of shared_examples with Ruby 3 #2674

WojciechKo opened this issue Dec 12, 2019 · 8 comments · Fixed by #2719

Comments

@WojciechKo
Copy link

Subject of the issue

Compatibility of shared_examples with a "Real" keyword argument from upcoming Ruby 3

Your environment

  • Ruby version: 2.7.0preview3
  • rspec-core version: 3.9.0

Steps to reproduce

RSpec.describe 'ruby 2.7.0.preview3 warning' do
  shared_examples 'expects hash' do |hash|
    it { expect(hash[:input].reverse).to eq(hash[:output]) }
  end

  shared_examples 'expects keywords' do |input:, output:|
    it { expect(input.reverse).to eq(output) }
  end

  include_examples 'expects hash', input: 'afla', output: 'alfa'
  include_examples 'expects hash', { input: 'ovarb', output: 'bravo' }

  include_examples 'expects keywords', input: 'eilrahc', output: 'charlie' # Warning
  include_examples 'expects keywords', { input: 'atled', output: 'delta' } # Warning
end

Expected behavior

  • charlie example should not produce any warning
  • alfa should not be supported to follow Ruby 3 changes (?)

Actual behavior

Following warning is being printed for charlie and delta case

/home/wojciechko/.rbenv/versions/2.7.0-preview3/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.0/lib/rspec/core/shared_example_group.rb:36: warning: The last argument is used as the keyword parameter
/home/wojciechko/dev/sample_spec.rb:6: warning: for method defined here
@JonRowe
Copy link
Member

JonRowe commented Dec 13, 2019

Work here is needed to figure out what they've changed and why this is issuing a warning, I suspect we just haven't considered keyword argument at all and they've just worked due to their hash like nature. The implementation should be agnostic to what is passed in and just pass through the arguments. (Meaning the "alfa" example will not work on ruby 3, but continue to work everywhere else).

@jaynetics
Copy link

This also affects rspec-expectations: rspec/rspec-expectations#1150
and rspec-mocks: rspec/rspec-mocks#1306

I was beginning to prepare PRs for these things that simply pass **kwargs trough all participating methods. This does fix these issues. However, in rspec-core that produced some other problems with example metadata being lost, which apparently passes through the same code paths. While I looked into that I noticed to my amazement that you still support ruby 1.9 and even 1.8, so that whole approach won't even work (at least without a lot of insane, RUBY_VERSION-based eval'ing).

Perhaps this is a sensible time to stop supporting these versions?

@JonRowe
Copy link
Member

JonRowe commented Dec 27, 2019

Metadata is a hash, so its not going to work treating it as keyword arguments everywhere, as we expect it to be a hash. So this warning is actually a symptom of mis-using the implementation.... So I'm kind of tempted to issue an explicit warning explaining that rather than trying to add explicit support for this behaviour.

While I looked into that I noticed to my amazement that you still support ruby 1.9 and even 1.8, so that whole approach won't even work (at least without a lot of insane, RUBY_VERSION-based eval'ing).

Perhaps this is a sensible time to stop supporting these versions?

I'm afraid that won't happen until the next major version due to semver.

@pirj
Copy link
Member

pirj commented Dec 28, 2019

Wondering if the change to:

-      def self.include_examples(name, *args, &block)
-        find_and_eval_shared("examples", name, caller.first, *args, &block)
+      def self.include_examples(name, *args, **kwargs, &block)
+        find_and_eval_shared("examples", name, caller.first, *args, **kwargs, &block)

will do the job.
Still, define_nested_shared_group_method and include_context needs to be tweaked the same way.
Please correct me if I'm wrong, it seems there's no way around eval and adding strict_kwargs? to rspec-support.

A temporary measure around warnings can be the usage of ruby2_keywords macro (also needs to be wrapped, to only be called when it's available).

@pirj
Copy link
Member

pirj commented Dec 28, 2019

Related rubocop/rubocop#7584

@JonRowe
Copy link
Member

JonRowe commented Dec 28, 2019

Yeah I mean you should be able to remove the warnings by just accepting the hash as is...

@pirj
Copy link
Member

pirj commented Jan 15, 2020

Would you like to be interested in sending a pull request to fix that issue @WojciechKo ?

@JonRowe
Copy link
Member

JonRowe commented Apr 10, 2020

We fixed this in #2716 but adding further specs in #2719, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants