WIP: spike composable matchers #388

Closed
wants to merge 27 commits into
from

Conversation

Projects
None yet
4 participants
Owner

myronmarston commented Dec 14, 2013

This is a WIP spike at supporting composable matchers (for #280). I'd like to get some feedback before going further down this path. In a nutshell, here's how you make an existing matcher support receiving another matcher as an argument:

  • include RSpec::Matchers::Composable in your matcher class.
  • Use values_match?(expected, actual) in your matches? logic (rather than expected == actual or whatever).
  • Use description_of(object) rather than object.inspect in your failure messages and descriptions.

The phrasing of the existing matchers reads well in an expect(...).to matcher expression, but reads poorly when they are passed as an argument to another matcher. For example, consider these two expressions:

expect { do_it }.to change { x }.from( be_within(0.1).of(3) )
# vs
expect { do_it }.to change { x }.from( a_value_within(0.1).of(3) )

The second reads much better, grammatically. I'd like to provide aliases of all the build-in matchers that "nounifies" them so they read well when passed as arguments to another matcher. In addition, the description method of these aliases needs to be changed a bit to read well. With this PR, the two expressions above read like so:

change result from be within 0.1 of 3

vs

change result from a value within 0.1 of 3

To implement this, I've had to implement a wrapper class that substitutes "a value within" for "be within" in the description. The implementation isn't too bad but it's not as simple as I was hoping for.

Feedback wanted before I proceed with more on this feature.

/cc @xaviershay @JonRowe @alindeman @soulcutter @samphippen

eloyesp and others added some commits Sep 30, 2013

Add chainning matcher capability.
- Make feature use ruby 1.8 hashes.
- Fix feature styling issues.
- Make Composable as a module.
Make the `or` method in the most simple way.
It needs refactoring I know...
Add specs for 'or' composition.
- Use send instead of public_send as it is not available in ruby 1.8.
Refactor composite matchers.
Add error messages.
Rename "composite" to "compound expectation".
We've been talking about supporting composable
matchers in RSpec 3, but that's a slightly
different feature than this, and I think
"compound" gives a more obvious sense of what
this does.
Improve failure messages from compound matchers.
The new formatting handles multiline failure
messages much better.
Remove specs that do not provide much value.
The compound matcher logic is fully exercised by other specs.

It's non-obvious what the purpose of this class is, I think a short class comment could help clarify.

Owner

xaviershay replied Dec 15, 2013

... especially because between BasicObject and method_missing it is doing something "weird".

Owner

myronmarston replied Dec 15, 2013

Yep, it's "weird". I plan to add better comments once we settle on the design, but briefly...

For a matcher like be_within, when we pass it as an arg to another matcher we'd like to use a_value_within and have the description read "a value within 0.1 of 3.0" rather than "be within 0.1 of 3.0". This proxy object is designed to preserve the exact behaviors of the wrapped matcher except for description: in that case, it overrides it using the provided block so it can be phrased differently. That's pretty straight forward.

Things get a bit weird once we start using the chained fluent interface of some matchers. For example for be_within, you write be_within(0.1).of(3.0). of is a method on the BeWithin instance that returns self, which would be the wrapped BeWithin instance rather than the MatcherWithComposedPhrasing (which is a terrible name, BTW: got a better idea?) instance that is overriding description. So, in order to properly override description when it is eventually called, we need to continue to wrap any values returned by proxied messages that appear to be a matcher. In practice, since description is the only thing we care to override, it's the method we check for.

Owner

xaviershay commented Dec 15, 2013

I don't really have my head in this problem, but this change doesn't seem overly complicated to me.

I haven't thought much about whether this abstraction supports "turtles all the way down", i.e. a matcher in a matcher in a matcher, and if it doesn't where the line is. If you've run that thought experiment already, great!

Owner

myronmarston commented Dec 15, 2013

I haven't thought much about whether this abstraction supports "turtles all the way down", i.e. a matcher in a matcher in a matcher, and if it doesn't where the line is. If you've run that thought experiment already, great!

I believe it does, but in practice, you quickly hit the limits of useful. Most matchers actually have semantics that don't make sense for accepting matchers (e.g. because they call a method on the actual object and check if the result is truthy -- there are no arguments involved here!), but as long as you have a use case for which it makes sense to pass matchers to matchers to matchers to matchers to matchers....it'll work. (Well, it will once we finish implementing this.).

Here's an example of a complex nested matcher expression:

expect { |probe|
  do_something(&probe)
}.to yield_with_args( array_including( a_string_matching(/foo/) ) )

...which would pass if the block yielded an argument like ["I", "like", "food"]

Owner

myronmarston commented Dec 15, 2013

A couple further questions that come to mind on this...

  • As this PR currently stands, it defines the matcher aliases in a module (ComposableAliases) that the user has to include in their example group (either in-line, or via RSpec.configuration.include) to have access to. My initial thinking here is that these aliases aren't useful that often and it adds so much more real estate that we're squatting on, so I'd rather let users opt-in to using the aliases. However, thinking about it some more, I don't think the squatting would ever cause any harm here, because the methods are not ones RSpec ever calls or depends on being present, and a user is free to override them for their own purposes with no negative consequences. So now I'm wondering if maybe we should add these aliases to the RSpec::Matchers module so they are always included: then a user can use them with no further configuration (e.g. the include) needed. Any opinions one way or the other?
  • a_value_within was the natural, obvious choice for the be_within alias...but for other matchers it's not quite so clear cut. It'd be good to have a convention we stick with as much as possible. Which forms should we favor?
    • include => array_including / a_collection_including
    • include => an_array_that_includes / a_collection_that_includes
    • respond_to => an_object_responding_to
    • respond_to => an_object_that_responds_to
Owner

xaviershay commented Dec 15, 2013

I don't think the squatting would ever cause any harm here

Agree, I think it is also a bad user experience to have to remember to include a module for a specific set of matchers.

which forms should we favor?

Very slight gut preference for array_including, with no reasoning behind it.

Owner

JonRowe commented Dec 15, 2013

👍 on array_including, also an_object_responding_to

Owner

myronmarston commented Dec 16, 2013

Sounds like the -ing phrasing wins. I think that'll be more consistent with what we already have (e.g. the rspec-mocks hash_including arg matcher). However, I think it should be collection_including, not array_including, because it need not be an array. It supports any type of collection object (including a hash).

@myronmarston myronmarston deleted the spike-composition branch Dec 18, 2013

@myronmarston myronmarston referenced this pull request Dec 19, 2013

Merged

Support composable matchers #393

10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment