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

include() doesn't work with ActiveModel::Errors #1367

Open
krainboltgreene opened this issue May 15, 2015 · 14 comments
Open

include() doesn't work with ActiveModel::Errors #1367

krainboltgreene opened this issue May 15, 2015 · 14 comments

Comments

@krainboltgreene
Copy link

it "fails to persist" do
  expect(errors).to include(name: "can't be blank")
end
error: [:name] does not include { :name => "can't be blank" }

The error might not be quite that, but basically that.

@myronmarston
Copy link
Member

What output do you currently get?

@krainboltgreene
Copy link
Author

Currently pairing, will get some output when I can.

@cupakromer
Copy link
Member

The error message is:

     Failure/Error: expect(errors).to include(name: "can't be blank")
       expected #<ActiveModel::Errors:0x007ff541a11088 @base=#<Widget id: nil, name: nil>, @messages={:name => ["can't be blank"]}> to include {:name => "can't be blank"}
       Diff:
       @@ -1,2 +1,2 @@
       -[{:name=>"can't be blank"}]
       +[:name]

The issue is ActiveModel::Errors has a custom implementation of include?:

def include?(attribute)
  messages[attribute].present?
end

Which only checks if the attribute is a key for the messages. Thus the diff says it has [:name].

Also, note that messages is a hash of arrays. So this will work:

expect(errors.messages).to include(name: ["can't be blank"])
expect(errors.messages).to include(name: including("can't be blank"))

@cupakromer
Copy link
Member

The root issue is that the RSpec::Matchers::BuiltIn::Include matcher has special case logic if the actual object is_a?(Hash). ActiveModel::Errors is not a hash:

errors.is_a?(Hash)
# => false

However, it acts as a hash. More specifically, it contains custom implementations for many methods (such as include?) which behave similar to how a Hash instance would behave. ActiveModel::Errors does implement to_hash; the Ruby implicit conversion method for when an object "can be a hash".

@myronmarston Perhaps we could consider improving the matcher to check both is_a?(Hash) || respond_to?(:to_hash) and use the special case hash logic there? (I'm happy to submit a PR this weekend for this)

@myronmarston
Copy link
Member

ActiveModel::Errors does implement to_hash; the Ruby implicit conversion method for when an object "can be a hash".

I thought that to_h was the conversion protocol? When Ruby 2.0 came out, it was announced that was the official conversion method that was going in core:

http://blog.marc-andre.ca/2013/02/23/ruby-2-by-example/#to_h

I've implemented to_hash on objects (before to_h was a thing) but I thought of it just as a convenience and not as one of Ruby's built-in protocols.

Regardless, relying on to_h is a bit problematic; while it works on some arrays, it fails on others:

irb(main):003:0> [[:a, 1], [:b, 2]].to_h
=> {:a=>1, :b=>2}
irb(main):004:0> [:a].to_h
TypeError: wrong element type Symbol at 0 (expected array)
    from (irb):4:in `to_h'
    from (irb):4
    from /Users/myron/.rubies/ruby-2.2.0/bin/irb:11:in `<main>'

Thus we can't assume it'll always work. Given the complexities around coercing to a hash, I think going that direction is potentially problematic. Maybe we can improve the include matcher so that under the following conditions:

  • actual is not a hash
  • :key => value has been given as the expected inclusion
  • actual.include?(:key => value) returns false
  • actual.include?(:key) returns true

...we include a note in the failure message about the fact that the object is not a hash and they may need to convert it to one.

@cupakromer
Copy link
Member

My understanding is there are two protocols:

  • explicit (to_h)

    Returns self. If called on a subclass of Hash, converts the receiver to a Hash object.

  • implicit (to_hash)

    Returns self.

In Ruby 1.8.7 only to_hash exists.

Explicit is for when a thing, can potentially convert to the core type. Implicit is for when a thing, can be the actual core type. I think the main source I got this from is Avdi's "Confident Ruby".

@cupakromer
Copy link
Member

@myronmarston
Copy link
Member

Huh, I never new about that. Sounds like to_hash is the way to go.

@krainboltgreene
Copy link
Author

Yeah that's news to me.

On Sat, May 16, 2015 at 3:59 PM, Myron Marston notifications@github.com
wrote:

Huh, I never new about that. Sounds like to_hash is the way to go.


Reply to this email directly or view it on GitHub
#1367 (comment).

Kurtis Rainbolt-Greene, Hacker
Software Developer
1631 8th St.
New Orleans, LA
70115

@cupakromer
Copy link
Member

Started looking into implementing this. It's a bit trickier than I initially anticipated due to the matcher being diffable.

I have it mostly working. On a failing example the output is:

     Failure/Error: expect(implicit_hash_no_match).to include_key_matcher
       expected #<ImplicitHash:0x007ff8da2fe718 @hash={:a => 15}> to include {:a => (a value within 3 of 10)}
       Diff:
       @@ -1,2 +1,2 @@
       -[{:a=>(a value within 3 of 10)}]
       +#<ImplicitHash:0x007ff8da2fe718 @hash={:a=>15}>

Though I'd prefer it to be:

     Failure/Error: expect(implicit_hash_no_match).to include_key_matcher
       expected #<ImplicitHash:0x007ff8da2fe718 @hash={:a => 15}> to include {:a => (a value within 3 of 10)}
       Diff:
       @@ -1,2 +1,2 @@
       -[{:a=>(a value within 3 of 10)}]
       +:a => 15,

Just debating implementation details I guess.

@pirj
Copy link
Member

pirj commented Aug 1, 2020

Shouldn't it be?

expect(model.errors.messages).to include(name: include("can't be blank"))

@JonRowe
Copy link
Member

JonRowe commented Aug 1, 2020

Its always several messages IIRC

@pirj
Copy link
Member

pirj commented Aug 1, 2020

You're right 👍 Fixed

@JonRowe
Copy link
Member

JonRowe commented Aug 1, 2020

The original issue is because the errors object is hash like, but not a hash, so this is one of our matchers that could (should?) be improved in rspec-rails

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

No branches or pull requests

5 participants