Resolve an issue using method doubles with keyword arguments on Ruby 3 by bryanp · Pull Request #1385 · rspec/rspec-mocks

bryanp · 2020-12-29T21:31:18Z

This captures and resolves an issue I encountered when running my test suite on Ruby 3. It may resolve more issues than just with and_call_original but that needs more investigation. Happy to fill in more test cases here but I haven't gone through to see what else uses MethodDouble yet.

bryanp · 2020-12-29T21:39:01Z

Well this approach clearly has some issues for Ruby < 2.7.

bryanp · 2020-12-29T21:57:28Z

1f2d12b fixes some of the test failures, but I'd prefer to use the ruby2_keywords shim. Are we okay adding that dependency?

JonRowe · 2020-12-29T22:30:18Z

          define_method(method_name) do |*args, &block|
            method_double.proxy_method_invoked(self, *args, &block)
          end
+          ruby2_keywords(method_name) if respond_to?(:ruby2_keywords, true)


Aha! Thats where its needed.

@bryanp 👏

Just wondering how did you figure it out? 2.7.1's warning traces are kind of unhelpful.

@pirj I found a test case that raised an exception, so there was a pretty strong hint in the backtrace:

ArgumentError: wrong number of arguments (given 1, expected 0; required keyword: bar) # ./spec/rspec/mocks/and_call_original_spec.rb:189:in `foo' # ./lib/rspec/mocks/message_expectation.rb:101:in `call' # ./lib/rspec/mocks/message_expectation.rb:101:in `block in and_call_original' # ./lib/rspec/mocks/message_expectation.rb:740:in `call' # ./lib/rspec/mocks/message_expectation.rb:572:in `invoke_incrementing_actual_calls_by' # ./lib/rspec/mocks/message_expectation.rb:427:in `invoke' # ./lib/rspec/mocks/proxy.rb:217:in `message_received' # ./lib/rspec/mocks/proxy.rb:361:in `message_received' # ./lib/rspec/mocks/method_double.rb:78:in `proxy_method_invoked' # ./lib/rspec/mocks/method_double.rb:64:in `block (2 levels) in define_proxy_method' # ./spec/rspec/mocks/and_call_original_spec.rb:191:in `block (3 levels) in <top (required)>'

Ah yes, sadly 2.7 is less helpful

Sorry for the inconvenience 🙏
Just a tip: if you run it on Ruby 3.0, it will raise an exception instead of a warning, which will show a more helpful backtrace.

JonRowe · 2020-12-29T22:31:17Z

+      def instance.foo(bar:); bar; end
+      expect(instance).to receive(:foo).and_call_original
+      expect(instance.foo(bar: "baz")).to eq("baz")
+    end


This needs to be wrapped in an eval for our older ruby test suite, and skipped when theres no keyword argument support. (RSpec 3 supports 1.8.7 up).

An example https://github.com/rspec/rspec-expectations/blob/955151da74fd6356e614e62addb3da26ea1ccaf6/spec/rspec/matchers/built_in/has_spec.rb#L20

Ah, makes sense. Did that here: 1433a42

pirj · 2020-12-29T23:23:45Z

Triggered a dummy build to figure out if those failures are due to the change or unrelated #1386

bryanp · 2020-12-30T02:36:33Z

Addressed feedback so far but still getting failures that seem unrelated to my change. Ideas?

JonRowe · 2021-01-01T13:12:29Z

So the build failure is from this change, I can reproduce it on a clean build with only that line changing (spec failed 3.0 and 2.7 but passed 2.5 2.6, add that one line and 3.0 and 2.7 pass but 2.5 2.6 fails)

The why still escapes me for the moment.

bryanp · 2021-01-02T21:47:19Z

Have you reproduced the failures locally? I've only seen them on CI so far.

JonRowe · 2021-01-04T15:10:11Z

@bryanp yes, they are from rspec-rails when combined with this branch

bryanp · 2021-01-05T15:26:38Z

@JonRowe What's the best way to locally run the rspec-rails specs against this branch?

