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

Fix: false positive of validate_presence_of #1545

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HeitorMC
Copy link
Contributor

@HeitorMC HeitorMC commented Mar 10, 2023

Referent to #1302

When your model has validate_presence_of with a custom message:

class Article < ApplicationRecord
  validates_presence_of :owner, message: 'this is wrong, it needs the owner'
end

it shouldn't pass, but it does

    it { should_not validate_presence_of(:owner) }

However, this spec spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb is failing in lines 554, 605, and 694.

I was checking with pry-rails and I saw that the matcher is passing through:

But it should pass through:

def failure_message_when_negated # rubocop:disable Metrics/MethodLength

Unfortunately, I have not found out why. @vsppedro @mcmire do you have any idea what might be going on?

validate_presence_of with a should_not matcher shows a false positive when the model has a custom error message
@HeitorMC HeitorMC marked this pull request as ready for review March 27, 2023 18:44
@HeitorMC
Copy link
Contributor Author

Hey, @mcmire @vsppedro any updates on this case?

@mcmire
Copy link
Collaborator

mcmire commented Mar 29, 2023

I have a long response that I tried to write here, but the short answer is that it's intentionally flipped. When used in the positive, the validate_presence_of matcher needs to test that the model does not allow nil; and when used in the negative, it needs to test that it does allow nil. Under the hood, the validate_presence_of matcher uses the allow_value matcher to do this, so this:

it { should_not validate_presence_of(:owner) }

is sort of like saying this:

it { should allow_value(nil).for(:owner) }

That's why it passes through failure_message and not failure_message_when_negated.

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

2 participants