Fix negative versions of validation matchers #1136

mcmire · 2018-09-12T05:30:49Z

NOTE: This branch incorporates changes from #1133, #1134, and #1135 so that development is easier and all tests pass. When those PRs are merged I'll rebase the branch.

@guialbuk I know this diff is large. I don't expect you to read the whole thing, but I really just wanted to make sure you were aware of the fix here and whether my explanation made sense.

When using a validation matcher in the negative, i.e.:

should_not validate_*(...)

as opposed to:

should validate_*(...)

...it's common to receive the following error:

undefined method `attribute_setter' for nil:NilClass

This happens particularly when using a matcher that makes use of
AllowValueMatcher or DisallowValueMatcher internally (which all of the
validation matchers do).

Whenever you make an assertion by using a matcher in the negative as
opposed to the positive, RSpec still calls the matches? method for
that matcher; however, the assertion will pass if that returns false
as opposed to true. In other words, it just inverts the result.

However, whenever we are using AllowValueMatcher or
DisallowValueMatcher, it doesn't really work to invert the result. like
this. This is because AllowValueMatcher and DisallowValueMatcher,
despite their name, aren't truly opposites of each other.

AllowValueMatcher performs these steps:

Set the attribute on the record to some value
Run validations on the record
Ask whether validations pass or fail
If validations fail, store the value that caused the failure along
with the validation errors and return false
Otherwise, return true

However, DisallowValueMatcher performs these steps:

Set the attribute on the record to some value
Run validations on the record
Ask whether validations pass or fail
If validations pass, store the value that caused the failure along
with some metadata and return false
Otherwise, return true

This difference in logic is achieved by having AllowValueMatcher
implement does_not_match? and then having DisallowValueMatcher use
this for its positive case and use matches? for its negative case.
It's easy to see because of this that does_not_match? is not the same
as !matches? and vice versa.

So a matcher that makes use of these submatchers internally needs to use
their opposite versions whenever that matcher is used in the negative
case. In other words, all of the matchers need a does_not_match? which
is like matches?, except that all of the logic is inverted, and in all
the cases in which AllowValueMatcher is used, DisallowValueMatcher needs
to be used.

Doing this ensures that when failure_message is called on
AllowValueMatcher or DisallowValueMatcher, step 4 in the list of steps
above stores a proper value that can then be referenced in the failure
message for the validation matcher itself.

References #904
Fixes #963
Fixes #931
Fixes #1075

The inclusion matcher, when qualified with `in_array`, was using AllowValueMatcher to check that values outside the array were disallowed by the model (and then inverting its result). However, it should have been using DisallowValueMatcher all this time. This commit fixes that. Without this fix, the following error is raised when using the inclusion matcher against a model which does not have the proper inclusion validation on it: undefined method `attribute_setter' for nil:NilClass This happens because the inclusion matcher is a complex matcher, i.e., it runs a series of submatchers internally and the result of those submatchers contributes to whether or not the matcher matches. If one of those submatchers fails, the inclusion matcher immediately fails and spits out the failure message associated with that submatcher. However, there is a fundamental difference between AllowValueMatcher and DisallowValueMatcher as it relates to how they function: * AllowValueMatcher sets an attribute to a value on a record and expects the record not to fail validation. * DisallowValueMatcher sets an attribute to a value on a record, but expects the record *to* fail validation. The issue in this case is that, because AllowValueMatcher was used instead of DisallowValueMatcher, the inclusion matcher thought that the AllowValueMatcher failed, when in fact it passed (this result was just inverted). So it tried to generate a failure message for a matcher that didn't fail in the first place. By using DisallowValueMatcher, we set the proper expectations.

When using a validation matcher in the negative, i.e.: should_not validate_*(...) as opposed to: should validate_*(...) ...it's common to receive the following error: undefined method `attribute_setter' for nil:NilClass This happens particularly when using a matcher that makes use of AllowValueMatcher or DisallowValueMatcher internally (which all of the validation matchers do). Whenever you make an assertion by using a matcher in the negative as opposed to the positive, RSpec still calls the `matches?` method for that matcher; however, the assertion will pass if that returns *false* as opposed to true. In other words, it just inverts the result. However, whenever we are using AllowValueMatcher or DisallowValueMatcher, it doesn't really work to invert the result. like this. This is because AllowValueMatcher and DisallowValueMatcher, despite their name, aren't truly opposites of each other. AllowValueMatcher performs these steps: 1. Set the attribute on the record to some value 2. Run validations on the record 3. Ask whether validations pass or fail 4. If validations fail, store the value that caused the failure along with the validation errors and return false 5. Otherwise, return true However, DisallowValueMatcher performs these steps: 1. Set the attribute on the record to some value 2. Run validations on the record 3. Ask whether validations pass or fail 4. If validations *pass*, store the value that caused the failure along with some metadata and return false 5. Otherwise, return true This difference in logic is achieved by having AllowValueMatcher implement `does_not_match?` and then having DisallowValueMatcher use this for its positive case and use `matches?` for its negative case. It's easy to see because of this that `does_not_match?` is not the same as `!matches?` and vice versa. So a matcher that makes use of these submatchers internally needs to use their opposite versions whenever that matcher is used in the negative case. In other words, all of the matchers need a `does_not_match?` which is like `matches?`, except that all of the logic is inverted, and in all the cases in which AllowValueMatcher is used, DisallowValueMatcher needs to be used. Doing this ensures that when `failure_message` is called on AllowValueMatcher or DisallowValueMatcher, step 4 in the list of steps above stores a proper value that can then be referenced in the failure message for the validation matcher itself.