JonRowe · 2021-01-06T08:39:11Z

@bryanp The RSpec repos will automatically run against the local version that sits in a folder next to them, e.g. if you have:

~/code/rspec-rails
~/code/rspec-mocks

Doing a bundle install in rspec-rails and then running the specs should fail if you have this branch checked out on rspec-mocks.

I am going to look at this when I have the time, but if you beat me to it I will be ecstatic :)

bryanp · 2021-01-06T14:45:59Z

Aha, finally reproduced the failure. I'm looking specifically at this one:

rspec ./spec/rspec/rails/example/view_example_group_spec.rb:114 # RSpec::Rails::ViewExampleGroup routes helpers collides with asset helpers uses routes helpers

Here's the important part of the failure:

TypeError:
       can't create instance of singleton class
     # /Users/bryanp/.gem/ruby/2.6.6/gems/railties-6.0.3.4/lib/rails/railtie.rb:167:in `new'
     # /Users/bryanp/.gem/ruby/2.6.6/gems/railties-6.0.3.4/lib/rails/railtie.rb:167:in `instance'
     # /Users/bryanp/.gem/ruby/2.6.6/gems/railties-6.0.3.4/lib/rails/application.rb:100:in `instance'
     # /Users/bryanp/.gem/ruby/2.6.6/gems/railties-6.0.3.4/lib/rails/railtie.rb:183:in `respond_to_missing?'
     # /Users/bryanp/Code/Forks/rspec-mocks/lib/rspec/mocks/method_double.rb:67:in `respond_to?'
     # /Users/bryanp/Code/Forks/rspec-mocks/lib/rspec/mocks/method_double.rb:67:in `block in define_proxy_method'

Couple of observations:

ruby2_keywords calls respond_to?.
The context we call respond_to? on is a RSpecRails::Application class, which is a Railtie.
Railtie::respond_to_missing? tries to create an instance, which we can't do because the context is a Singleton.

I don't yet understand how RSpecRails::Application is created, but an idea is to redefine ::instance to behave like Singleton::instance rather than Railtie::instance. There may be a more obvious solution that I'm missing here.

pirj · 2021-01-06T15:00:01Z

@bryanp Interesting!
We call if respond_to? ourselves there, it's not ruby2_keywords itself.
Do you think we can add the check for respond_to? somewhere up the chain? Rubies 2.5 & 2.6 don't have it.

Or probably use Module.respond_to?(:ruby2_keywords, true) and rely on the assumption that if Module responds to it, other classes do as well?

bryanp · 2021-01-06T15:29:05Z

@pirj lol, you're right we do call respond_to?. I should have finished my coffee before diving into this today :)

I like the Module.respond_to?(:ruby2_keywords, true) idea, but maybe we should use TOPLEVEL_BINDING.receiver instead? That's the approach used in the ruby2_keywords gem here.

pirj · 2021-01-06T16:01:43Z

I have some serious problem understanding what is going on in that gem while looking at this line 😄

I don't mind either option. To me, Module.respond_to? is more compact.

@mame, can you please advise what is the best option for our case?
We are trying to call ruby2_keywords, but only if the object responds to it. Railtie#respond_to_missing? is calling instance and fails.

In order to avoid that respond_to? call on the current object, is it ok to call it on Module, Object, or TOPLEVEL_BINDING.receiver instead?

JonRowe · 2021-01-06T16:23:33Z

We do use the "unbind the real method and use that" to work around monkey patching in various places, maybe we need that trick in rspec-support for defining keyword support?

mame · 2021-01-07T01:42:35Z

Hello!

I think Module.private_method_defined?(:ruby2_keywords) is the best option. If directly calling ruby2_keywords on Railtie is troublesome, Module.instance_method(:ruby2_keywords).bind(self).call(method_name) may work, as @JonRowe said.

