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

Fuzzy include matchers #85

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
5 participants
@lukeredpath
Contributor

lukeredpath commented Jul 25, 2011

I often want to assert the inclusion of objects in a collection in a more fuzzy way; this often makes tests less brittle as they aren't quite as coupled with the overall equality of objects inside the collection.

This pull request allows fuzzy matching by extending the include() matcher to take matchers as arguments.

A simple example might be:

# where a_user_named is a pre-defined matcher
collection_of_users.should include( a_user_named("Bob") )

I've only implemented this for arrays and the specs/features reflect that but it could possibly be extended to support hashes too.

All specs/features passing locally for me.

lukeredpath added some commits Jul 25, 2011

Should be able to do fuzzy matching against arrays using include?(som…
…e_matcher) or include?(array_of_matchers).

I've deliberately only implemented fuzzy matching support for arrays right now, it might make sense
to extend support to hashes though.
@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 25, 2011

Member

@lukeredpath - nice to see you contributing again!

I'm a little uncomfortable with is_a?(Matcher). We could resolve that with a duck-type check, but another approach to this is just to alias_method :==, :matches? on Matcher. That's how the mock argument matchers work, and there's no reason not to do the same here. The only catch is that the failure message isn't good out of the box, so we'd want a way to improve it. Thoughts?

Member

dchelimsky commented Jul 25, 2011

@lukeredpath - nice to see you contributing again!

I'm a little uncomfortable with is_a?(Matcher). We could resolve that with a duck-type check, but another approach to this is just to alias_method :==, :matches? on Matcher. That's how the mock argument matchers work, and there's no reason not to do the same here. The only catch is that the failure message isn't good out of the box, so we'd want a way to improve it. Thoughts?

@lukeredpath

This comment has been minimized.

Show comment
Hide comment
@lukeredpath

lukeredpath Jul 25, 2011

Contributor

To be honest, so was I and my original thought was to actually just check with a responds_to?(:matches?) instead.

Aliasing :== to :matches? works for me if it doesn't break anything (I'll check). What about dealing with the matcher in to_word? I think I could live with that...what about you?

Contributor

lukeredpath commented Jul 25, 2011

To be honest, so was I and my original thought was to actually just check with a responds_to?(:matches?) instead.

Aliasing :== to :matches? works for me if it doesn't break anything (I'll check). What about dealing with the matcher in to_word? I think I could live with that...what about you?

@lukeredpath

This comment has been minimized.

Show comment
Hide comment
@lukeredpath

lukeredpath Jul 25, 2011

Contributor

OK, I might be missing something obvious but aliasing :== and :matches? doesn't seem to do the trick, namely because she implementation of Array#include calls #== on the collection member, not the matcher, i.e. matcher == object is true but object == matcher is false.

Contributor

lukeredpath commented Jul 25, 2011

OK, I might be missing something obvious but aliasing :== and :matches? doesn't seem to do the trick, namely because she implementation of Array#include calls #== on the collection member, not the matcher, i.e. matcher == object is true but object == matcher is false.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 25, 2011

Member

This looks fantastic! We've tossed around some better ways to do "fuzzy" collection matchers before and never really came up with an API we all liked and agreed with. I really like this.

I was just thinking that this probably won't work with the operator matchers (i.e. should include( > 7)) although it might if you use be--should include(be > 7) but I'm not sure about that. Either way, it'd probably be good to document any matchers that won't work with this in the cuke.

Member

myronmarston commented Jul 25, 2011

This looks fantastic! We've tossed around some better ways to do "fuzzy" collection matchers before and never really came up with an API we all liked and agreed with. I really like this.

I was just thinking that this probably won't work with the operator matchers (i.e. should include( > 7)) although it might if you use be--should include(be > 7) but I'm not sure about that. Either way, it'd probably be good to document any matchers that won't work with this in the cuke.

@lukeredpath

This comment has been minimized.

Show comment
Hide comment
@lukeredpath

lukeredpath Jul 26, 2011

Contributor

I notice you seem to use Cucumber for a lot of documentation; what would be the best way of documenting this?

It's probably worth writing a few specs to see what works and what doesn't either way.

Contributor

lukeredpath commented Jul 26, 2011

I notice you seem to use Cucumber for a lot of documentation; what would be the best way of documenting this?

