From eaaa2d83e5cd31a3ca0a1aaa65441ea1a4fffa49 Mon Sep 17 00:00:00 2001 From: Anthony Navarre + Elliot Winkler Date: Fri, 22 Nov 2013 13:46:59 -0700 Subject: [PATCH] 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 | 8 ++++ .../active_model/allow_value_matcher.rb | 32 +++++++++++++ .../validate_presence_of_matcher.rb | 14 ++---- spec/support/unit/helpers/model_builder.rb | 10 ++++- .../active_model/allow_value_matcher_spec.rb | 45 +++++++++++++++++++ .../validate_presence_of_matcher_spec.rb | 19 -------- 6 files changed, 97 insertions(+), 31 deletions(-) diff --git a/NEWS.md b/NEWS.md index 24abfd49..798ee673 100644 --- a/NEWS.md +++ b/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 diff --git a/lib/shoulda/matchers/active_model/allow_value_matcher.rb b/lib/shoulda/matchers/active_model/allow_value_matcher.rb index 2afcab81..81708a3e 100644 --- a/lib/shoulda/matchers/active_model/allow_value_matcher.rb +++ b/lib/shoulda/matchers/active_model/allow_value_matcher.rb @@ -170,6 +170,24 @@ module Shoulda # @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 @@ module Shoulda 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 @@ module Shoulda 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 diff --git a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb index 059d96c8..75826a53 100644 --- a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb @@ -140,17 +140,9 @@ module Shoulda 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 diff --git a/spec/support/unit/helpers/model_builder.rb b/spec/support/unit/helpers/model_builder.rb index c1060c86..955ef5d7 100644 --- a/spec/support/unit/helpers/model_builder.rb +++ b/spec/support/unit/helpers/model_builder.rb @@ -32,10 +32,18 @@ module UnitTests 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 diff --git a/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb b/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb index feb889f3..e118ac2a 100644 --- a/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb @@ -315,4 +315,49 @@ describe Shoulda::Matchers::ActiveModel::AllowValueMatcher, type: :model do 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 diff --git a/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb b/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb index f165245b..8b3ce490 100644 --- a/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb @@ -163,25 +163,6 @@ describe Shoulda::Matchers::ActiveModel::ValidatePresenceOfMatcher, type: :model 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