Display keyword hashes in in expectation error messages

[casperisfine]

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]

Do you think it's achievable with Ruby 2.x?

[eregon]

In 2.7 yes, but before I believe it's impossible to know in the callee if kwargs or a positional Hash was passed.

[eregon]

Note: Hash.ruby2_keywords_hash? will only recognize hashes from a ruby2_keywords method/block, or a hash explcitly marked with Hash.ruby2_keywords_hash, but e.g., not kw in def m(*args, **kw); kw; end.
I guess ruby2_keywords is used somewhere before and so it just works here (is that in RSpec code?), but it's a bit subtle.

If rspec-mocks would store positional and kwargs arguments in separate fields (only on Ruby 3+, before can't separate them reliably) then I think it would be nicer and more reliable to check.

[casperisfine]

@eregon what you are saying is right, but I'm not trying to display whether a hash was keyword args 100% of the times, I only want to display it when two hashes are identical except for the ruby2_keywords_hash?.

#1394 add this code:

        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
          if Hash === expected_args.last && Hash === actual_args.last
            if !Hash.ruby2_keywords_hash?(actual_args.last) && Hash.ruby2_keywords_hash?(expected_args.last)
              return false
            end
          end
        end

And it's this case I want to display. So yeah, my PR only apply to 3+ like the previous PR.

[eregon]

Hash.ruby2_keywords_hash? still can't tell if a Hash was used as keyword arguments, except for the case we are sure the keyword arguments were passed to a ruby2_keywords method, and that Hash then arrives here, not through another call (which would remove the flag IIRC).
Is that guaranteed here that expected_hash and actual_hash come from ruby2_keywords methods?

In other words, I think the current patch might not be able to show the helpful kwargs/non-kwargs suffix for things like:

def foo(*args, **kwargs)
  ...
end

foo({a: 1})
vs
foo(a: 1)

but given this is rspec-mocks maybe it could because it might always control the definition of foo and always use ruby2_keywords there?

[casperisfine]

Is that guaranteed here that expected_hash and actual_hash come from ruby2_keywords methods?

if expected_hash == actual_hash then AFAIK yes that's the only way. Otherwise we wouldn't be printing an error.

Of course there might be some weird edge cases if you monkey patch Hash#== but...

[eregon]

I see, there is similar logic in https://github.com/mame/rspec-mocks/blob/865ea3a795e0e56309b823e76e3f65eff5cfa448/lib/rspec/mocks/argument_list_matcher.rb#L63-L70
I think a # See #args_match? comment would be helpful.

Also the check doesn't necessarily seem correct, it's only in one direction, because passing kwargs to a method not taking kwargs is fine: https://github.com/mame/rspec-mocks/blob/865ea3a795e0e56309b823e76e3f65eff5cfa448/lib/rspec/mocks/argument_list_matcher.rb#L66
(the comment is unfortunately outdated/not matching the code)

I'd suggest to extract the check to a separate method, then it's clear it's the same condition.

[pirj]

In addition to what Benoit said, there are certain problems with comparing method signatures, might be related.

[casperisfine]

I added the RUBY_VERSION >= "3" which should solve the CI issues.

[pirj]

We typically add such things to rspec-support and use a self-explanatory method on RubyFeatures. Do you think it's possible to come up with a name that would explain why a special case is needed?

[casperisfine]

Hum, RubyFeatures.has_ruby2_compatibility_keywords?

[pirj]

Or RubyFeatures.distincts_kw_args_from_positional_hash? ?

[casperisfine]

Sure. Should I make a PR to rspec-support?

[pirj]

Yes, please 👍

[pirj]

Cosmetic: with RUBY_VERSION >= "3", I believe RSpec::Support::RubyFeatures.kw_args_supported? will always evaluate to true, so it's redundant.

[casperisfine]

Well, ruby2_keywords might go away one day (not any time soon though).

[pirj]

kw_args_supported? has no awareness of that.
Currently, this code expands to:

if RUBY_VERSION >= "3" && RUBY_VERSION >= '2.0.0'

[pirj]

Nice! Thank you for the contribution.
Does it make sense to add an example the is the other way around (receive(:foo).with(expected_hash) and d.foo(**expected_hash))?

[pirj]

Another example probably worth adding/checking is reported in #1505, https://github.com/ruby-grape/grape/runs/5347064188?check_suite_focus=true:

expect(subject).to receive(:namespace_inheritable).with(:version_options, using: :path)
...
options = options.reverse_merge(using: :path)
namespace_inheritable(:version_options, options)

[casperisfine]

Does it make sense to add an example the is the other way around

I don't think it's possible, because passing keyword arguments to a method that expects an option hash automatically fallback to an option hash. So unless I'm missing something I don't think it can happen.

[casperisfine]

Another example probably worth adding/checking is reported in

I added a second test case, the first one used splats **, the additional one use literal keyword arguments.

[pirj]

It can be the other way around, expected: (options hash) + got: (keyword arguments), do we need a test case for that?

[casperisfine]

No as I said above, I don't think it's possible. If you know a way it could happen, let me know.

[pirj]

I don't mind merging as is.
Just wondering if the following will be sufficient:

 if Hash === expected_hash && Hash === actual_hash &&
-  (Hash.ruby2_keywords_hash?(expected_hash) != Hash.ruby2_keywords_hash?(actual_hash))
+  Hash.ruby2_keywords_hash?(expected_hash) && !Hash.ruby2_keywords_hash?(actual_hash)
-  actual_args += Hash.ruby2_keywords_hash?(actual_hash) ? " (keyword arguments)" : " (options hash)"
+  actual_args += " (options hash)"
-  expected_args += Hash.ruby2_keywords_hash?(expected_hash) ? " (keyword arguments)" : " (options hash)"
+  expected_args += " (keyword arguments)"
 end

because it's enough to make the specs pass.

[casperisfine]

Just wondering if the following will be sufficient:

Assuming I'm right on the fact that it's not possible to pass keyword args when option hashes are expected then yes.

That being said I may be wrong, in which case making this change would mean that we don't handle it.

So it's up to you.

[eregon]

I ran into this as well, anything left before merging it?

[pirj]

@eregon I guess you're working on the bleeding edge and don't need a release to benefit from this improvement?

[eregon]

For rspec/rspec-metagem#68 indeed I don't need a release.
However, I think several people already ran into this (e.g., see links to this PR), so it would be very beneficial to get this in a release (avoids a significant amount of confusion).

BTW, for #1464 a release will be needed, I'll explain there.

[pirj]

@casperisfine Thank you for the contribution!