Sorry, the check in ruby2_keywords gem looks wrong. Module.respond_to?(:ruby2_keywords) does NOT check if Module#ruby2_keywords is available. Fortunately, it works well because the check returns true in Ruby 2.7 or later due to the toplevel ruby2_keywords method. And thank you for letting me know about the syntactic error 🤣 I will talk with @nobu and will fix.

bryanp · 2021-01-07T02:22:29Z

Thank you @mame for you input. I've made the suggested change.

This resolved the issues in Ruby 2.* but 1.8.7 and 1.9.2 are still failing in Travis. I haven't looked into why yet.

mame · 2021-01-07T05:20:43Z

Looks like this issue is unrelated to this PR, this time for sure?

https://travis-ci.org/github/rspec/rspec-mocks/jobs/753288703

$ gem install bundler -v '< 2'
ERROR:  Could not find a valid gem 'bundler' (< 2), here is why:
          Unable to download data from https://rubygems.org/ - hostname was not match with the server certificate (https://api.rubygems.org/specs.4.8.gz)
The command "gem install bundler -v '< 2'" failed and exited with 2 during .

Using `respond_to?` causes issues in rspec-rails because of how the `Railtie` class implements `respond_to_missing?`.

pirj · 2021-01-07T08:34:25Z

There were some changes to .travis.yml, I've rebased this branch on main and force-pushed it.
However, chances of fixing the issue are quite low, since judging by the list of jobs (1.8.7, 1.9.2, ree, jruby-1.7) that the failing build was using the .travis.yml from main anyway, not the one from this branch that also lists 1.9.3 and 2.0.0. I vaguely remember that was the case, can't remember why this is the case.

JonRowe · 2021-01-07T08:41:28Z

Travis is broken and I'm replacing the builds as I can.

pirj · 2021-01-07T08:43:01Z

@mame Thanks a lot for your help!

@bryanp Thanks a lot for dealing with this issue!

Resolve an issue using method doubles with keyword arguments on Ruby 3

…rgs-for-ruby3 Resolve an issue using method doubles with keyword arguments on Ruby 3 --- This commit was imported from rspec/rspec-mocks@117b33d.

This commit was imported from rspec/rspec-mocks@ea4d8ea.

…rgs-for-ruby3 Resolve an issue using method doubles with keyword arguments on Ruby 3 --- This commit was imported from rspec/rspec-mocks@a0f33ee.

This commit was imported from rspec/rspec-mocks@b843db7.

…rgs-for-ruby3 Resolve an issue using method doubles with keyword arguments on Ruby 3 --- This commit was imported from rspec/rspec-mocks@0306f46.

eregon · 2022-03-24T11:49:34Z

I've run into this weird issue with respond_to?(:ruby2_keywords, true) as well in #1464 and wondered for a while what the heck was going on. bundle exec rspec -b spec in rspec-rails helps to see the first error, which helps. The following errors are all completely undecipherable.

For some reason it only happens on 2.5/2.6, I guess because 1) 2.7+ have the methods, so don't fall into the trap, and 2) only those Ruby versions test the Rails version with this weird behavior? (which still exists on Rails main)
What's pretty crazy here is that the Railtie class defines respond_to_missing? on the Railtie class, not for instances, and try to create an instance in respond_to_missing?, which is definitely not expected for a respond_to_missing? implementation.

eregon · 2022-03-24T11:52:59Z

Sorry, the check in ruby2_keywords gem looks wrong. Module.respond_to?(:ruby2_keywords) does NOT check if Module#ruby2_keywords is available.

FWIW, that was fine and no bug, it finds Module#ruby2_keywords, since it's called on the Module class, which is a module (as all classes are).

eregon · 2022-03-24T12:08:21Z

I've filed an issue to Rails about this, and found at least one spot where they unintentionally create a Railtie instance by a respond_to? check: rails/rails#44761

bryanp mentioned this pull request Dec 29, 2020

Handle keyword arguments separately for and_call_original in supported rubies #1324

Closed

JonRowe reviewed Dec 29, 2020

View reviewed changes

mame mentioned this pull request Jan 6, 2021

Make with support Ruby 3 keywords #1394

Merged

