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

New matcher: have_attributes #571

Merged
merged 1 commit into from Jun 24, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions Changelog.md
Expand Up @@ -27,6 +27,15 @@ Bug Fixes:
expecatations) weren't being used. (Myron Marston, #566)
* Structs are no longer treated as arrays when diffed. (Jon Rowe, #576)

Enhancements:

* Add `have_attributes` matcher, that passes if actual's attribute
values match the expected attributes hash:
`Person = Struct.new(:name, :age)`
`person = Person.new("Bob", 32)`
`expect(person).to have_attributes(:name => "Bob", :age => 32)`.
(Adam Farhi)

### 3.0.0 / 2014-06-01
[Full Changelog](http://github.com/rspec/rspec-expectations/compare/v3.0.0.rc1...v3.0.0)

Expand Down
47 changes: 47 additions & 0 deletions features/built_in_matchers/have_attributes.feature
@@ -0,0 +1,47 @@
Feature: have_attributes matcher

Use the have_attributes matcher to specify that
an object's attributes match the expected attributes:

```ruby
Person = Struct.new(:name, :age)
person = Person.new("Jim", 32)

expect(person).to have_attributes(:name => "Jim", :age => 32)
expect(person).to have_attributes(:name => a_string_starting_with("J"), :age => (a_value > 30) )
```

The matcher will fail if actual doesn't respond to any of the expected attributes:

```ruby
expect(person).to have_attributes(:name => "Jim", :color => 'red')
```

Scenario: basic usage
Given a file named "basic_have_attributes_matcher_spec.rb" with:
"""ruby
Person = Struct.new(:name, :age)

RSpec.describe Person.new("Jim", 32) do
it { is_expected.to have_attributes(:name => "Jim") }
it { is_expected.to have_attributes(:name => a_string_starting_with("J") ) }
it { is_expected.to have_attributes(:age => 32) }
it { is_expected.to have_attributes(:age => (a_value > 30) ) }
it { is_expected.to have_attributes(:name => "Jim", :age => 32) }
it { is_expected.to have_attributes(:name => a_string_starting_with("J"), :age => (a_value > 30) ) }
it { is_expected.not_to have_attributes(:name => "Bob") }
it { is_expected.not_to have_attributes(:age => 10) }
it { is_expected.not_to have_attributes(:age => (a_value < 30) ) }

# deliberate failures
it { is_expected.to have_attributes(:name => "Bob") }
it { is_expected.to have_attributes(:age => 10) }

# fails if any of the attributes don't match
it { is_expected.to have_attributes(:name => "Bob", :age => 32) }
it { is_expected.to have_attributes(:name => "Jim", :age => 10) }
it { is_expected.to have_attributes(:name => "Bob", :age => 10) }
end
"""
When I run `rspec basic_have_attributes_matcher_spec.rb`
Then the output should contain "14 examples, 5 failures"
24 changes: 24 additions & 0 deletions lib/rspec/matchers.rb
Expand Up @@ -560,6 +560,30 @@ def exist(*args)
alias_matcher :an_object_existing, :exist
alias_matcher :existing, :exist

# Passes if actual's attribute values match the expected attributes hash.
# This works no matter how you define your attribute readers.
#
# @example
#
# Person = Struct.new(:name, :age)
# person = Person.new("Bob", 32)
#
# expect(person).to have_attributes(:name => "Bob", :age => 32)
# expect(person).to have_attributes(:name => a_string_starting_with("B"), :age => (a_value > 30) )
#
# @note It will fail if actual doesn't respond to any of the expected attributes.
#
# @example
#
# expect(person).to have_attributes(:color => "red")
#
# rubocop:disable Style/PredicateName
Copy link
Member

Choose a reason for hiding this comment

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

I think you should put # rubocop:enable Style/PredicateName after the method so it is turned back on. @yujinakayama -- can you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. If the line contains only the annotation comment, the disablement of cop continues until next # rubocop:enable Style/PredicateName appears. On the other hand, if the comment is put after any token (as I suggested), it affects only the line. You can check the range of disablement by runnning rubocop --format progress --format disabled lib.

def have_attributes(expected)
Copy link
Member

Choose a reason for hiding this comment

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

You can disable the offense report by RuboCop by adding an annotation comment as def have_attributes(expected) # rubocop:disable Style/PredicateName.

Copy link
Author

Choose a reason for hiding this comment

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

thanks @yujinakayama !

BuiltIn::HaveAttributes.new(expected)
end
Copy link
Member

Choose a reason for hiding this comment

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

We should add an aliased form of this matcher for use composed expressions:

expect { |probe|
  foo(&probe)
}.to yield_with_args(an_object_having_attributes(:name => "Jane", :age => 40))

I'm not sure what the best phrasing for the alias is, though. Most of our other aliases use the -ing form (which is why an_object_having_attributes is what I used above). Another possibility is an_object_with_attributes, which reads more naturally (to me, anyway) but is less consistent with the naming convention of the other aliases.

What do others think?

/cc @samphippen @xaviershay @JonRowe @soulcutter @cupakromer

Copy link
Author

Choose a reason for hiding this comment

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

@myronmarston what about:

  • an_object_with_attributes + single form * an_object_with_an_attribute
  • with_attributes + single form * with_an_attribute

Copy link
Member

Choose a reason for hiding this comment

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

@yelled3 -- I'm not following...can you show example specs of what you are proposing?

Copy link
Author

Choose a reason for hiding this comment

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

i was simply suggesting possible aliases:

expect(person).to an_object_with_attributes(:name => "Myron", :age => 32)
expect(person).to an_object_with_attribute(:name => "Myron")

expect { |probe|
  foo(&probe)
}.to yield_with_args(an_object_having_attribute(:age => (a_value > 18)))


expect(person).to with_attributes(:name => "Myron", :age => 32)
expect(person).to with_an_attribute(:name => "Myron")

expect { |probe|
  foo(&probe)
}.to yield_with_args(with_attributes(:name => "Jane", :age => 40))

Copy link
Member

Choose a reason for hiding this comment

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

I don't think with_attributes on its own makes sense. Either an_object_with_attributes or an_object_having_attributes make sense to me -- which we pick depends on if we favor consistency or more natural phrasing.

@JonRowe, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think with_attributes on its own makes sense.

Agree, but just attributes(...) would but that's probably too generic, I have a minor preference for an_object_with_attributes(..) over an_object_having_attributes(..) but I'm ok with the latter for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

which we pick depends on if we favor consistency or more natural phrasing.

if consistency means doing:

alias_matcher :an_object_ORIG_NAME, :ORIG_NAME

than by looking at other alias_matcher definitions, it seem like natural phrasing is mostly favored:

alias_matcher :an_object_existing, :exist
alias_matcher :an_object_matching, :match
alias_matcher :an_object_responding_to, :respond_to
alias_matcher :an_object_satisfying, :satisfy

so I vote for an_object_with_attributes

Copy link
Member

Choose a reason for hiding this comment

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

if consistency means doing:

That's not what I mean by consistency. (In fact, I don't think we have any matcher aliases that use that form).

I mean that most of our matcher aliases use an -ing form:

➜  rspec-expectations git:(3-0-maintenance) ack "alias_matcher.*ing" lib/rspec/matchers.rb
    alias_matcher :a_block_changing,  :change
    alias_matcher :changing,          :change
    alias_matcher :a_collection_containing_exactly, :contain_exactly
    alias_matcher :containing_exactly,              :contain_exactly
    alias_matcher :a_range_covering, :cover
    alias_matcher :covering,         :cover
    alias_matcher :a_collection_ending_with, :end_with
    alias_matcher :a_string_ending_with,     :end_with
    alias_matcher :ending_with,              :end_with
    alias_matcher :an_object_existing, :exist
    alias_matcher :existing,           :exist
    alias_matcher :a_collection_including, :include
    alias_matcher :a_string_including,     :include
    alias_matcher :a_hash_including,       :include
    alias_matcher :including,              :include
    alias_matcher :an_object_matching, :match
    alias_matcher :a_string_matching,  :match
    alias_matcher :matching,           :match
    alias_matcher :a_block_outputting, :output
    alias_matcher :a_block_raising,  :raise_error do |desc|
    alias_matcher :raising,        :raise_error do |desc|
    alias_matcher :an_object_responding_to, :respond_to
    alias_matcher :responding_to,           :respond_to
    alias_matcher :an_object_satisfying, :satisfy
    alias_matcher :satisfying,           :satisfy
    alias_matcher :a_collection_starting_with, :start_with
    alias_matcher :a_string_starting_with,     :start_with
    alias_matcher :starting_with,              :start_with
    alias_matcher :a_block_throwing, :throw_symbol do |desc|
    alias_matcher :throwing,        :throw_symbol do |desc|
    alias_matcher :a_block_yielding_control,  :yield_control
    alias_matcher :yielding_control,          :yield_control
    alias_matcher :a_block_yielding_with_no_args,  :yield_with_no_args
    alias_matcher :yielding_with_no_args,          :yield_with_no_args
    alias_matcher :a_block_yielding_with_args,  :yield_with_args
    alias_matcher :yielding_with_args,          :yield_with_args
    alias_matcher :a_block_yielding_successive_args,  :yield_successive_args
    alias_matcher :yielding_successive_args,          :yield_successive_args

It's easier to guess what the matcher alias will be when we stick with a common convention like this.

than by looking at other alias_matcher definitions, it seem like natural phrasing is mostly favored:

Those are all examples of -ing phrasing. They do read pretty naturally, but there are other phrasings that are pretty natural, too:

  • an_object_that_matches("foo") rather than an_object_matching("foo")
  • an_object_that_responds_to(:foo) rather than an_object_responding_to(:foo)
  • etc.

I think I'm leaning towards an_object_having_attributes for consistency.

Thoughts from @samphippen @soulcutter @cupakromer @xaviershay @yujinakayama ?

Copy link
Author

Choose a reason for hiding this comment

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

OK.
are there any other issues?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, I think the alias is the last thing needed.

# rubocop:enable Style/PredicateName
alias_matcher :an_object_having_attributes, :have_attributes

# Passes if actual includes expected. This works for
# collections and Strings. You can also pass in multiple args
# and it will only pass if all args are found in collection.
Expand Down
1 change: 1 addition & 0 deletions lib/rspec/matchers/built_in.rb
Expand Up @@ -30,6 +30,7 @@ module BuiltIn
autoload :Equal, 'rspec/matchers/built_in/equal'
autoload :Exist, 'rspec/matchers/built_in/exist'
autoload :Has, 'rspec/matchers/built_in/has'
autoload :HaveAttributes, 'rspec/matchers/built_in/have_attributes'
autoload :Include, 'rspec/matchers/built_in/include'
autoload :All, 'rspec/matchers/built_in/all'
autoload :Match, 'rspec/matchers/built_in/match'
Expand Down
84 changes: 84 additions & 0 deletions lib/rspec/matchers/built_in/have_attributes.rb
@@ -0,0 +1,84 @@
module RSpec
module Matchers
module BuiltIn
# @api private
# Provides the implementation for `have_attributes`.
# Not intended to be instantiated directly.
class HaveAttributes < BaseMatcher
# @private
attr_reader :respond_to_failed

def initialize(expected)
@expected = expected
@respond_to_failed = false
end

# @api private
# @return [Boolean]
def matches?(actual)
@actual = actual
return false unless respond_to_attributes?
perform_match(:all?)
end

# @api private
# @return [Boolean]
def does_not_match?(actual)
@actual = actual
return false unless respond_to_attributes?
perform_match(:none?)
end

# @api private
# @return [String]
def description
described_items = surface_descriptions_in(expected)
improve_hash_formatting "have attributes #{described_items.inspect}"
end

# @api private
# @return [String]
def failure_message
respond_to_failure_message_or { super }
end

# @api private
# @return [String]
def failure_message_when_negated
respond_to_failure_message_or { super }
end
Copy link
Member

Choose a reason for hiding this comment

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

I noticed in the build that the diff is odd:

https://travis-ci.org/rspec/rspec-expectations/jobs/27226588#L275

If you have an idea for how to fix that, feel free -- otherwise, my advice is to make it not diffable for now.

Copy link
Member

Choose a reason for hiding this comment

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

It's odd because Structs are enumerable and the differ is interpreting it as an array.

Copy link
Author

Choose a reason for hiding this comment

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

solved it, by doing:

module Composable

   def surface_descriptions_in(item)
     #...
     elsif Struct === item
         Hash[surface_descriptions_in(item.each_pair.to_a)]

WDYT? @myronmarston @JonRowe /cc

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of special-casing Struct. Our examples here use a struct for simplicity but any kind of ruby object can be used. It'll be confusing when other types of objects are used. Thinking about this more broadly, the actual object is any arbitrary ruby object, while the expected is provided as a hash. I don't think it makes sense to do a textual diff of an arbitrary ruby object against a hash. Let's make the matcher not diffable for now. We can always revisit.

Copy link
Author

Choose a reason for hiding this comment

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

sounds good.

I thought, that was a specific issue with Struct...
surface_descriptions_in isn't tested enough (from what I've seen)
we should have a unit test for each kind of type.
see: https://github.com/rspec/rspec-expectations/blob/master/spec/rspec/matchers/composable_spec.rb

