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 all matcher [DONE] #491

Merged
merged 1 commit into from Apr 17, 2014
Merged

Conversation

yelled3
Copy link

@yelled3 yelled3 commented Mar 6, 2014

#483

Changelog

  • Add support for all matcher, to allow you to match matcher on a collection.
    • expect(stoplight.color).to ~eq("blue")
    • expect([1, 3, 5]).to all( be_odd.and be_an(Integer) )
      (Adam Farhi)

as @myronmarston mentioned;
waiting for 3.1 release

#
# expect([3,3,3]).to all.eq(3)
# expect([1,2,3]).to all.eq(3) # fails
# expect([1,2,3]).not_to all.eq(5)
Copy link
Member

@myronmarston myronmarston Mar 6, 2014

Choose a reason for hiding this comment

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

Shouldn't these examples be formatted like all(eq 3) rather than all.eq(3)? The matcher is an argument, not a a chained method.

Also, please put spaces in your arrays: [1, 2, 3], not [1,2,3]. Spaceshelpmakeiteasiertoread.

Copy link
Member

@JonRowe JonRowe Mar 6, 2014

Choose a reason for hiding this comment

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

Yeah these should be methods accepting matchers rather than a fluent interface.

@JonRowe
Copy link
Member

JonRowe commented Mar 6, 2014

Good start! @yelled3

it 'returns the index of the failed object' do
expect {
expect(invalid_collection).to all( eq('A') )
}.to fail_with(dedent <<-EOS)
Copy link
Author

@yelled3 yelled3 Mar 11, 2014

Choose a reason for hiding this comment

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

please ignore this format for now - still not sure what's the optimal error message...
@myronmarston @JonRowe any suggestions?

I was thinking about something like:

Failure/Error: expect(invalid_collection).to all( eq('A') )
       expected ["A", "B", "A", "C", "A"] to all eq "A"

       object at index "1" failed: 
       expected: "A"
            got: "B"

       (compared using ==)


       object at index "3" failed: 
       expected: "A"
            got: "C"

       (compared using ==)

Copy link
Member

@myronmarston myronmarston Mar 11, 2014

Choose a reason for hiding this comment

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

Hmmm. More thought is needed. You mind if we defer this until 3.1? I want to stay focused on the rspec-core changes that are still needed for 3.0.

Copy link
Member

@JonRowe JonRowe Mar 11, 2014

Choose a reason for hiding this comment

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

Yeah I think that's wise, I think that error message is a bit verbose, how about expressing a diff of failing items?

@yelled3
Copy link
Author

yelled3 commented Mar 14, 2014

@JonRowe you're welcome to a final check :-)

@penelopezone
Copy link
Member

penelopezone commented Mar 14, 2014

@yelled3 I like the idea behind this matcher and thanks for your contribution. My issue isn't with the code you've written here, but with the grammar of the matcher and how it reads:

I expect foo to all equal bar doesn't seem like a well formed sentence to me. I have some alternatives but I'm not in love with any of them:

expect(foo).to have_all_values_that(eq(3))
expect(foo).to contain_all_values_that(eq(3))
expect(foo).to all_values_that(eq(3))
expect(foo).to contain_values_that_all(eq(3))

and so on. Does anyone have thoughts around this?

@yelled3
Copy link
Author

yelled3 commented Mar 15, 2014

Hi @samphippen - I see no harm in adding aliases to all;

I don't think the word values is suitable here. collections have objects of some kind. values is very specific...
how about:

expect(items).to all(be_active)
expect(items).to all(be_active).items # add chaining ,like - have(3).items

expect.all(items).to be_active # not sure if possible, but very readable
expect(array).to have_only_objects_that(eq(3)) # too long? :-)

#
# expect([3, 3, 3]).to all.eq(3)
# expect([1, 2, 3]).to all.eq(3) # fails
# expect([1, 2, 3]).not_to all.eq(5)
Copy link
Member

@myronmarston myronmarston Mar 15, 2014

Choose a reason for hiding this comment

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

These examples are wrong; you need to pass eq as an arg, not chain it.

Copy link
Member

@myronmarston myronmarston Mar 15, 2014

Choose a reason for hiding this comment

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

Also, this is kind of a lame example; maybe this is a bit better?

expect([1, 3, 5]).to all be_odd

@myronmarston
Copy link
Member

myronmarston commented Mar 15, 2014

@samphippen -- I think all reads better than any of your suggestions. It actually doesn't really bother me.

@yelled3 -- this'll need a cuke, as well.

improve_hash_formatting "all#{to_sentence(described_items)}"
end

private
Copy link
Member

@JonRowe JonRowe Mar 15, 2014

Choose a reason for hiding this comment

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

This should be dedented by 2 spaces, we aline our privates with class/end

message.start_with?("\n") ? message : "\n#{message}"
end

def initialize_copy(other)
Copy link
Author

@yelled3 yelled3 Apr 8, 2014

Choose a reason for hiding this comment

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

@myronmarston added clone support + specs

@yelled3
Copy link
Author

yelled3 commented Apr 8, 2014

@myronmarston @JonRowe getting some sporadic CI failures:

Installing ffi (1.9.3)
Gem::RemoteFetcher::FetchError: Errno::ETIMEDOUT: Connection timed out - connect(2) for "rubygems.org" port 443 (https://rubygems.org/gems/childprocess-0.5.2.gem)
An error occurred while installing childprocess (0.5.2), and Bundler cannot
continue.
Make sure that `gem install childprocess -v '0.5.2'` succeeds before bundling.
The command "script/run_build" exited with 5.

https://travis-ci.org/rspec/rspec-expectations/jobs/22533536

any idea?

@yelled3
Copy link
Author

yelled3 commented Apr 8, 2014

Aside from CI issues (see above), and squashing to a single commit, I think this is ready for a final review.
@myronmarston @JonRowe @samphippen /cc

@JonRowe
Copy link
Member

JonRowe commented Apr 9, 2014

I rebooted the build ;)