It's probably worth writing a few specs to see what works and what doesn't either way.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 26, 2011

Member

The cukes are indeed the source of the official docs these days, so that'd be great if you can add a note about the supported matchers to the cuke. We tend to use the cuke feature narrative to put free-form feature discussion. I think that'd be a great place for a note about this.

Member

myronmarston commented Jul 26, 2011

The cukes are indeed the source of the official docs these days, so that'd be great if you can add a note about the supported matchers to the cuke. We tend to use the cuke feature narrative to put free-form feature discussion. I think that'd be a great place for a note about this.

Started to add some integration specs to show the interaction between…
… include()

matcher support and the built-in matchers.
@lukeredpath

This comment has been minimized.

Show comment
Hide comment
@lukeredpath

lukeredpath Jul 26, 2011

Contributor

I've started work on some integration specs for this feature. It's not exhaustive as I wanted to get some feedback on whether you are happy with the approach I've taken (Cucumber documentation will still be needed too).

I didn't feel happy putting these specs in either the include() matcher specs or individual matcher specs; they felt like integration specs so I named it as such.

If you think this is the right idea, I'll find the time to finish them all off. Where a built-in matcher doesn't work, then the integration spec can simply be written to express that.

Contributor

lukeredpath commented Jul 26, 2011

I've started work on some integration specs for this feature. It's not exhaustive as I wanted to get some feedback on whether you are happy with the approach I've taken (Cucumber documentation will still be needed too).

I didn't feel happy putting these specs in either the include() matcher specs or individual matcher specs; they felt like integration specs so I named it as such.

If you think this is the right idea, I'll find the time to finish them all off. Where a built-in matcher doesn't work, then the integration spec can simply be written to express that.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 26, 2011

Member

I think your sense is right to put them in another spec file.

I don't think we necessarily need a spec for every built in matcher. All of the methods that return matcher objects should work fine. My concern was for the non-standard matchers--rspec-matchers has support for them baked directly into the handler and I doubt they will work.

@dchelimsky: can you weigh in about what you'd like to see as far as docs/specs about what matchers are compatible with this feature?

On a side note: I'm noticing that the wording of a matcher that works well with this feature is awkward when used alone, and vice-versa. I'm not sure if there's anything that can be done about that, though. It's easy enough for people to alias the matchers they use with this to something that reads nicer.

Member

myronmarston commented Jul 26, 2011

I think your sense is right to put them in another spec file.

I don't think we necessarily need a spec for every built in matcher. All of the methods that return matcher objects should work fine. My concern was for the non-standard matchers--rspec-matchers has support for them baked directly into the handler and I doubt they will work.

@dchelimsky: can you weigh in about what you'd like to see as far as docs/specs about what matchers are compatible with this feature?

On a side note: I'm noticing that the wording of a matcher that works well with this feature is awkward when used alone, and vice-versa. I'm not sure if there's anything that can be done about that, though. It's easy enough for people to alias the matchers they use with this to something that reads nicer.

@lukeredpath

This comment has been minimized.

Show comment
Hide comment
@lukeredpath

lukeredpath Jul 26, 2011

Contributor

I agree that the built-in matchers don't tend to read well when used with this, but I'm not sure that's a terrible thing. As you say, people can alias, but in all honesty, I think people should be encouraged to write their own domain-specific matchers for this kind of thing. Perhaps this should be gently hinted at in documentation. Every time I've felt the need for this feature I've used a domain-specific matcher.

Having said that, I'm not sure it's as bad the other way around, e.g.:

# this would of course be a rubbish spec
user = User.new(:name => "joe")
user.should == a_user_named("joe")

I'm not sure there is any harm in adding a simple spec for each built-in matcher unless you think it adds some maintenance overhead. It's easy to say it should work fine, but it's better to say it does work fine. ;)

Contributor

lukeredpath commented Jul 26, 2011

I agree that the built-in matchers don't tend to read well when used with this, but I'm not sure that's a terrible thing. As you say, people can alias, but in all honesty, I think people should be encouraged to write their own domain-specific matchers for this kind of thing. Perhaps this should be gently hinted at in documentation. Every time I've felt the need for this feature I've used a domain-specific matcher.

Having said that, I'm not sure it's as bad the other way around, e.g.:

# this would of course be a rubbish spec
user = User.new(:name => "joe")
user.should == a_user_named("joe")

I'm not sure there is any harm in adding a simple spec for each built-in matcher unless you think it adds some maintenance overhead. It's easy to say it should work fine, but it's better to say it does work fine. ;)

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Jul 30, 2011

Member

Assuming this will work with all matchers aside from the operator matchers, I think just a couple of examples of it working correctly and text docs saying "operator matchers are not supported" is fine. Agree it should be in the cuke - not sure I agree it needs a separate integration spec.

Member

dchelimsky commented Jul 30, 2011

Assuming this will work with all matchers aside from the operator matchers, I think just a couple of examples of it working correctly and text docs saying "operator matchers are not supported" is fine. Agree it should be in the cuke - not sure I agree it needs a separate integration spec.

@d11wtq

This comment has been minimized.

Show comment
Hide comment
@d11wtq

d11wtq Sep 30, 2011

I often find myself attempting this:

@some_collection.should include { |v| ... some arbitrary logic }

Which effectively would be the RSpec way of describing any?.

d11wtq commented Sep 30, 2011

I often find myself attempting this:

@some_collection.should include { |v| ... some arbitrary logic }

Which effectively would be the RSpec way of describing any?.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Sep 30, 2011

Member

@d11wtq just use include:

@some_collection.should include("some value")

That's the same as saying:

@some_collection.any? {|e| e == "some value"}.should be_true

If that doesn't answer your question, however, please write the rspec users list rather than diverting this thread.

Member

dchelimsky commented Sep 30, 2011

@d11wtq just use include:

@some_collection.should include("some value")

That's the same as saying:

@some_collection.any? {|e| e == "some value"}.should be_true

If that doesn't answer your question, however, please write the rspec users list rather than diverting this thread.

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Dec 6, 2012

Contributor

@myronmarston, @dchelimsky, any thoughts on this? Are we interested in rebasing this and pulling it in? If so, is there anything about the current implementation that needs to be adjusted?

Contributor

alindeman commented Dec 6, 2012

@myronmarston, @dchelimsky, any thoughts on this? Are we interested in rebasing this and pulling it in? If so, is there anything about the current implementation that needs to be adjusted?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 6, 2012

Member

I still like this a lot. (Sorry about dropping the ball about following up; it's hard to stay on top of all the issues and pull requests!).

A couple concerns I have:

  • Does this work well with something like expect(collection).not_to include(a_user_named("Jack"), a user_named("Jill"))? We had issues a while back where matchers that operate on collections and accepted multiple things to match against (like this does) would work improperly. Consider a case like [1, 2, 3].should_not include(3, 4). The original logic allowed this to pass, because include_matcher.matches? returned false (that is, [1, 2, 3] does not include both 3 and 4). However, we decided that the expected semantics of should_not include(a, b) are that it only passes if BOTH a and b are not included. That's where the send(predicate) business in the matcher comes from.
  • The "voice" of a matcher defined for this is quite different from normal matchers...you wouldn't say user.should a_user_named("Jack"). Not sure what if anything we can do about this.
Member

myronmarston commented Dec 6, 2012

I still like this a lot. (Sorry about dropping the ball about following up; it's hard to stay on top of all the issues and pull requests!).

A couple concerns I have:

  • Does this work well with something like expect(collection).not_to include(a_user_named("Jack"), a user_named("Jill"))? We had issues a while back where matchers that operate on collections and accepted multiple things to match against (like this does) would work improperly. Consider a case like [1, 2, 3].should_not include(3, 4). The original logic allowed this to pass, because include_matcher.matches? returned false (that is, [1, 2, 3] does not include both 3 and 4). However, we decided that the expected semantics of should_not include(a, b) are that it only passes if BOTH a and b are not included. That's where the send(predicate) business in the matcher comes from.
  • The "voice" of a matcher defined for this is quite different from normal matchers...you wouldn't say user.should a_user_named("Jack"). Not sure what if anything we can do about this.

@ghost ghost assigned myronmarston Jan 1, 2013

myronmarston added a commit that referenced this pull request Feb 10, 2013

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 10, 2013

Member

Sorry it's taken me so long...but I finally merged this!

Member

myronmarston commented Feb 10, 2013

Sorry it's taken me so long...but I finally merged this!

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