Closed philnash closed 1 year ago
It sounds like Ruby 3.2 has changed something that has broken our detection between keywords and hashes again, wether that fix needs to go into Ruby or us depends on if it was a deliberate change on their side or an accidental one.
It's likely due to this change, search for ruby2_keywords
in https://github.com/ruby/ruby/blob/master/NEWS.md, and it is deliberate, so probably it needs some extra ruby2_keywords
in RSpec.
diff --git a/lib/rspec/mocks/verifying_proxy.rb b/lib/rspec/mocks/verifying_proxy.rb
index b39871c8..1d8d207d 100644
--- a/lib/rspec/mocks/verifying_proxy.rb
+++ b/lib/rspec/mocks/verifying_proxy.rb
@@ -160,6 +160,7 @@ module RSpec
validate_arguments!(args)
super
end
+ ruby2_keywords :proxy_method_invoked if respond_to?(:ruby2_keywords, true)
def validate_arguments!(actual_args)
@method_reference.with_signature do |signature|
fixes
class Xxx
def self.x(extra:)
end
end
RSpec.describe 'ruby-3.2.0' do
specify do
allow(Xxx).to receive(:x).with(extra: {})
Xxx.x(extra: {})
end
specify do
expect(Xxx).to receive(:x).with(extra: {})
Xxx.x(extra: {})
end
end
for me (and CI for some small internal project)
With the change above testsuite passes except reentrant mutext inside the Fiber spec, see https://github.com/ruby/ruby/pull/6680#issuecomment-1318481643
Would you like to send a PR, @ojab ?
@pirj sure, I just want to get some clarification on https://github.com/ruby/ruby/pull/6680#issuecomment-1318481643 first. Or I could make a PR with the fix above and adding ruby-3.2 to CI + skipping reentrant mutex specs for rubies >=3.2 for the time being.
As I replied there, I think the best is to adjust that spec: https://github.com/ruby/ruby/pull/6680#issuecomment-1318514767
We have a ruby-head
job, will that suffice, @ojab ?
@pirj yep, ruby-head is fine too. https://github.com/rspec/rspec-support/pull/553 https://github.com/rspec/rspec-mocks/pull/1502 (draft until rspec-support PR is merged because CI would be broken anyway)
I found another place we need to instrument a method via ruby2_keywords
. https://github.com/rspec/rspec-mocks/pull/1508
I almost wonder if we should just audit all methods that have a *args
in the signature without a **kwargs
and blindly instrument with ruby2_keywords
, otherwise there might be a longer tail of bug reports trickling in.
I almost wonder if we should just audit all methods that have a
*args
in the signature without a**kwargs
and blindly instrument withruby2_keywords
, otherwise there might be a longer tail of bug reports trickling in.
I did that for some gems, yes it's a good idea to audit all *args
(or *<name>
of course) methods which use those arguments to call another method and either:
ruby2_keywords
is necessaryIndeed it's not semantically harmful to add ruby2_keywords
to more places than needed, although it can have a little bit of a performance impact and it may confuse people reading the code if no keywords should ever come there.
At least I wouldn't do it if there is no delegation, i.e., if *args
is not used for calling some other method later.
Subject of the issue
I ran a bunch of specs on my project through GitHub actions and a bunch failed recently against Ruby head. I'm not sure whether it's the fault of Ruby or RSpec or my own misunderstanding, so I thought I would report it here first.
The issue came down to expectations for instance doubles for objects that take keyword arguments and how to test them. Please see the steps to reproduce below for details.
Your environment
Steps to reproduce
Expected behavior
I expected the first test to pass, since the expectation is that the method is called with two keyword arguments and then the method is called with two keyword arguments.
I'd expect the second test to fail because the expectation is that the method is called with a hash, but the method is called with keyword arguments.
Actual behavior
The first test fails and the second passes.
However, just to throw some extra information in:
Is this something that should be raised with Ruby or is it something that RSpec needs to handle for Ruby 3.2?