Display keyword hashes in in expectation error messages

[casperisfine]

Ref: https://github.com/vcr/vcr/pull/925 Ref: https://github.com/rspec/rspec-mocks/pull/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.

[pirj]

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

[pirj]

Another example probably worth adding/checking is reported in https://github.com/rspec/rspec-mocks/issues/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]

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 https://github.com/rspec/rspec-metagem/issues/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 https://github.com/rspec/rspec-mocks/pull/1464 a release will be needed, I'll explain there.

[pirj]

@casperisfine Thank you for the contribution!