Implement chained (composed) assertions. #329

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
5 participants
@eloyesp
Contributor

eloyesp commented Sep 30, 2013

The idea is to make possible to have multiple matchers in a sigle expectation.
It does only works with matchers based on BaseMatcher as it define the and
method there.

This idea arised in the issue #280 about make composable matchers.

I've added specs and cucumber features, but I'm almost sure that I've missed
some functionality as the error messages. I can work on those but I want to be
sure that I'm not misdirected.

Can I have some feedback?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 30, 2013

Coverage Status

Coverage decreased (-0.22%) when pulling 8d8fb70153312c870a3f6c7bac962e48ad0dac32 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

Coverage Status

Coverage decreased (-0.22%) when pulling 8d8fb70153312c870a3f6c7bac962e48ad0dac32 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 30, 2013

Coverage Status

Coverage increased (+0.16%) when pulling 0842cf7c15976e31a6e9786d1abcdc8d63df1b83 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

Coverage Status

Coverage increased (+0.16%) when pulling 0842cf7c15976e31a6e9786d1abcdc8d63df1b83 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

@xaviershay

View changes

features/built_in_matchers/composition.feature
+Feature: matcher composition
+
+ Matchers can be composed to make several assertions on the same object.
+ For example we can specify that it should be a Hash and also that it contains

This comment has been minimized.

@xaviershay

xaviershay Oct 1, 2013

Member

should backtick Hash here to be consistent (or lowercase it)

@xaviershay

xaviershay Oct 1, 2013

Member

should backtick Hash here to be consistent (or lowercase it)

@xaviershay

View changes