Bryan Powell added 5 commits January 7, 2021 11:24

Mark proxy methods with ruby2_keywords

82dd54b

Only use ruby2_keywords if defined

c120a91

Only test keyword arguments on rubies that support them

be1b513

Update a spec to work in rubies that don't support required kwargs

c5f4d54

Avoid respond_to? when checking existence of the ruby2_keywords method

2ca170b

Using `respond_to?` causes issues in rspec-rails because of how the `Railtie` class implements `respond_to_missing?`.

pirj force-pushed the fix-kwargs-for-ruby3 branch from 85504da to 2ca170b Compare January 7, 2021 08:25

JonRowe merged commit 117b33d into rspec:main Jan 7, 2021

JonRowe added a commit that referenced this pull request Jan 7, 2021

Change log for #1385

ea4d8ea

JonRowe mentioned this pull request Jan 7, 2021

Keyword argument warning on Ruby 2.7 for #and_call_original #1306

Closed

bryanp deleted the fix-kwargs-for-ruby3 branch January 7, 2021 13:38

pirj mentioned this pull request Jan 13, 2021

[wip] #1390

Closed

JonRowe added a commit that referenced this pull request Jan 15, 2021

Merge pull request #1385 from bryanp/fix-kwargs-for-ruby3

a0f33ee

Resolve an issue using method doubles with keyword arguments on Ruby 3

JonRowe added a commit that referenced this pull request Jan 15, 2021

Change log for #1385

b843db7

JonRowe added a commit that referenced this pull request Jan 15, 2021

Merge pull request #1385 from bryanp/fix-kwargs-for-ruby3

0306f46

Resolve an issue using method doubles with keyword arguments on Ruby 3

JonRowe mentioned this pull request Jan 25, 2021

Add tests and fix for ruby 3 keyword arguments issues #1407

Merged

kachick mentioned this pull request May 9, 2021

Ruby 3.0 から keyword arguments がどう変わって、具体的にどう言うケースでどう対処すればいいか、なんとなくなわかっているようでぼんやりしている kachick/times_kachick#66

Closed

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[mocks] Change log for rspec/rspec-mocks#1385

ecffaf2

This commit was imported from rspec/rspec-mocks@ea4d8ea.

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[mocks] Change log for rspec/rspec-mocks#1385

8159a74

This commit was imported from rspec/rspec-mocks@b843db7.

eregon mentioned this pull request Mar 24, 2022

Add missing ruby2_keywords calls in rspec-mocks #1464

Merged

eregon mentioned this pull request Mar 24, 2022

Railtie.respond_to_missing? creates an instance unexpectedly rails/rails#44761

Closed

Uh oh!

Conversation

bryanp commented Dec 29, 2020

Uh oh!

bryanp commented Dec 29, 2020

Uh oh!

bryanp commented Dec 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pirj commented Dec 29, 2020

Uh oh!

bryanp commented Dec 30, 2020

Uh oh!

JonRowe commented Jan 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanp commented Jan 2, 2021

Uh oh!

JonRowe commented Jan 4, 2021

Uh oh!

bryanp commented Jan 5, 2021

Uh oh!

JonRowe commented Jan 6, 2021

Uh oh!

bryanp commented Jan 6, 2021

Uh oh!

pirj commented Jan 6, 2021

Uh oh!

bryanp commented Jan 6, 2021

Uh oh!

pirj commented Jan 6, 2021

Uh oh!

JonRowe commented Jan 6, 2021

Uh oh!

mame commented Jan 7, 2021

Uh oh!

bryanp commented Jan 7, 2021

Uh oh!

mame commented Jan 7, 2021

Uh oh!

pirj commented Jan 7, 2021

Uh oh!

JonRowe commented Jan 7, 2021

Uh oh!

pirj commented Jan 7, 2021

Uh oh!

eregon commented Mar 24, 2022

Uh oh!

eregon commented Mar 24, 2022

Uh oh!

eregon commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JonRowe commented Jan 1, 2021 •

edited

Loading