def match(_, actual)
index_failed_objects
actual.all?{ |actual_item| matcher.matches?(actual_item) }
end
Copy link
Member

@myronmarston myronmarston Apr 9, 2014

Choose a reason for hiding this comment

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

This implementation loops over the actual list twice (once to index, once to see if the it matches all items) and the matcher is matched against every item twice. There's no guarantee that matcher.matches? will be a fast operation, so it's best to avoid matching multiple times. I think you can implement this in one pass over the list like so:

def match(_, actual)
  index_failed_objects
  failed_objects.empty?
end

Copy link
Author

@yelled3 yelled3 Apr 9, 2014

Choose a reason for hiding this comment

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

good point 👍
although slightly less readable, then

actual.all?{ |actual_item| matcher.matches?(actual_item) }

describe :description do
it 'provides a description' do
matcher = all(inner_matcher)
matcher.matches?(collection)
Copy link
Member

@myronmarston myronmarston Apr 9, 2014

Choose a reason for hiding this comment

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

Is it actually necessary to call this before checking the description?

Copy link
Author

@yelled3 yelled3 Apr 9, 2014

Choose a reason for hiding this comment

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

nope :-)

@yelled3
Copy link
Author

yelled3 commented Apr 9, 2014

@JonRowe thanks for helping with the CI :-)

def index_failed_objects
actual.each_with_index do |actual_item, index|
unless values_match?(matcher, actual_item)
matcher.matches?(actual_item) # this is needed in order to generate the 'matcher.failure_message'
Copy link
Author

@yelled3 yelled3 Apr 9, 2014

Choose a reason for hiding this comment

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

@myronmarston this the issue I mentioned:
#491 (comment)

Copy link
Member

@myronmarston myronmarston Apr 9, 2014

Choose a reason for hiding this comment

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

Right...that wasn't thinking about the need for the message. I think we should switch back to matchers.matches? to get the message and only have it called once. However, once you do that, it's important that you clone the matcher yourself before calling matches? on it to avoid bugs with matchers that use internal memoization based on the actual value. It'd be good to have a failing spec for that before you fix it -- you can see an example of a spec like that in composable_spec.rb.

Copy link
Member

@myronmarston myronmarston Apr 9, 2014

Choose a reason for hiding this comment

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

Also, it'd be good to have a spec that forces you to use clone rather than dup by passing all a DSL-defined custom matcher. If we were going to use values_match? we wouldn't need these specs as that handles it internally but given we have to handle those cases here we should have specs to enforce that.

Copy link
Author

@yelled3 yelled3 Apr 10, 2014

Choose a reason for hiding this comment

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

yelled3 pushed a commit to yelled3/rspec-expectations that referenced this issue Apr 9, 2014
@yelled3
Copy link
Author

yelled3 commented Apr 9, 2014

@myronmarston @JonRowe all squashed :-)
hope it didn't remove any of the comments.

end
end

include_examples "making a copy", [:clone, :dup]
Copy link
Member

@myronmarston myronmarston Apr 9, 2014

Choose a reason for hiding this comment

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

It seems silly to make a shared example group, have it support an internal loop and then only include it once. I'd rather make it not support an internal loop and than use it twice (once with a :clone arg, once with a :dup arg).

Copy link
Author

@yelled3 yelled3 Apr 10, 2014

Choose a reason for hiding this comment

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

sure, np

@myronmarston
Copy link
Member

myronmarston commented Apr 9, 2014

you're correct - I did this purely for readability.

object at index "3" failed to match:
vs

object at index 3 failed to match:
would you prefer another message format?

I prefer the raw integer (3), as I said.

agreed, changed to:

expect([1, 3, 22]).to all( be_odd.or be < 10 )

This is confusing -- the expectation doesn't pass (22 is neither odd nor less than 10). Maybe change 3 to 4 and 22 to 21? Then it'll pass.

@myronmarston
Copy link
Member

myronmarston commented Apr 9, 2014

could you please explain?
the example you gave have the same amount of blank lines.

When I wrote the comment the spec showed 2 blank lines between (compared using ==) and object at index "3" failed to match:. Looks like you've fixed it now, though.

EOS
end

it 'returns the index of the failed object' do
Copy link
Member

@myronmarston myronmarston Apr 9, 2014

Choose a reason for hiding this comment

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

This doc string is the same as above. I'd change it to it "returns the indexes of all failed objects, not just the first one" (if your implementation used all?, for example, it would stop after the first failure).

@yelled3
Copy link
Author

yelled3 commented Apr 10, 2014

@myronmarston take a look at the last batch of fixes

include_examples 'making a copy', :clone
include_examples 'making a copy', :dup

RSpec::Matchers.define :have_string_length do |expected|
Copy link
Author

@yelled3 yelled3 Apr 10, 2014

Choose a reason for hiding this comment

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

@yelled3
Copy link
Author

yelled3 commented Apr 17, 2014

@myronmarston

  • removed the ~ message
  • updated changelog
  • all squashed

myronmarston added a commit that referenced this issue Apr 17, 2014
@myronmarston myronmarston merged commit 43ffa79 into rspec:master Apr 17, 2014
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.

None yet

4 participants