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

Make with support Ruby 3 keywords #1394

Merged
merged 9 commits into from
Feb 18, 2021
Merged

Make with support Ruby 3 keywords #1394

merged 9 commits into from
Feb 18, 2021

Conversation

mame
Copy link
Contributor

@mame mame commented Jan 6, 2021

As you know, keyword arguments have been separated from positional ones in Ruby 3.
How about making with to match the semantics?
It would be good to allow rspec to check if positional or keyword arguments are passed appropriately.

For example, if a mock object has `with(a: 'a', b: 'b'), it should accept keywords, not a positional Hash.

class Foo
end

foo = Foo.new
allow(foo).to receive(:bar).with(a: 'a', b: 'b')
args = { a: 'a', b: 'b' }

foo.bar(args)   # fail
foo.bar(**args) # pass

Also, if a mock object has `with({ a: 'a', b: 'b' }), it should accept a positional Hash, not keywords.

foo = Foo.new
allow(foo).to receive(:bar).with({ a: 'a', b: 'b' })
args = { a: 'a', b: 'b' }

foo.bar(args)   # pass
foo.bar(**args) # fail

This PR is just a proof of concept and the change partially includes #1385. What do you think?

@jeremyevans
Copy link

Also, if a mock object has `with({ a: 'a', b: 'b' }), it should accept a positional Hash, not keywords.

I think in this case, it should accept a positional hash or keywords. This is because Ruby 3 (and Ruby 2) will convert the keywords to a positional hash if the method does not accept keywords.

@eregon
Copy link
Contributor

eregon commented Jan 6, 2021

@jeremyevans Good point, it's a scenario like:

def foo(options)
end
# or
def foo(options = {})
end

# Both OK in all Ruby versions
foo({a: 'a'})
foo(a: 'a')

with() is a bit mind-bending because we have to guess what method parameters would correspond to it.

@mame
Copy link
Contributor Author

mame commented Jan 7, 2021

@jeremyevans @eregon Thanks. I've updated the PR to allow the conversion from keywords to a positional Hash.

I first thought that a with expectation checks actual arguments in the caller side (i.e., before the conversion) because it works even if there is no actual method definition. But, now I notice that the rspec DSL says in English allow obj to recevie method_name with arguments, which looks like this is a callee-side check.

@mame
Copy link
Contributor Author

mame commented Jan 7, 2021

I'll try fixing the CI error after #1385 is merged.

BTW, an assertion error message is less informative if this PR is accepted:

class Foo; end

RSpec.describe do
  it do
    foo = Foo.new
    allow(foo).to receive(:bar).with(a: 'a', b: 'b')
    foo.bar({ a: 'a', b: 'b' })
  end
end
       #<Foo:0x0000562a3c2c3ac8> received :bar with unexpected arguments
         expected: ({:a=>"a", :b=>"b"})
              got: ({:a=>"a", :b=>"b"})

The expected and got lines are completely the same, so it is a bit confusing. It would be good to change it to, for example,

       #<Foo:0x0000562a3c2c3ac8> received :bar with unexpected arguments
         expected: (a: "a", b: "b")
              got: ({:a=>"a", :b=>"b"})

or what style you like.

If you are okay, I have already created a patch of ruby-support to show the above message. If this PR is accepted, I'll send another PR to ruby-support. (I guess I need to update other rspec-* repositories because it will break many rspec's specs that checks an error message.)

@mame mame force-pushed the with-for-ruby3 branch 2 times, most recently from 31cbd3c to c1b057e Compare January 8, 2021 05:43
mame added a commit to mame/rspec-mocks that referenced this pull request Jan 8, 2021
@mame mame force-pushed the with-for-ruby3 branch 2 times, most recently from 2a596c1 to f9d2abb Compare January 8, 2021 08:30
@mame
Copy link
Contributor Author

mame commented Jan 8, 2021

Sorry for too many retries, I think I managed to make it pass on Ruby 1.9.3 😅

@JonRowe @pirj Could you check this PR out? Thanks!

@JonRowe
Copy link
Member

JonRowe commented Jan 8, 2021

@mame Yes it's on the list to review

@benoittgt
Copy link
Member

The expected and got lines are completely the same, so it is a bit confusing. It would be good to change it to, for example,

I am wondering it this is something we should add to next major RSpec. Maybe it is a little bit abrupt and should deprecated first?

Thanks a lot @mame for your patch. 🙌🏻

bbatsov added a commit to toptal/chewy that referenced this pull request Jan 15, 2021
Seems we're blocked by rspec/rspec-mocks#1394

This reverts commit da4ff3e.
dalthon pushed a commit to toptal/chewy that referenced this pull request Feb 4, 2021
Seems we're blocked by rspec/rspec-mocks#1394

This reverts commit da4ff3e.
@JonRowe
Copy link
Member

JonRowe commented Feb 4, 2021

Can you rebase? Our CI is now fully on Github Actions so can run all the legacy rubies. If this goes green I'm inclined to merge it.

This change makes `receive(:foo).with(a: "A")` allow only `foo(a: "A")`
and deny `foo({ a: "A" })` in Ruby 3.
Also, `receive(:foo).with({ a: "A" })` allows only `foo({ a: "A" })` and
denies `foo(a: "A")`.
This change allows a Hash argument expectation (`with({ a: 'a' })`) to
accept keyword arguments (`foo(a: 'a')`) both in Ruby 2 and in Ruby 3.

```
allow(foo).to receive(:bar).with({ a: 'a' })

