Reconcile duplicatated (but slightly different) matchers between rspec-mocks and rspec-expectations

[myronmarston]

The arg matchers are now starting to overlap with the matchers in rspec-expectations as of rspec/rspec-expectations#393. @xaviershay suggested maybe moving the common matchers into rspec-support, and I think I like that idea.

[JonRowe]

I'm wary of making rspec-support an uber gem that basically morphs into an rspec not split in three. I'm not against this specific change, just noting my wariness.

[myronmarston]

So I've been thinking more about this (particularly as there have been other issues that would benefit from this being resolved), and at some point I definitely want us to unify the matchers, but there are multiple options to consider:

1. As the original title of this tickets suggests, we can put common matchers in rspec-support. However, it would be hard to leverage things like our RSpec::Matchers::Composable module from there since it can't depend on rspec-expectations, and to get all the features of normal matchers that's nice to have (since it provides and and or, etc). Also, as @JonRowe said, that starts to make rspec-support an uber gem that's starting to swallow up all of RSpec.
2. We could merge rspec-mocks and rspec-expectations. @dchelimsky has brought this up as an option to think about in the past (particularly when we were working out how to make expect(foo).to receive(:bar) work as an rspec-mocks API w/ and w/o rspec-expectations given that the latter usually provides expect). I still think keeping them separate is worthwhile as plenty of folks like using an alternate library like bogus, rr or mocha for mocking, and likewise some folks want to use rspec-mocks but prefer minitest's assertions or wrong. (So I'm mostly brining this option up for completeness but I think I'm against it).
3. We could move the matchers to rspec-expectations and then make rspec-mocks depend on rspec-expectations. That way, the matchers would be in one place (rspec-expectations) while being available from here as well. Downside: rspec-expectations still monkey patches should onto everything when loaded (although it triggers a warning when used), and I wouldn't want that dependency as long as rspec-expectations has the baggage of the monkeypatches on-by-default. This might be an option to consider for RSpec 4, though.
4. We could move most matchers to rspec-expectations and keep a bare minimum set here of the ones that only make sense as argument matchers such as anything and any_args. We'd continue to leverage rspec-support's fuzzy matching logic (which uses === and ==), so that would allow common things like classes and regexes to be usable just fine, as well as explicit objects that are equal. If users wanted more matchers beyond that basic set, we'd recommend they also add rspec-expectations to the project as it would provide a full set of matchers. Again, this is probably something we can only consider for RSpec 4, since for SemVer reasons we need to keep the arg matchers here until the next major version. Actually...we could potentially do it in 3.x if we port them to rspec-expectations but keep a version of them here that is only included if rspec-expectations isn't also loaded.

What do others think? I'm linking option 4 the more I think about it. It "feels right" to me to make rspec-expectations be the main source of matchers, that we recommend users use with rspec-mocks if they want go beyond the basics of arg matching.

[fables-tales]