we can deal with surface_descriptions_in in another PR :-)

Copy link
Member

Choose a reason for hiding this comment

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

(See #576)

Copy link
Author

Choose a reason for hiding this comment

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

that was fast :-)
thanks @JonRowe

Copy link
Member

Choose a reason for hiding this comment

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

This and failure_message have duplicated logic. How about something like this?

def failure_message
  respond_to_failure_message_or { super }
end

def failure_message_when_negated
  respond_to_failure_message_or { super }
end

private

def respond_to_failure_message_or
  if respond_to_failed
    respond_to_matcher.failure_message
  else
    improve_hash_formatting(yield)
  end
end

Copy link
Author

Choose a reason for hiding this comment

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

done :-)


private

def perform_match(predicate)
expected.__send__(predicate) do |attribute_key, attribute_value|
actual_has_attribute?(attribute_key, attribute_value)
end
end

def actual_has_attribute?(attribute_key, attribute_value)
actual_value = actual.__send__(attribute_key)
values_match?(attribute_value, actual_value)
end

def respond_to_attributes?
matches = respond_to_matcher.matches?(actual)
@respond_to_failed = !matches
matches
end

def respond_to_matcher
@respond_to_matcher ||= RespondTo.new(*expected.keys).with(0).arguments
end

def respond_to_failure_message_or
if respond_to_failed
respond_to_matcher.failure_message
else
improve_hash_formatting(yield)
end
end
end
end
end
end
8 changes: 8 additions & 0 deletions spec/rspec/matchers/aliases_spec.rb
Expand Up @@ -212,6 +212,14 @@ module RSpec
expect(existing).to be_aliased_to(exist).with_description("existing")
end