features/built_in_matchers/composition.feature
+ """ruby
+ describe "composing matchers" do
+ it "make both assertions" do
+ expect({ :foo => 'bar' }).to be_kind_of(Hash).and(include(:foo))

This comment has been minimized.

@xaviershay

xaviershay Oct 1, 2013

Member

{} are redundant, remove them.

@xaviershay

xaviershay Oct 1, 2013

Member

{} are redundant, remove them.

@xaviershay

View changes

lib/rspec/matchers/built_in/composite.rb
+ end
+
+ def matches?(actual)
+ base_matcher.matches?(actual) and new_matcher.matches?(actual)

This comment has been minimized.

@xaviershay

xaviershay Oct 1, 2013

Member

Use && instead of and.

@xaviershay

xaviershay Oct 1, 2013

Member

Use && instead of and.

This comment has been minimized.

@myronmarston

myronmarston Oct 1, 2013

Member

To give some context to what @xaviershay said -- and is intended for control flow and && is for boolean expressions. Due to their different precedence rules, you can get some nasty surprises when using and in boolean expressions like this and it's best to avoid that.

@myronmarston

myronmarston Oct 1, 2013

Member

To give some context to what @xaviershay said -- and is intended for control flow and && is for boolean expressions. Due to their different precedence rules, you can get some nasty surprises when using and in boolean expressions like this and it's best to avoid that.

@xaviershay

View changes

lib/rspec/matchers/built_in/composite_negative.rb
+ class CompositeNegative < Composite
+
+ def matches?(actual)
+ base_matcher.matches?(actual) and not new_matcher.matches?(actual)

This comment has been minimized.

@xaviershay

xaviershay Oct 1, 2013

Member

ditto

This comment has been minimized.

@myronmarston

myronmarston Oct 1, 2013

Member

Some matchers define custom negation logic using does_not_match?. You can see how rspec-expectations uses that internally, falling back to matches?:

match = matcher.respond_to?(:does_not_match?) ?
!matcher.does_not_match?(actual, &block) :
matcher.matches?(actual, &block)
return match unless match

The reason for this is for a case like this:

expect(some_object).not_to respond_to(:foo, :bar)

The respond_to matcher has separate negative matching logic for this:

https://github.com/rspec/rspec-expectations/blob/a74dee7a8a50df0f0d11e62697f1cf3b6ac39a61/lib/rspec/matchers/built_in/respond_to.rb

Because the expectation above is stating that both :foo and :bar are messages that some_object does not respond to (at least, that's what people mean when they use it). If it simply used the negation of matches?, then it would pass if some_object responded to :foo but not :bar or vice versa.

@myronmarston

myronmarston Oct 1, 2013

Member

Some matchers define custom negation logic using does_not_match?. You can see how rspec-expectations uses that internally, falling back to matches?:

match = matcher.respond_to?(:does_not_match?) ?
!matcher.does_not_match?(actual, &block) :
matcher.matches?(actual, &block)
return match unless match

The reason for this is for a case like this:

expect(some_object).not_to respond_to(:foo, :bar)

The respond_to matcher has separate negative matching logic for this:

https://github.com/rspec/rspec-expectations/blob/a74dee7a8a50df0f0d11e62697f1cf3b6ac39a61/lib/rspec/matchers/built_in/respond_to.rb

Because the expectation above is stating that both :foo and :bar are messages that some_object does not respond to (at least, that's what people mean when they use it). If it simply used the negation of matches?, then it would pass if some_object responded to :foo but not :bar or vice versa.

@xaviershay

View changes

lib/rspec/matchers/built_in/composite.rb
+
+ attr_reader :base_matcher, :new_matcher
+
+ def initialize base_matcher, new_matcher

This comment has been minimized.

@xaviershay

xaviershay Oct 1, 2013

Member

Add brackets around method def.

@xaviershay

xaviershay Oct 1, 2013

Member

Add brackets around method def.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Oct 1, 2013

Member

Please add a changelog entry.

Member

xaviershay commented Oct 1, 2013

Please add a changelog entry.

@xaviershay

This comment has been minimized.

Show comment
Hide comment
@xaviershay

xaviershay Oct 1, 2013

Member

Nit-picking aside, this looks pretty good to me.

The describe language doesn't quite feel right, I'll have a sleep on that and come up with some suggestions later if no one beats me to the punch.

Thanks for doing this!

Member

xaviershay commented Oct 1, 2013

Nit-picking aside, this looks pretty good to me.

The describe language doesn't quite feel right, I'll have a sleep on that and come up with some suggestions later if no one beats me to the punch.

Thanks for doing this!

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Oct 1, 2013

Member

@eloyesp -- Nice work! This will be a nice building block towards having fully composable matchers. I have some general thoughts/suggestions, and then I'll dig into some specifics in later comments.

  • It would be nice to have or for parity.
  • Rather than and_not, I'm thinking maybe we should just add not as it's own building block, so that you could do be_a(Hash).and(not include(:some_key)). The benefit I see is that not becomes a nice building block on its own; for example, expect(array).to include(not match(/some regex/)) could be used to specify that an array should include something that does not match /some regex/. It could also be used with or if we added that.
  • That said, not is a keyword, and if you use it without an explicit receiver it doesn't send the not message. An alternative method name is not_to, as it reads nicely for a case like expect(k).to be_a(Hash).and(not_to include(:some_key)). That said, it doesn't read very nicely in the include(not_to match(/some regex/)) case. I'm not sure what's best here.
  • It would be nice for users to have a simple way to make there matchers composable as well. To that end, what do you think about moving the definition of and out of BaseMatcher and into a RSpec::Matchers::Composable module that we document as a publicly available module that users can mixin to their matchers to make them composable? I suspect there'll be more that would go into there. Then BaseMatcher can include it.
Member

myronmarston commented Oct 1, 2013

@eloyesp -- Nice work! This will be a nice building block towards having fully composable matchers. I have some general thoughts/suggestions, and then I'll dig into some specifics in later comments.

  • It would be nice to have or for parity.
  • Rather than and_not, I'm thinking maybe we should just add not as it's own building block, so that you could do be_a(Hash).and(not include(:some_key)). The benefit I see is that not becomes a nice building block on its own; for example, expect(array).to include(not match(/some regex/)) could be used to specify that an array should include something that does not match /some regex/. It could also be used with or if we added that.
  • That said, not is a keyword, and if you use it without an explicit receiver it doesn't send the not message. An alternative method name is not_to, as it reads nicely for a case like expect(k).to be_a(Hash).and(not_to include(:some_key)). That said, it doesn't read very nicely in the include(not_to match(/some regex/)) case. I'm not sure what's best here.
  • It would be nice for users to have a simple way to make there matchers composable as well. To that end, what do you think about moving the definition of and out of BaseMatcher and into a RSpec::Matchers::Composable module that we document as a publicly available module that users can mixin to their matchers to make them composable? I suspect there'll be more that would go into there. Then BaseMatcher can include it.
@myronmarston

View changes

lib/rspec/matchers/built_in/composite.rb
+ base_matcher.matches?(actual) and new_matcher.matches?(actual)
+ end
+
+ end

This comment has been minimized.

@myronmarston

myronmarston Oct 1, 2013

Member

The matcher needs a good, human readable failure message (for both positive and negative cases) and a description. I think you can define it with simple delegation:

def failure_message_for_should
  "#{base_matcher.failure_message_for_should} and #{new_matcher.failure_message_for_should}"
end

# etc for the other methods

(These things would need specs, of course.)

@myronmarston

myronmarston Oct 1, 2013

Member

The matcher needs a good, human readable failure message (for both positive and negative cases) and a description. I think you can define it with simple delegation:

def failure_message_for_should
  "#{base_matcher.failure_message_for_should} and #{new_matcher.failure_message_for_should}"
end

# etc for the other methods

(These things would need specs, of course.)

@myronmarston

View changes

spec/rspec/matchers/base_matcher_spec.rb
+
+ describe "#and" do
+
+ it "chain multiple assertions" do

This comment has been minimized.

@myronmarston

myronmarston Oct 1, 2013

Member

it "chain multiple assertions" is not a very descriptive doc string. And really, there are multiple examples below. Can these be broken up into descriptive examples?

it "passes when both matchers match"
it "fails when the first matcher does not match but the second matches"
it "fails when the first matcher matches but the second does not"
it "fails when both matchers do not match"
@myronmarston

myronmarston Oct 1, 2013

Member

it "chain multiple assertions" is not a very descriptive doc string. And really, there are multiple examples below. Can these be broken up into descriptive examples?

it "passes when both matchers match"
it "fails when the first matcher does not match but the second matches"
it "fails when the first matcher matches but the second does not"
it "fails when both matchers do not match"
@myronmarston

View changes

spec/rspec/matchers/base_matcher_spec.rb
+ expect(matcher.new(4).and(matcher.new(3)).matches?(3)).to be_falsey
+ expect(matcher.new(3).and(matcher.new(4)).matches?(3)).to be_falsey
+ expect(matcher.new(3).and(matcher.new(3)).matches?(4)).to be_falsey
+ end

This comment has been minimized.

@myronmarston

myronmarston Oct 1, 2013

Member

In my specs for matchers, I prefer to use the matcher as a user would (e.g. in an expect( ).to expression) rather than calling matches? directly. This helps show how the matcher will actually be used, and surfaces what the failure message will be (which is really important). With this approach there are at least 4 cases that need to be spec'd:

  • Using expect( ).to matcher where it passes.
  • Using expect( ).to matcher where it fails.
  • Using expect( ).not_to matcher where it passes.
  • Using expect( ).not_to matcher where it fails.

For an example of this, see the yield_spec.

@myronmarston

myronmarston Oct 1, 2013

Member

In my specs for matchers, I prefer to use the matcher as a user would (e.g. in an expect( ).to expression) rather than calling matches? directly. This helps show how the matcher will actually be used, and surfaces what the failure message will be (which is really important). With this approach there are at least 4 cases that need to be spec'd:

  • Using expect( ).to matcher where it passes.
  • Using expect( ).to matcher where it fails.
  • Using expect( ).not_to matcher where it passes.
  • Using expect( ).not_to matcher where it fails.

For an example of this, see the yield_spec.

@eloyesp

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Oct 1, 2013

Contributor

Thank you for your comments, I will work on this ASAP.

Contributor

eloyesp commented Oct 1, 2013

Thank you for your comments, I will work on this ASAP.

@xaviershay

View changes

features/built_in_matchers/composition.feature
+ end
+
+ it "deliberate failure on the second matcher" do
+ expect({ :foo => 'bar' }).to be_kind_of(Hash).and(include(:not_included))

This comment has been minimized.

@xaviershay

xaviershay Oct 1, 2013

Member

On a philosophical point, checking the type of an object doesn't feel like good Ruby style, and as such I don't think we should be leading with it in our documentation. Why not use the same example you used in the text above (contains this key but not that key)?

@xaviershay

xaviershay Oct 1, 2013

Member

On a philosophical point, checking the type of an object doesn't feel like good Ruby style, and as such I don't think we should be leading with it in our documentation. Why not use the same example you used in the text above (contains this key but not that key)?

This comment has been minimized.

@JonRowe

JonRowe Oct 1, 2013

Member

👍

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 1, 2013

Coverage Status

Coverage increased (+0.16%) when pulling aa6af5b8e93200d14c2d7b0f12a297396602395b on eloyesp:chainned_assertions into a74dee7 on rspec:master.

Coverage Status

Coverage increased (+0.16%) when pulling aa6af5b8e93200d14c2d7b0f12a297396602395b on eloyesp:chainned_assertions into a74dee7 on rspec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 2, 2013

Coverage Status

Coverage increased (+0.16%) when pulling 95c7f6d6ca16709b95fde58ba3b6f8717acc04e7 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

Coverage Status

Coverage increased (+0.16%) when pulling 95c7f6d6ca16709b95fde58ba3b6f8717acc04e7 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 2, 2013

Coverage Status

Coverage increased (+0.11%) when pulling 6145c8c3159054cb606826cf58674826b517fbf3 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

Coverage Status

Coverage increased (+0.11%) when pulling 6145c8c3159054cb606826cf58674826b517fbf3 on eloyesp:chainned_assertions into a74dee7 on rspec:master.

@myronmarston

View changes

lib/rspec/matchers/built_in/composite.rb
+ else
+ raise ArgumentError
+ end
+ end

This comment has been minimized.

@myronmarston

myronmarston Oct 2, 2013

Member

I generally prefer polymorphism to conditionals such as case statements unless the introduction of an additional class introduces lots of complexity. I don't think it would here, though; I think it would simplify to have an AndComposite and an OrComposite matcher, each of which does the correct thing. Then there's no need for the else case here (or a case statement at all) and the and and or methods can return the appropriate one.

@myronmarston

myronmarston Oct 2, 2013

Member

I generally prefer polymorphism to conditionals such as case statements unless the introduction of an additional class introduces lots of complexity. I don't think it would here, though; I think it would simplify to have an AndComposite and an OrComposite matcher, each of which does the correct thing. Then there's no need for the else case here (or a case statement at all) and the and and or methods can return the appropriate one.

@eloyesp

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Nov 5, 2013

Contributor

After a lot of thinking I'm almost sure that the 'to_not' side of this matcher should not be implemented. It is not really clear what should mean to negate them.

Also I'm thinking that I have a deeper insight about how to solve the negation problem that @myronmarston presented here.

I think that all the negation responsibility should be moved from the expectation to the matcher

# transforming
expect({a: 'b'}).to_not include(:b)
# into
expect({a: 'b'}).to not_include(:b)

Then, chaining matchers would be written like this:

expect({a: 'b'}).to include(:a).and not_include(:b, :c)

Then not_included, not_matching, etc need only to be decorators for the positive matcher (NegativeMatcher) that turns match? into not_match? and the same with the failure message.

On the other hand I feel that the ExpectationHandler violation of TDA turns out to be an obstacle as I need to implement it again for the CompositeMatcher.

That said I wanted to make clear that this comment have not the intention of offend, as I'm willing to learn (and I already learned a lot) and I think that this is an awesome library. (But I'm feeling now that not being English my native language it may be not understood).

I'm working now on the last commits and can do a re-base if you you think it is necessary.

Also, please feel free to comment any correction on my code or comments. Thanks again.

Contributor

eloyesp commented Nov 5, 2013

After a lot of thinking I'm almost sure that the 'to_not' side of this matcher should not be implemented. It is not really clear what should mean to negate them.

Also I'm thinking that I have a deeper insight about how to solve the negation problem that @myronmarston presented here.

I think that all the negation responsibility should be moved from the expectation to the matcher

# transforming
expect({a: 'b'}).to_not include(:b)
# into
expect({a: 'b'}).to not_include(:b)

Then, chaining matchers would be written like this:

expect({a: 'b'}).to include(:a).and not_include(:b, :c)

Then not_included, not_matching, etc need only to be decorators for the positive matcher (NegativeMatcher) that turns match? into not_match? and the same with the failure message.

On the other hand I feel that the ExpectationHandler violation of TDA turns out to be an obstacle as I need to implement it again for the CompositeMatcher.

That said I wanted to make clear that this comment have not the intention of offend, as I'm willing to learn (and I already learned a lot) and I think that this is an awesome library. (But I'm feeling now that not being English my native language it may be not understood).

I'm working now on the last commits and can do a re-base if you you think it is necessary.

Also, please feel free to comment any correction on my code or comments. Thanks again.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 10, 2013

Member

Hey, @eloyesp, thanks for this. I'm finally getting around to adding the rspec-expectations features for rspec 3. I'm going to rebase and squash this and get this merged soon.

Member

myronmarston commented Dec 10, 2013

Hey, @eloyesp, thanks for this. I'm finally getting around to adding the rspec-expectations features for rspec 3. I'm going to rebase and squash this and get this merged soon.

eloyesp 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.
@eloyesp

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Dec 11, 2013

Contributor

@myronmarston Sorry for the delay but the work didn't left me enough time to work on this. I re-based on master and squashed some minor commits to see if that help.

Thanks for the learning experience, for the comments and for the patience.

Contributor

eloyesp commented Dec 11, 2013

@myronmarston Sorry for the delay but the work didn't left me enough time to work on this. I re-based on master and squashed some minor commits to see if that help.

Thanks for the learning experience, for the comments and for the patience.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 11, 2013

@eloyesp -- can you speak to the reason for this refactoring? I think I prefer how it was, but I want to understand your reasoning.

@eloyesp -- can you speak to the reason for this refactoring? I think I prefer how it was, but I want to understand your reasoning.

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Dec 11, 2013

Owner

Well, I didn't like the idea of constricting to two matchers as it makes sense to say something as:

my_custom_matcher = AndMatcher.new(be_kind_of(Array), include(:foo), end_with(2))
expect([:foo, 2]).to my_custom_matcher

Also the syntax in matchers.all? seems pretty than this_matcher && other_matcher.

On the other hand, keeping the evaluated_matchers in an array, help with the error messages (I couldn't make this pass without that).

Owner

eloyesp replied Dec 11, 2013

Well, I didn't like the idea of constricting to two matchers as it makes sense to say something as:

my_custom_matcher = AndMatcher.new(be_kind_of(Array), include(:foo), end_with(2))
expect([:foo, 2]).to my_custom_matcher

Also the syntax in matchers.all? seems pretty than this_matcher && other_matcher.

On the other hand, keeping the evaluated_matchers in an array, help with the error messages (I couldn't make this pass without that).

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 11, 2013

I see. You didn't actually document that use case with a spec.

Anyhow, I think this is actually more readable:

my_custom_matcher = be_kind_of(Array).and( include(:foo) ).and( end_with(2) )

...which doesn't require AndMatcher to support more than 2 matchers. What you had before was simpler, I think, so I think I'll change it back.

I see. You didn't actually document that use case with a spec.

Anyhow, I think this is actually more readable:

my_custom_matcher = be_kind_of(Array).and( include(:foo) ).and( end_with(2) )

...which doesn't require AndMatcher to support more than 2 matchers. What you had before was simpler, I think, so I think I'll change it back.

This comment has been minimized.

Show comment
Hide comment
@eloyesp

eloyesp Dec 12, 2013

Owner

ok, the main problem I had is that I needed the evaluated matchers for the failure message:

def failure_message_for_should
  "#{ base_matcher.failure_message } and #{ new_matcher.failure_message }"
  # => error if new_matcher wasn't evaluated.
end

But you may find other way to solve that.

Owner

eloyesp replied Dec 12, 2013

ok, the main problem I had is that I needed the evaluated matchers for the failure message:

def failure_message_for_should
  "#{ base_matcher.failure_message } and #{ new_matcher.failure_message }"
  # => error if new_matcher wasn't evaluated.
end

But you may find other way to solve that.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 11, 2013

Member

@myronmarston Sorry for the delay but the work didn't left me enough time to work on this. I re-based on master and squashed some minor commits to see if that help.

Thanks for the learning experience, for the comments and for the patience.

Thank you for taking the time to follow up! I did have one question about 22e0e05 -- see you my comment there.

Member

myronmarston commented Dec 11, 2013

@myronmarston Sorry for the delay but the work didn't left me enough time to work on this. I re-based on master and squashed some minor commits to see if that help.

Thanks for the learning experience, for the comments and for the patience.

Thank you for taking the time to follow up! I did have one question about 22e0e05 -- see you my comment there.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Dec 13, 2013

Member

I'm going to close this as I've opened a new PR (#387) that works off of this to provide further improvements. We'll be merging that soon. Your feedback is welcome on my further changes, of course.

Member

myronmarston commented Dec 13, 2013

I'm going to close this as I've opened a new PR (#387) that works off of this to provide further improvements. We'll be merging that soon. Your feedback is welcome on my further changes, of course.

myronmarston added a commit that referenced this pull request Dec 16, 2013

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