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

Add support for rspec-mocks argument macthers #128

Merged

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented May 3, 2021

This PR adds support for diffing rspec-mocks argument matchers. I often use hash_including in rspec-mocks instead of a_hash_including in rspec-expectations for partial hash comparisons. SuperDiff shows a great diff for a_hash_including, but hash_including doesn't.

In rspec-mocks, there is an argument matcher similar to the matcher provided by rspec-expectations. This PR is primarily aimed at them. The following support has been added:

  • hash_including (similar to a_hash_including)
  • array_including (similar to a_collection_including)
  • kind_of (similar to a_kind_of)
  • instance_of (similar to an_instance_of)

Other than that, rspec-mocks provides useful matchers (e.g. hash_excluding, duck_type`, etc.), but they are out of scope here.

Before

Failures:

  1) is expected to match { a: #<RSpec::Mocks::ArgumentMatchers::HashIncludingMatcher:0x00007f92fca5a4d8 @expected={:b=>{:c=>{:d=>2}}}> }
     Failure/Error: expect({ a: { b: { c: { d: 1 }}} }).to match({ a: hash_including(b: { c: { d: 2 } }) })

       Expected { a: { b: { c: { d: 1 } } } }
       to match { a: #<RSpec::Mocks::ArgumentMatchers::HashIncludingMatcher:0x00007f92fca5a4d8 @expected={:b=>{:c=>{:d=>2}}}> }

       Diff:

       ┌ (Key) ──────────────────────────┐
       │ ‹-› in expected, not in actual  │
       │ ‹+› in actual, not in expected  │
       │ ‹ › in both expected and actual │
       └─────────────────────────────────┘
         {
       -   a: #<RSpec::Mocks::ArgumentMatchers::HashIncludingMatcher:0xaf0 {
       -     @expected={
       -       b: {
       -         c: {
       -           d: 2
       -         }
       -       }
       -     }
       -   }>
       +   a: {
       +     b: {
       +       c: {
       +         d: 1
       +       }
       +     }
       +   }
         }

After

Failures:

  1) is expected to match { a: #<a hash including (b: { c: { d: 2 } })> }
     Failure/Error: expect({ a: { b: { c: { d: 1 }}} }).to match({ a: hash_including(b: { c: { d: 2 } }) })

       Expected { a: { b: { c: { d: 1 } } } } to match { a: #<a hash including (b: { c: { d: 2 } })> }.

       Diff:

       ┌ (Key) ──────────────────────────┐
       │ ‹-› in expected, not in actual  │
       │ ‹+› in actual, not in expected  │
       │ ‹ › in both expected and actual │
       └─────────────────────────────────┘

         {
           a: {
             b: {
               c: {
       -         d: 2
       +         d: 1
               }
             }
           }
         }

Finally, thank you for the great gem. In our project, we had a problem that the diff of the request spec comparing huge JSON was very hard to read, but this gem solved it brilliantly.

array = if SuperDiff::RSpec.a_collection_including_something?(value)
value.expecteds
else
value.instance_variable_get(:@expected)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand this is a bad practice, but unfortunately, the rspec-mocks matcher didn't have a public interface to access the values.
https://github.com/rspec/rspec-mocks/blob/v3.10.2/lib/rspec/mocks/argument_matchers.rb#L231-L259

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense! (I also do this in other places)

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I had a few little comments but overall this is a nice PR, thank you for doing this!

@@ -3,7 +3,8 @@ module RSpec
module Differs
class CollectionIncluding < SuperDiff::Differs::Array
def self.applies_to?(expected, actual)
SuperDiff::RSpec.a_collection_including_something?(expected) &&
(SuperDiff::RSpec.a_collection_including_something?(expected) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: What do you think about formatting it like this so the grouping is easier to parse?

(
  SuperDiff::RSpec.a_collection_including_something?(expected) ||
  SuperDiff::RSpec.array_including_something?(expected)
) && actual.is_a?(::Array)

or

(
  SuperDiff::RSpec.a_collection_including_something?(expected) ||
  SuperDiff::RSpec.array_including_something?(expected)
) &&
actual.is_a?(::Array)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lib/super_diff/rspec/differs/hash_including.rb Outdated Show resolved Hide resolved
array = if SuperDiff::RSpec.a_collection_including_something?(value)
value.expecteds
else
value.instance_variable_get(:@expected)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense! (I also do this in other places)

@wata727 wata727 force-pushed the add_support_for_rspec_mocks_argument_macthers branch from c98b483 to 60938bd Compare May 4, 2021 06:07
@mcmire
Copy link
Collaborator

mcmire commented May 6, 2021

This is great! Thanks so much for adding this!

@mcmire mcmire merged commit 2bce2b1 into splitwise:master May 6, 2021
@wata727 wata727 deleted the add_support_for_rspec_mocks_argument_macthers branch May 6, 2021 04:46
@mcmire
Copy link
Collaborator

mcmire commented May 8, 2021

Now available in v0.7.0!

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.

2 participants