allow_value: Raise error if attr sets value differently

`allow_value` will now raise a CouldNotSetAttribute error if the
attribute in question cannot be changed from a non-nil value to a nil
value, or vice versa. In other words, these are the exact cases in which
the error will occur:

* If you're testing whether the attribute allows `nil`, but the
  attribute detects and ignores nil. (For instance, you have a model
  that `has_secure_password`. This will add a #password= method to your
  model that is defined in a such a way that you cannot clear the
  password by setting it to nil -- nothing happens.)
* If you're testing whether the attribute allows a non-nil value, but
  the attribute fails to set that value. (For instance, you have an
  ActiveRecord model. If ActiveRecord cannot typecast the value in the
  context of the column, then it will do nothing, and the attribute will be
  effectively set to nil.)

What's the reasoning behind this change? Simply put, if you are assuming
that the attribute is changing but in fact it is not, then the test
you're writing isn't the test that actually gets run. We feel that this
is dishonest and produces an invalid test.

NEWS.md

@@ -21,6 +21,14 @@
   * `set_session['key'].to(nil)` will no longer pass when the key in question
     has not been set yet.
 
+* `allow_value` may raise an error if the attribute in question contains custom
+  logic to ignore certain values, resulting in a discrepancy between the value
+  you provide and the value that the attribute is actually set to. Specifically,
+  if the attribute cannot be changed from a non-nil value to a nil value, or
+  vice versa, then you'll get a CouldNotSetAttributeError. The current behavior
+  (which is to permit this) is misleading, as the test that you write by using
+  `allow_value` is different from the test that actually ends up getting run.
+
 ### Bug fixes
 
 * So far the tests for the gem have been running against only SQLite. Now they

lib/shoulda/matchers/active_model/allow_value_matcher.rb

@@ -170,6 +170,24 @@ def allow_value(*values)
 
       # @private
       class AllowValueMatcher
+        # @private
+        class CouldNotSetAttributeError < Shoulda::Matchers::Error
+          def self.create(model, attribute, expected_value, actual_value)
+            super(
+              model: model,
+              attribute: attribute,
+              expected_value: expected_value,
+              actual_value: actual_value
+            )
+          end
+
+          attr_accessor :model, :attribute, :expected_value, :actual_value
+
+          def message
+            "Expected #{model.class} to be able to set #{attribute} to #{expected_value.inspect}, but got #{actual_value.inspect} instead."
+          end
+        end
+
         include Helpers
 
         attr_accessor :attribute_with_message
@@ -266,6 +284,7 @@ def set_attribute(value)
 
         def set_attribute_ignoring_range_errors(value)
           instance.__send__("#{attribute_to_set}=", value)
+          ensure_that_attribute_has_been_changed_to_or_from_nil!(value)
         rescue RangeError => exception
           # Have to reset the attribute so that we don't get a RangeError the
           # next time we attempt to write the attribute (ActiveRecord seems to
@@ -278,6 +297,19 @@ def reset_attribute
           instance.send(:raw_write_attribute, attribute_to_set, nil)
         end
 
+        def ensure_that_attribute_has_been_changed_to_or_from_nil!(expected_value)
+          actual_value = instance.__send__(attribute_to_set)
+
+          if expected_value.nil? != actual_value.nil?
+            raise CouldNotSetAttributeError.create(
+              instance.class,
+              attribute_to_set,
+              expected_value,
+              actual_value
+            )
+          end
+        end
+
         def errors_match?
           has_messages? && errors_for_attribute_match?
         end

lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb

@@ -140,17 +140,9 @@ def secure_password_being_validated?
         end
 
         def disallows_and_double_checks_value_of!(value, message)
-          error_class = Shoulda::Matchers::ActiveModel::CouldNotSetPasswordError
-
-          disallows_value_of(value, message) do |matcher|
-            matcher._after_setting_value do
-              actual_value = @subject.__send__(@attribute)
-
-              if !actual_value.nil?
-                raise error_class.create(@subject.class)
-              end
-            end
-          end
+          disallows_value_of(value, message)
+        rescue ActiveModel::AllowValueMatcher::CouldNotSetAttributeError
+          raise ActiveModel::CouldNotSetPasswordError.create(@subject.class)
         end
 
         def blank_value

spec/support/unit/helpers/model_builder.rb

@@ -32,10 +32,18 @@ def define_model_class(class_name, &block)
     end
 
     def define_active_model_class(class_name, options = {}, &block)
+      accessors = options.fetch(:accessors, [])
+
       define_class(class_name) do
         include ActiveModel::Validations
 
-        options[:accessors].each do |column|
+        def initialize(attributes = {})
+          attributes.each do |name, value|
+            __send__("#{name}=", value)
+          end
+        end
+
+        accessors.each do |column|
           attr_accessor column.to_sym
         end
 

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

@@ -315,4 +315,49 @@
     end
   end
 
+  context 'when the attribute writer method ignores a non-nil value' do
+    context 'when the attribute has a reader method' do
+      it 'raises a CouldNotSetAttributeError' do
+        model = define_active_model_class 'Example' do
+          attr_reader :name
+
+          def name=(_value)
+            nil
+          end
+        end
+
+        assertion = -> {
+          expect(model.new).to allow_value('anything').for(:name)
+        }
+
+        expect(&assertion).to raise_error(
+          Shoulda::Matchers::ActiveModel::AllowValueMatcher::CouldNotSetAttributeError
+        )
+      end
+    end
+  end
+
+  context 'when the attribute writer method ignores a nil value' do
+    context 'when the attribute has a reader method' do
+      it 'raises a CouldNotSetAttribute error' do
+        model = define_active_model_class 'Example' do
+          attr_reader :name
+
+          def name=(value)
+            @name = value unless value.nil?
+          end
+        end
+
+        record = model.new(name: 'some name')
+
+        assertion = -> {
+          expect(record).to allow_value(nil).for(:name)
+        }
+
+        expect(&assertion).to raise_error(
+          Shoulda::Matchers::ActiveModel::AllowValueMatcher::CouldNotSetAttributeError
+        )
+      end
+    end
+  end
 end

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

@@ -163,25 +163,6 @@
     end
   end
 
-  context 'when the attribute being tested intercepts the blank value we set on it (issue #479)' do
-    context 'for a non-collection attribute' do
-      it 'does not raise an error' do
-        record = define_model :example, attr: :string do
-          validates :attr, presence: true
-
-          def attr=(value)
-            value = '' if value.nil?
-            super(value)
-          end
-        end.new
-
-        expect do
-          expect(record).to validate_presence_of(:attr)
-        end.not_to raise_error
-      end
-    end
-  end
-
   def matcher
     validate_presence_of(:attr)
   end