I like 4 the best, with the sort of caveat that matchers in rspec-expectations should never be mocking specific. For example in the expression: expect(:foo).to receive(:blah).with(hash_including(:foo => bar)) (where hash_including is the rspec mocks hash including) I see no reason why we could not move that matcher into RSpec expectations (it's doing hash matching, basically). There are many similar matchers that aren't clearly argument-matching specific when you take them out of that context.

For example:

• hash_including as mentioned above
• hash_excluding
• array_including
• instance_of
• kind_of

but then some clearly are:

• any_args
• no_args

I think overall I'm definitely in favour of number 4, as long as we're selective about which matchers end up getting moved. I think the matchers that do get moved should be only those that clearly make sense outside of an argument matching context.

[myronmarston]

FWIW, we already have hash_including and array_including in rspec-expectations in the form of the include matcher (which has aliases a_hash_including and a_collection_including), with the caveat that the rspec-expectations form doesn't validate the object type, so you can use a_hash_including(:foo, :bar) expecting it to verify it has those keys and it could be an array with those values instead.

instance_of and kind_of are available in rspec-expectations as an_instance_of and a_kind_of as well.

[JonRowe]

I'd support option 4 in the long term. I'm not sure I like this myself but I'd like to suggest:

5. Create an rspec-matchers gem that contains the matchers for use between mocks and expectations.

[myronmarston]

5. Create an rspec-matchers gem that contains the matchers for use between mocks and expectations.

I thought about that, but then I realized that that's basically what rspec-expectations is: a collection of matchers plus an API to use them and an API to easily define them.

Once we extract the matchers into their own gem, there's not much left to rspec-expectations, and once it does no monkey patching by default, I don't see any real problems or downsides of including it in a project that only wants to use it for argument matching with rspec-mocks.

[JonRowe]

Like I said, not sure I like it :P just wanted to put it out there

[myronmarston]

I'd support option 4 in the long term.

Any reason you wouldn't support it in the short term, provided we can find a way to keep full SemVer backwards compatibility?

Option 4 seems to have broad support so I think I'm going to do an audit of the matchers and try to list out a plan here for each one, in terms of which ones we move, which ones we keep here, and any open questions I can think of. This issue seems to be gating other things, so I want to tackle this soon.

[JonRowe]

Any reason you wouldn't support it in the short term, provided we can find a way to keep full SemVer backwards compatibility?

No, as long as we can keep SemVer compatibility reasonably cleanly.

[myronmarston]

I've audited the matchers we have in rspec-mocks. Here's the list, grouped into categories:

• Must remain in rspec-mocks. These matchers need special handling in rspec-mocks (as they can't simply match against an object) and only make sense as argument matchers.
  • any_args
  • no_args
• Could be moved to rspec-expectations but probably should stay in rspec-mocks.
  • anything: No equivalent in rspec-expectations, but could be useful (e.g. expect([3, 3.5, 4]).to match([3, anything, 4])). anything is pretty necessary for matching args though (at least if you want to be careful/specific about which args you are matching), and given that no one has ever asked for it as an rspec-expectations matcher, my instinct is to leave it for now. (That doesn't preclude us from moving it later on).
  • boolean: No equivalent in rspec-expectations. I've never used this matcher, to be honest. I could see it being useful in rspec-expectations, but my instinct is to leave it for now.
• Has an equivalent in rspec-expectations that behaves identically.
  • duck_type: Available as an_object_responding_to in rspec-expectations. We should make a duck_type alias of that in rspec-expectations and plan to remove this from here. The rspec-expectations matcher is more flexible, anyway, as it allows you to specify arity, etc.
  • instance_of/an_instance_of: Available as an_instance_of in rspec-expectations (so they already overlap - in fact the rspec-expectations method takes precedence!).
• Has an equivalent in rspec-expectations that behaves slightly differently.
  • kind_of: In 2.x this was implemented using kind_of?, but in 3.0, kind_of just returns the argument since RSpec 3 uses === and Module#=== already provides the same semantics as kind_of?. In rspec-expectations it uses kind_of?, so the behavior differs slightly. For example, for objects or classes that define a custom kind_of? or === implementation, these would behave different. However, our docs still say that it uses kind_of? and I think it's fair to use rspec-expectation's version and not consider it a breaking change.
  • hash_including: Available as a_hash_including (an alias of include) in rspec-expectations and we can simply define an additional hash_including alias. However, it behaves differently in a few ways:
    • rspec-expectations' version raises errors when given an arg that does not respond to include?. (Fix coming in rspec/rspec-expectations#607). rspec-mocks' version does not (returns false instead).
    • Both support being given a list of keys, but rspec-mocks' version won't match against an Array, whereas rspec-expectations' version will. For example, hash_including(:a, :b) === [:a, :b, :c] returns false, but a_hash_including(:a, :b) === [:a, :b, :c] returns true.
  • array_including: Available as a_collection_including (again an alias of include) in rspec-expectations. Can be aliased to array_including. Like with hash_including, there are some differences:
    • Since rspec-expectations' version is an alias of include, which works on hashes, and it can match against a set of hash keys, a_collection_including(:a, :b) === { :a => 1, :b => 2 } returns true, whereas array_including(:a, :b) === { :a => 1, :b => 2 } returns false. Likewise, a_collection_including(:a => 1) === { :a => 1 } returns true, but array_including(:a => 1) === { :a => 1 } returns false.
    • array_including has some weird/helpful behavior that allows items to be passed individually or packaged in an array. I've mentioned elsewhere that I'd like to do away with this behavior but we have to consider it if we're going to address this issue in the 3.x timeframe.
• Does not yet have an equivalent in rspec-expectations but probably will once rspec/rspec-expectations#547 is merged.
  • hash_excluding/hash_not_including: I expect we'll add this to rspec-expectations as a negative matcher but the behavior may not be identical.

I'll follow up on this in a later comment with some specific thoughts about what we should do for the issues outlined above. For now I just wanted to catalog them and the issues I'm aware of.

[cupakromer]

I'm on board with option 4.

• Could be moved to rspec-expectations but probably should stay in rspec-mocks.
  • anything: No equivalent in rspec-expectations, but could be useful (e.g. expect([3, 3.5, 4]).to match([3, anything, 4])). anything is pretty necessary for matching args though (at least if you want to be careful/specific about which args you are matching), and given that no one has ever asked for it as an rspec-expectations matcher, my instinct is to leave it for now. (That doesn't preclude us from moving it later on).
  • boolean: No equivalent in rspec-expectations. I've never used this matcher, to be honest. I could see it being useful in rspec-expectations, but my instinct is to leave it for now.

:+1: I can't remember if I've used anything like that before, but I feel like I have. Since I've always used both rspec-expectations and rspec-mocks, I never paid much attention to which lib it was in before. Would be good to be available in both.

• Has an equivalent in rspec-expectations that behaves identically.

:+1:

• Has an equivalent in rspec-expectations that behaves slightly differently.
  • kind_of: In 2.x this was implemented using kind_of?, but in 3.0, kind_of just returns the argument since RSpec 3 uses === and Module#=== already provides the same semantics as kind_of?. In rspec-expectations it uses kind_of?, so the behavior differs slightly. For example, for objects or classes that define a custom kind_of? or === implementation, these would behave different. However, our docs still say that it uses kind_of? and I think it's fair to use rspec-expectation's version and not consider it a breaking change.

Yes, let's standardize on using kind_of?. It makes sense that the matcher uses this as per it's name. If someone wants custom === behavior, they should used the class/module constant per normal Ruby case-statement semantics.

• hash_including: Available as a_hash_including (an alias of include) in rspec-expectations and we can simply define an additional hash_including alias. However, it behaves differently in a few ways:
  • rspec-expectations' version raises errors when given an arg that does not respond to include?. (Fix coming in rspec/rspec-expectations#607). rspec-mocks' version does not (returns false instead).
  • Both support being given a list of keys, but rspec-mocks' version won't match against an Array, whereas rspec-expectations' version will. For example, hash_including(:a, :b) === [:a, :b, :c] returns false, but a_hash_including(:a, :b) === [:a, :b, :c] returns true.

I think both versions of hash_including (and aliases except include more on that later) should return false. The matcher was provided the wrong type when asked to match and per our protocol that should return false. The name implies a hash or something that quacks like one, an array is neither.

• array_including: Available as a_collection_including (again an alias of include) in rspec-expectations. Can be aliased to array_including. Like with hash_including, there are some differences:
  • Since rspec-expectations' version is an alias of include, which works on hashes, and it can match against a set of hash keys, a_collection_including(:a, :b) === { :a => 1, :b => 2 } returns true, whereas array_including(:a, :b) === { :a => 1, :b => 2 } returns false. Likewise, a_collection_including(:a => 1) === { :a => 1 } returns true, but array_including(:a => 1) === { :a => 1 } returns false.

This is a bit trickier than above. Let me address a_collection_including first. I think this matcher is properly aliased with include as a collection can be many different types. Both hashes and arrays are collections of things. All collections can include other things. My current vote is for the rspec-expectations version for that.

Now regarding array_including, like with hash_including, I would think this should have different behavior. While you could argue that an array of two element arrays is a hash (surely Ruby let's you convert this), I personally think of them as fundamentally different types. As with hash_including, I would think that array_including would return false when provided a hash to match against. Additionally, I would think array_including(:a => 1) should raise an ArgumentError as you cannot create a proper matcher from this condition.

• array_including has some weird/helpful behavior that allows items to be passed individually or packaged in an array. I've mentioned elsewhere that I'd like to do away with this behavior but we have to consider it if we're going to address this issue in the 3.x timeframe.

I agree we should change / standardize this behavior. Though personally I'm not sure what I'd like to see. When I manually write the array out, this style feels more natural: array_including(1, 2, 3). It's clear I expect the array to include those individual things. Having to do array_including([1, 2, 3]) wouldn't make sense to me.

However, if I stored the matcher argument array in a variable, then this feels natural: array_including(required_elements). And I would undoubtedly be tripped up having to do array_including(*required_elements). However, this happens to me all the time, especially when I work with Kernel#system or Process#spawn. So, it is something I would be ok with requiring. We could adjust the failure message to add a helpful hint if we see that a single array argument was provided to match against.

include matcher and relatives wrap-up

Based on my last three sets of comments, I feel that the inclusion matchers need to be split-up. I would suggest the following sets:

• hash_including, a_hash_including (returns false when matching against an array)
• array_including, an_array_including (returns false when matching against a hash)
• include, a_collection_including (uses the more intuitive / flexible matching options)

[myronmarston]

:+1: I can't remember if I've used anything like that before, but I feel like I have. Since I've always used both rspec-expectations and rspec-mocks, I never paid much attention to which lib it was in before. Would be good to be available in both.

Your response here confuses me a bit since I said "my instinct is to leave anything in rspec-mocks" and you gave a thumbs up, then said it would be good to be available in both (which suggests you think we should move it to rspec-expectations). FWIW, regardless of which lib it's defined in it'll be available in both in projects that use both.

Re: hash_including vs array_including: rather than splitting these as entirely separate matchers (since the logic of include works for both cases and the rspec-expectations equivalents are just aliases), I'm thinking about expanding RSpec::Matchers.alias_matcher so that it can take an additional type argument that would cause the aliased matcher to do an additional type check when matching. e.g. RSpec::Matchers.alias_matcher :a_hash_including, :include, Hash would cause it to do an additional Hash === actual check on top of delegating to include. This would provide a generalized mechanism for dealing with these sorts of issues. One potential issue: it could be considered a breaking change, but then again, the fact that a_hash_including can match against an array would probably be considered a bug by many users so maybe most users would consider it a bug fix?

[cupakromer]

Sorry, about the confusion. I had misread the original statement. I think it would be good to be in both standalone to avoid possible future confusion. That also documents that the .to eq [3, anything, 5] usage is by design and not coincidence.

I'm thinking about expanding RSpec::Matchers.alias_matcher so that it can take an additional type argument that would cause the aliased matcher to do an additional type check when matching.

:+1: that works for me.

[JonRowe]

• Must remain in rspec-mocks. These matchers need special handling](https://github.com/rspec/rspec-mocks/blob/v3.0.2/lib/rspec/mocks/argument_list_matcher.rb#L49-L50) in rspec-mocks (as they can't simply match against an object) and only make sense as argument matchers.
  • any_args
  • no_args

Agree

• Could be moved to rspec-expectations but probably should stay in rspec-mocks.
  • anything: No equivalent in rspec-expectations, but could be useful (e.g. expect([3, 3.5, 4]).to match([3, anything, 4])). anything is pretty necessary for matching args though (at least if you want to be careful/specific about which args you are matching), and given that no one has ever asked for it as an rspec-expectations matcher, my instinct is to leave it for now. (That doesn't preclude us from moving it later on).
  • boolean: No equivalent in rspec-expectations. I've never used this matcher, to be honest. I could see it being useful in rspec-expectations, but my instinct is to leave it for now.

Agree on both counts.

• Has an equivalent in rspec-expectations that behaves identically.
  • duck_type: Available as an_object_responding_to in rspec-expectations. We should make a duck_type alias of that in rspec-expectations and plan to remove this from here. The rspec-expectations matcher is more flexible, anyway, as it allows you to specify arity, etc.

So for now we'd need to detect it's existence and then define or alias it depending?

• instance_of/an_instance_of: Available as an_instance_of in rspec-expectations (so they already overlap - in fact the rspec-expectations method takes precedence!).

Sweet, means theres probably a low risk of this going wrong :P, as above we'll have to just detect it and then define an alias or define it as a fallback?

• Has an equivalent in rspec-expectations that behaves slightly differently.
  • kind_of: In 2.x this was implemented using kind_of?, but in 3.0, kind_of just returns the argument since RSpec 3 uses === and Module#=== already provides the same semantics as kind_of?. In rspec-expectations it uses kind_of?, so the behavior differs slightly. For example, for objects or classes that define a custom kind_of? or === implementation, these would behave different. However, our docs still say that it uses kind_of? and I think it's fair to use rspec-expectation's version and not consider it a breaking change.

As per #744 it seems we should switch back to kind_of? anyway.

• hash_including: Available as a_hash_including (an alias of include) in rspec-expectations and we can simply define an additional hash_including alias. However, it behaves differently in a few ways:
  • rspec-expectations' version raises errors when given an arg that does not respond to include?. (Fix coming in rspec/rspec-expectations#607). rspec-mocks' version does not (returns false instead).
  • Both support being given a list of keys, but rspec-mocks' version won't match against an Array, whereas rspec-expectations' version will. For example, hash_including(:a, :b) === [:a, :b, :c] returns false, but a_hash_including(:a, :b) === [:a, :b, :c] returns true.

We can use the same underlying implementation and flag it as to wether it raises or not no?

• array_including: Available as a_collection_including (again an alias of include) in rspec-expectations. Can be aliased to array_including. Like with hash_including, there are some differences:
  • Since rspec-expectations' version is an alias of include, which works on hashes, and it can match against a set of hash keys, a_collection_including(:a, :b) === { :a => 1, :b => 2 } returns true, whereas array_including(:a, :b) === { :a => 1, :b => 2 } returns false. Likewise, a_collection_including(:a => 1) === { :a => 1 } returns true, but array_including(:a => 1) === { :a => 1 } returns false.
  • array_including has some weird/helpful behavior that allows items to be passed individually or packaged in an array. I've mentioned elsewhere that I'd like to do away with this behavior but we have to consider it if we're going to address this issue in the 3.x timeframe.

I vote we standardise on passed individually.

• Does not yet have an equivalent in rspec-expectations but probably will once rspec/rspec-expectations#547 is merged.
  • hash_excluding/hash_not_including: I expect we'll add this to rspec-expectations as a negative matcher but the behavior may not be identical.

Seems legit.

[myronmarston]

We can use the same underlying implementation and flag it as to wether it raises or not no?

Huh?

[JonRowe]

I mean can we share the same implementation but just have a way of telling it wether to raise or not internally