houndci-bot · 2018-09-12T05:31:43Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1659,7 +1686,8 @@ def set_attr!; self.attr = 5 end
          attribute_name: :attr,
          changing_values_with: :numeric_value,
          expected_message: <<-MESSAGE.strip
-Example did not properly validate that :attr looks like a number.
+Expected Example to validate that :attr looks like a number, but this


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

So this won't work as we are still testing against Ruby 2.2 and it doesn't support <<~. I've updated the Rubocop configuration in #1137 to address this.

houndci-bot · 2018-09-12T05:31:43Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1578,8 +1605,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an integer
-greater than 18 and less than 99.
+Expected Example to validate that :attr looks like an integer greater


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an integer
-greater than 18 and less than 99.
+Expected Example to validate that :attr looks like an integer greater


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1516,8 +1544,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an odd number
-less than or equal to 99.
+Expected Example to validate that :attr looks like an odd number less


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1492,8 +1520,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an odd number
-less than or equal to 99.
+Expected Example to validate that :attr looks like an odd number less


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1306,8 +1336,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an even number
-greater than 18.
+Expected Example to validate that :attr looks like an even number


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1282,8 +1312,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an even number
-greater than 18.
+Expected Example to validate that :attr looks like an even number


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1254,8 +1284,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE.strip_heredoc
-Example did not properly validate that :attr looks like an integer
-greater than 18.
+Expected Example to validate that :attr looks like an integer greater


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1229,8 +1259,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr looks like an integer
-greater than 18.
+Expected Example to validate that :attr looks like an integer greater


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:31:44Z

spec/unit/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb

@@ -1192,7 +1221,8 @@ def configure_validation_matcher(matcher)
      end

      message = <<-MESSAGE
-Example did not properly validate that :attr looks like a number.
+Expected Example to validate that :attr looks like a number, but this


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

[skip ci]

houndci-bot · 2018-09-12T05:38:11Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -1363,7 +1397,8 @@ def name=(name)
          end

          message = <<-MESSAGE.strip
-Example did not properly validate that :name is case-sensitively unique.
+Expected Example to validate that :name is case-sensitively unique, but


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:11Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -1336,7 +1369,8 @@ def configure_validation_matcher(matcher)
      end

      message = <<-MESSAGE.strip
-Example did not properly validate that :attr is case-sensitively unique.
+Expected Example to validate that :attr is case-sensitively unique, but


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:11Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -1256,8 +1289,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr is case-sensitively unique,
-but only if it is not blank.
+Expected Example to validate that :attr is case-sensitively unique as


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -1230,8 +1263,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr is case-sensitively unique,
-but only if it is not blank.
+Expected Example to validate that :attr is case-sensitively unique as


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -1204,8 +1237,8 @@ def configure_validation_matcher(matcher)
        end

        message = <<-MESSAGE
-Example did not properly validate that :attr is case-sensitively unique,
-but only if it is not blank.
+Expected Example to validate that :attr is case-sensitively unique as


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -777,8 +809,8 @@
            end

            message = <<-MESSAGE
-Example did not properly validate that :attr is case-sensitively unique
-within the scope of :scope1.
+Expected Example to validate that :attr is case-sensitively unique


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -755,8 +786,9 @@
            end

            message = <<-MESSAGE
-Example did not properly validate that :attr is case-sensitively unique
-within the scope of :scope1, :scope2, and :other.
+Expected Example to validate that :attr is case-sensitively unique


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

+      end
+
+      message = <<-MESSAGE
+Expected Example not to validate that :attr is case-sensitively unique,


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

houndci-bot · 2018-09-12T05:38:12Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

+      assertion = lambda do
+        record = build_record_validating_uniqueness(
+          attribute_type: :string,
+          attribute_options: { limit: 1 }


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2018-09-12T05:38:13Z

spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb

@@ -663,7 +671,8 @@
          default_value: 'some value',
          changing_values_with: :next_value,
          expected_message: <<-MESSAGE.strip
-Example did not properly validate that :attr is case-sensitively unique.
+Expected Example to validate that :attr is case-sensitively unique, but


Use 2 spaces for indentation in a heredoc by using <<~ instead of <<-.

guialbuk

It fixes those issues in a clear way and set a standard for future matchers that will need to implement their negative versions as well 👍

mcmire added 3 commits September 11, 2018 23:21

Fix Zeus (this is addressed by other commits)

93ab76b

mcmire added this to the v4.0 milestone Sep 12, 2018

houndci-bot reviewed Sep 12, 2018

View reviewed changes

Update NEWS

5fa130c

[skip ci]

houndci-bot reviewed Sep 12, 2018

View reviewed changes

mcmire requested a review from guialbuk September 12, 2018 06:15

mcmire mentioned this pull request Sep 13, 2018

Resolves #1097: Add allow_nil qualifier to validate_presence_of #1100

Closed

guialbuk approved these changes Sep 15, 2018

View reviewed changes

guialbuk merged commit 18e7c88 into master Sep 15, 2018

guialbuk deleted the fix-negative-versions-of-matchers branch September 15, 2018 16:43

guialbuk mentioned this pull request Sep 15, 2018

Fix inclusion to correctly disallow outside values #1135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix negative versions of validation matchers #1136

Fix negative versions of validation matchers #1136

mcmire commented Sep 12, 2018 •

edited

Loading

houndci-bot Sep 12, 2018

mcmire Sep 13, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

guialbuk left a comment

Fix negative versions of validation matchers #1136

Fix negative versions of validation matchers #1136

Conversation

mcmire commented Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guialbuk left a comment

Choose a reason for hiding this comment

mcmire commented Sep 12, 2018 •

edited

Loading