shoulda-matchers icon indicating copy to clipboard operation
shoulda-matchers copied to clipboard

Confusing error message from inclusion_of

Open runephilosof-abtion opened this issue 3 years ago • 7 comments

In the below case, the odd error message appears, when the subject is a new record with the id set to something.

class Low < ApplicationRecord
  has_many :issues
end
class Medium < ApplicationRecord
  has_many :issues
end
class High < ApplicationRecord
  has_many :issues
end
class Issue < ApplicationRecord
  belongs_to :severity, polymorphic: true

  validates_inclusion_of :severity_type,
    in: %w(Low Medium High)
end
RSpec.describe Issue, type: :model do
  subject { Issue.new(severity_id: 1) } # if this is commented out the test is succesful.
  it do
    should validate_inclusion_of(:severity).
      in_array(%w(Low Medium High)).
  end
end

Results in

     Failure/Error:
       should validate_inclusion_of(:severity_type).in_array(%w[Low Medium High])
     
     NameError:
       wrong constant name shoulda-matchers test string

runephilosof-abtion avatar Mar 24 '21 15:03 runephilosof-abtion

Hi @runephilosof-abtion,

severity_type looks to be a "magic" polymorphic column. Therefore, I believe you are getting this error because ActiveRecord (and we) are expecting severity_type to be a reference to a model. What are the possible models that severity could refer to? Perhaps you want to use that set in the array rather than a set of severity levels?

mcmire avatar Mar 24 '21 16:03 mcmire

@mcmire That is exactly what I meant. I have updated the first comment to reflect that. I want to test that the type is only set to the class names in the validation.

runephilosof-abtion avatar Mar 25 '21 11:03 runephilosof-abtion

Hey folks, after updating to rails 7, this error happens in my test suite, is there a workaround to fix this?

AlfonsoUceda avatar Dec 22 '21 12:12 AlfonsoUceda

I'm working on adding support for Rails 7 at the moment. Thank you for sharing this problem with us, I'll take a look ASAP.

vsppedro avatar Dec 22 '21 13:12 vsppedro

We're also seeing the same error in conjunction with the Rails 7 upgrade. I believe that this is because Rails is trying to constantize the passed string to check if it's a valid model, but is running into a invalid name for a constant here: https://github.com/thoughtbot/shoulda-matchers/blob/main/lib/shoulda/matchers/active_model/validate_inclusion_of_matcher.rb#L272

Which explains the cryptic error message.

I'm not sure how to get around that, looks like it's a new behavior of rails to try to constantize this so early?

yboulkaid avatar Dec 28 '21 15:12 yboulkaid

I ran into this testing Rails 7, and was able to track it down to this change: https://github.com/rails/rails/pull/42124

I haven't dug into it much yet, but the first fix that comes to mind is to add some logic rescue NameError if it's a polymorphic type column.

technicalpickles avatar Jan 25 '22 03:01 technicalpickles

The problem still occurs in v5.2.0 :(

krzysztofjablonski avatar Sep 23 '22 14:09 krzysztofjablonski

@krzysztofjablonski, sorry about that. To be honest, I forgot about this error. This issue will be my priority at the moment. Thank you for your report.

vsppedro avatar Sep 28 '22 12:09 vsppedro

We ran into this issue as well when upgrading to Rails 7 as well.

Another way we could get around this is to change ARBITRARY_OUTSIDE_STRING = 'shoulda-matchers test string'.freeze to be a string that refers to a class that we know will exist for all shoulda-matchers users. I tested this out locally by creating a new class inside that file called class ArbitraryShouldaMatchersTestString and then used ArbitraryShouldaMatchersTestString.name (which will return the fully qualified class name) in place of 'shoulda-matchers test string'.

In the case where someone is verifying the inclusion of generic strings in a collection, this is still just an arbitrary string - just a bit less arbitrary than before. But in the case where the column refers to a class name that Rails is expecting to exist, Rails will be happy because the class exists.

christhomson avatar Nov 14 '22 20:11 christhomson