specify do
expect(
an_object_having_attributes(:age => 32)
).to be_aliased_to(
have_attributes(:age => 32)
).with_description("an object having attributes {:age => 32}")
end
Copy link
Author

Choose a reason for hiding this comment

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

@myronmarston added the alias


specify do
expect(
a_string_including("a")
Expand Down
177 changes: 177 additions & 0 deletions spec/rspec/matchers/built_in/have_attributes_spec.rb
@@ -0,0 +1,177 @@
RSpec.describe "#have_attributes matcher" do

Person = Struct.new(:name, :age)

class Person
def parent(parent_name)
@parent = parent_name
end
end

let(:wrong_name) { "Wrong Name" }
let(:wrong_age) { 11 }

let(:correct_name) { "Correct name" }
let(:correct_age) { 33 }

let(:person) { Person.new(correct_name, correct_age) }

it "is not diffable" do
expect(have_attributes(:age => correct_age)).to_not be_diffable
end

describe "expect(...).to have_attributes(with_one_attribute)" do

it_behaves_like "an RSpec matcher", :valid_value => Person.new("Correct name", 33), :invalid_value => Person.new("Wrong Name", 11) do
let(:matcher) { have_attributes(:name => "Correct name") }
end

it "passes if target has the provided attributes" do
expect(person).to have_attributes(:name => correct_name)
end

it "fails if target does not have any of the expected attributes" do
expect {
expect(person).to have_attributes(:name => wrong_name)
}.to fail_matching(%r|expected #{object_inspect person} to have attributes #{hash_inspect :name => wrong_name}|)
end

it "fails if target does not responds to any of the attributes" do
expect {
expect(person).to have_attributes(:color => 'red')
}.to fail_matching("expected #{object_inspect person} to respond to :color")
end

it "fails if target responds to the attribute but requires arguments" do
Copy link
Author

Choose a reason for hiding this comment

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

@myronmarston

I don't see a spec for the case where the object responds to the attribute method but requires arguments (I noticed you addressed that in the implementation, though). Can you add a spec for that?

see bellow, for all uses cases

expect {
expect(person).to have_attributes(:parent => 'Billy')
}.to fail_matching("expected #{object_inspect person} to respond to :parent with 0 arguments")
end

describe "expect(...).to have_attributes(key => matcher)" do

it "passes when the matchers match" do
expect(person).to have_attributes(:age => (a_value > 30))
end

it 'provides a description' do
description = have_attributes(:age => (a_value > 30)).description
expect(description).to eq("have attributes {:age => (a value > 30)}")
end

it "fails with a clear message when the matcher does not match" do
expect {
expect(person).to have_attributes(:age => (a_value < 10))
}.to fail_matching("expected #{object_inspect person} to have attributes {:age => (a value < 10)}")
end
end
end

describe "expect(...).to_not have_attributes(with_one_attribute)" do

it "passes if target does not have any of the expected attributes" do
expect(person).to_not have_attributes(:age => wrong_age)
end

it "fails if target has all of the expected attributes" do
expect {
expect(person).to_not have_attributes(:age => correct_age)
}.to fail_matching(%r|expected #{object_inspect person} not to have attributes #{hash_inspect :age => correct_age}|)
end

it "fails if target does not responds to any of the attributes" do
expect {
expect(person).to_not have_attributes(:color => 'red')
}.to fail_matching("expected #{object_inspect person} to respond to :color")
end

it "fails if target responds to the attribute but requires arguments" do
expect {
expect(person).to_not have_attributes(:parent => 'Billy')
}.to fail_matching("expected #{object_inspect person} to respond to :parent with 0 arguments")
end
end

describe "expect(...).to have_attributes(with_multiple_attributes)" do

it_behaves_like "an RSpec matcher", :valid_value => Person.new("Correct name", 33), :invalid_value => Person.new("Wrong Name", 11) do
let(:matcher) { have_attributes(:name => "Correct name", :age => 33) }
end

it "passes if target has the provided attributes" do
expect(person).to have_attributes(:name => correct_name, :age => correct_age)
end

it "fails if target does not have any of the expected attributes" do
expect {
expect(person).to have_attributes(:name => correct_name, :age => wrong_age)
}.to fail_matching(%r|expected #{object_inspect person} to have attributes #{hash_inspect :name => correct_name, :age => wrong_age}|)
end

it "fails if target does not responds to any of the attributes" do
expect {
expect(person).to have_attributes(:name => correct_name, :color => 'red')
}.to fail_matching("expected #{object_inspect person} to respond to :color")
end

it "fails if target responds to the attribute but requires arguments" do
expect {
expect(person).to have_attributes(:name => correct_name, :parent => 'Billy')
}.to fail_matching("expected #{object_inspect person} to respond to :parent with 0 arguments")
end
end

describe "expect(...).to_not have_attributes(with_multiple_attributes)" do

it "passes if target has none of the expected attributes" do
expect(person).to_not have_attributes(:name => wrong_name, :age => wrong_age)
end

it "fails if target has any of the expected attributes" do
expect {
expect(person).to_not have_attributes(:name => wrong_name, :age => correct_age)
}.to fail_matching(%r|expected #{object_inspect person} not to have attributes #{hash_inspect :name => wrong_name, :age => correct_age}|)
end

it "fails if target has all of the expected attributes" do
expect {
expect(person).to_not have_attributes(:name => correct_name, :age => correct_age)
}.to fail_matching(%r|expected #{object_inspect person} not to have attributes #{hash_inspect :name => correct_name, :age => correct_age}|)
end

it "fails if target does not responds to any of the attributes" do
expect {
expect(person).to_not have_attributes(:name => correct_name, :color => 'red')
}.to fail_matching("expected #{object_inspect person} to respond to :color")
end

it "fails if target responds to the attribute but requires arguments" do
expect {
expect(person).to_not have_attributes(:name => correct_name, :parent => 'Billy')
}.to fail_matching("expected #{object_inspect person} to respond to :parent with 0 arguments")
end
end


include RSpec::Matchers::Pretty
# We have to use Hash#inspect in examples that have multi-entry
# hashes because the #inspect output on 1.8.7 is non-deterministic
# due to the fact that hashes are not ordered. So we can't simply
# put a literal string for what we expect because it varies.
if RUBY_VERSION.to_f == 1.8
def hash_inspect(hash)
/\{(#{hash.map { |key,value| "#{key.inspect} => #{value.inspect}.*" }.join "|" }){#{hash.size}}\}/
end
else
def hash_inspect(hash)
improve_hash_formatting hash.inspect
end
end

include RSpec::Matchers::Composable
# a helper for failure message assertion
def object_inspect(object)
surface_descriptions_in object.inspect
end

end