foo.bar(a: 'a')
foo.bar({ a: 'a' })
```

This is because Ruby 3 also allows the automatic conversion from
keywords to a positional Hash. (But the conversion from a positional
last Hash to keywords is not allowed.)

```
def foo(opts = {})
end

foo(a: 'a')
foo({ a: 'a' })
```
to avoid a warning: "assigned but unused variable - hash"
pirj pushed a commit that referenced this pull request Mar 22, 2022
Ref: vcr/vcr#925
Ref: #1394

I spent quite a lot of time figuring this error:

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true})
              got: ({:ignore_cassettes=>true})
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```

I quickly suspected it was a keyword argument issue, but it's far from
obvious to everyone, and even when you are familair with the issue
it doesn't tell you what was expected and what was received.

I doubt the way I implemented this is ok, but I think it's worth
opening the discussion

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true}) (keyword arguments)
              got: ({:ignore_cassettes=>true}) (options hash)
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```
mishina2228 added a commit to mishina2228/uniform_notifier that referenced this pull request Mar 22, 2022
Since rspec-mocks v3.10.3, `with` now distinguishes between keyword args and hash in Ruby 3.
See rspec/rspec-mocks#1394 for details.
mishina2228 added a commit to mishina2228/uniform_notifier that referenced this pull request Mar 22, 2022
Since rspec-mocks v3.10.3, `with` now distinguishes between keyword args and hash in Ruby 3.
See rspec/rspec-mocks#1394 for details.
voxik added a commit to voxik/multi_json that referenced this pull request Sep 6, 2022
This is due to change [1], which cuases issues such as:

~~~
  1) MultiJson default options sets both load and dump options
     Failure/Error: self.load_options = self.dump_options = value

       MultiJson received :dump_options= with unexpected arguments
         expected: ({:foo=>"bar"})
              got: ({:foo=>"bar"})
     # ./lib/multi_json.rb:15:in `default_options='
     # ./spec/multi_json_spec.rb:171:in `block (4 levels) in <top (required)>'
     # /builddir/build/BUILD/spec/spec_helper.rb:12:in `silence_warnings'
     # ./spec/multi_json_spec.rb:171:in `block (3 levels) in <top (required)>'
~~~

Fixes intridea#203

[1]: rspec/rspec-mocks#1394
voxik added a commit to voxik/multi_json that referenced this pull request Sep 6, 2022
This is due to change [[1]], which cuases issues such as:

~~~
  1) MultiJson default options sets both load and dump options
     Failure/Error: self.load_options = self.dump_options = value

       MultiJson received :dump_options= with unexpected arguments
         expected: ({:foo=>"bar"})
              got: ({:foo=>"bar"})
     # ./lib/multi_json.rb:15:in `default_options='
     # ./spec/multi_json_spec.rb:171:in `block (4 levels) in <top (required)>'
     # /builddir/build/BUILD/spec/spec_helper.rb:12:in `silence_warnings'
     # ./spec/multi_json_spec.rb:171:in `block (3 levels) in <top (required)>'
~~~

Fixes intridea#203

[1]: rspec/rspec-mocks#1394
voxik added a commit to voxik/multi_json that referenced this pull request Sep 6, 2022
This is due to change in RSpec [[1]], which cuases issues such as:

~~~
  1) MultiJson default options sets both load and dump options
     Failure/Error: self.load_options = self.dump_options = value

       MultiJson received :dump_options= with unexpected arguments
         expected: ({:foo=>"bar"})
              got: ({:foo=>"bar"})
     # ./lib/multi_json.rb:15:in `default_options='
     # ./spec/multi_json_spec.rb:171:in `block (4 levels) in <top (required)>'
     # /builddir/build/BUILD/spec/spec_helper.rb:12:in `silence_warnings'
     # ./spec/multi_json_spec.rb:171:in `block (3 levels) in <top (required)>'
~~~

Fixes intridea#203

[1]: rspec/rspec-mocks#1394
voxik added a commit to voxik/multi_json that referenced this pull request Sep 6, 2022
This is due to change in RSpec [[1]], which cuases issues such as:

~~~
  1) MultiJson default options sets both load and dump options
     Failure/Error: self.load_options = self.dump_options = value

       MultiJson received :dump_options= with unexpected arguments
         expected: ({:foo=>"bar"})
              got: ({:foo=>"bar"})
     # ./lib/multi_json.rb:15:in `default_options='
     # ./spec/multi_json_spec.rb:171:in `block (4 levels) in <top (required)>'
     # /builddir/build/BUILD/spec/spec_helper.rb:12:in `silence_warnings'
     # ./spec/multi_json_spec.rb:171:in `block (3 levels) in <top (required)>'
~~~

Fixes intridea#203

[1]: rspec/rspec-mocks#1394
voxik added a commit to voxik/multi_json that referenced this pull request Sep 6, 2022
This is due to change in RSpec [[1]], which cuases issues such as:

~~~
  1) MultiJson default options sets both load and dump options
     Failure/Error: self.load_options = self.dump_options = value

       MultiJson received :dump_options= with unexpected arguments
         expected: ({:foo=>"bar"})
              got: ({:foo=>"bar"})
     # ./lib/multi_json.rb:15:in `default_options='
     # ./spec/multi_json_spec.rb:171:in `block (4 levels) in <top (required)>'
     # /builddir/build/BUILD/spec/spec_helper.rb:12:in `silence_warnings'
     # ./spec/multi_json_spec.rb:171:in `block (3 levels) in <top (required)>'
~~~

Fixes intridea#203

[1]: rspec/rspec-mocks#1394
pirj pushed a commit that referenced this pull request Nov 1, 2022
pirj pushed a commit that referenced this pull request Nov 1, 2022
Ref: vcr/vcr#925
Ref: #1394

I spent quite a lot of time figuring this error:

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true})
              got: ({:ignore_cassettes=>true})
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```

I quickly suspected it was a keyword argument issue, but it's far from
obvious to everyone, and even when you are familair with the issue
it doesn't tell you what was expected and what was received.

I doubt the way I implemented this is ok, but I think it's worth
opening the discussion

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true}) (keyword arguments)
              got: ({:ignore_cassettes=>true}) (options hash)
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```
pirj pushed a commit that referenced this pull request Nov 1, 2022
pirj pushed a commit that referenced this pull request Nov 1, 2022
Ref: vcr/vcr#925
Ref: #1394

I spent quite a lot of time figuring this error:

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true})
              got: ({:ignore_cassettes=>true})
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```

I quickly suspected it was a keyword argument issue, but it's far from
obvious to everyone, and even when you are familair with the issue
it doesn't tell you what was expected and what was received.

I doubt the way I implemented this is ok, but I think it's worth
opening the discussion

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true}) (keyword arguments)
              got: ({:ignore_cassettes=>true}) (options hash)
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```
pirj pushed a commit that referenced this pull request Nov 1, 2022
pirj pushed a commit that referenced this pull request Nov 1, 2022
Ref: vcr/vcr#925
Ref: #1394

I spent quite a lot of time figuring this error:

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true})
              got: ({:ignore_cassettes=>true})
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```

I quickly suspected it was a keyword argument issue, but it's far from
obvious to everyone, and even when you are familair with the issue
it doesn't tell you what was expected and what was received.

I doubt the way I implemented this is ok, but I think it's worth
opening the discussion

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true}) (keyword arguments)
              got: ({:ignore_cassettes=>true}) (options hash)
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```
JonRowe added a commit that referenced this pull request Nov 10, 2022
JonRowe pushed a commit that referenced this pull request Nov 10, 2022
Ref: vcr/vcr#925
Ref: #1394

I spent quite a lot of time figuring this error:

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true})
              got: ({:ignore_cassettes=>true})
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```

I quickly suspected it was a keyword argument issue, but it's far from
obvious to everyone, and even when you are familair with the issue
it doesn't tell you what was expected and what was received.

I doubt the way I implemented this is ok, but I think it's worth
opening the discussion

```
  2) VCR.turned_on passes options through to .turn_off!
     Failure/Error: turn_off!(options)

       VCR received :turn_off! with unexpected arguments
         expected: ({:ignore_cassettes=>true}) (keyword arguments)
              got: ({:ignore_cassettes=>true}) (options hash)
     # ./lib/vcr.rb:317:in `turned_on'
     # ./spec/lib/vcr_spec.rb:367:in `block (3 levels) in <top (required)>'
```
return false if expected_args.size != actual_args.size

if RUBY_VERSION >= "3"
# if both arguments end with Hashes, and if one is a keyword hash and the other is not, they don't match
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused quite a bit of confusion 😅 , will be fixed in #1514 (comment) hopefully

cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023
Seems we're blocked by rspec/rspec-mocks#1394

This reverts commit da4ff3e.
y-yagi added a commit to y-yagi/draper that referenced this pull request May 24, 2024
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 this pull request may close these issues.

8 participants