rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 358 forks source link

Make `with` support Ruby 3 keywords #1394

Closed mame closed 3 years ago

mame commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@mame Yes it's on the list to review

benoittgt commented 3 years ago

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. 🙌🏻

JonRowe commented 3 years ago

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.

mame commented 3 years ago

@JonRowe Thank you for your review! I've rebased.

mame commented 3 years ago

The CI failed on Ruby 2.2, but I believe that this is not a fault of this PR.

https://github.com/rspec/rspec-mocks/pull/1394/checks?check_run_id=1836305443

benoittgt commented 3 years ago

The CI failed on Ruby 2.2, but I believe that this is not a fault of this PR.

it's all green now. :)

ys commented 3 years ago

Cannot wait to see this one landing! Thanks all for the work on this!

JonRowe commented 3 years ago

Thank you @mame!

casperisfine commented 2 years ago

I submitted https://github.com/rspec/rspec-mocks/pull/1461 as some kind of proof of concept on how to better display the difference between keyword args and regular hashes when they are identical.