Skip to content

Commit

Permalink
Add ignoring_interference_by_writer to all matchers
Browse files Browse the repository at this point in the history
`allow_value` matcher is, of course, concerned with setting values on a
particular attribute on a particular record, and then checking that the
record is valid after doing so. That comes with a caveat: if the
attribute is overridden in such a way so that the same value going into
the attribute isn't the same value coming out of it, then `allow_value`
will balk -- it'll say, "I can't do that because that changes how I
work."

That's all well and good, but what the attribute intentionally changes
incoming values? ActiveRecord's typecasting behavior, for instance,
would trigger such an exception. What if the developer needs a way to
get around this? This is where `ignoring_interference_by_writer` comes
into play. You can tack it on to the end of the matcher, and you're free
to go on your way.

So, prior to this commit you could already apply it to `allow_value`,
but now in this commit it also works on any other matcher.

But, one little thing: sometimes using this qualifier isn't going to
work. Perhaps you or something else actually *is* overriding the
attribute to change incoming values in a specific way, and perhaps the
value that comes out makes the record fail validation, and there's
nothing you can do about it. So in this case, even if you're using
`ignoring_interference_by_writer`, we want to inform you about what the
attribute is doing -- what the input and output was. And so we do.
  • Loading branch information
mcmire committed Jan 5, 2016
1 parent 9e8603e commit 1189934
Show file tree
Hide file tree
Showing 32 changed files with 2,216 additions and 368 deletions.
6 changes: 6 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@
(formerly CouldNotSetAttributeError) when used against an attribute that is an
enum in an ActiveRecord model.

### Features

* Add a `ignoring_interference_by_writer` qualifier to all matchers, not just
`allow_value`. This makes it possible to get around CouldNotSetAttributeErrors
(now AttributeChangedValueErrors) that you are probably well acquainted with.

### Improvements

* Improve failure messages and descriptions of all matchers across the board so
Expand Down
1 change: 1 addition & 0 deletions lib/shoulda/matchers/active_model.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'shoulda/matchers/active_model/helpers'
require 'shoulda/matchers/active_model/qualifiers'
require 'shoulda/matchers/active_model/validation_matcher'
require 'shoulda/matchers/active_model/validation_matcher/build_description'
require 'shoulda/matchers/active_model/validator'
Expand Down
86 changes: 49 additions & 37 deletions lib/shoulda/matchers/active_model/allow_value_matcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ module ActiveModel
# #### Caveats
#
# When using `allow_value` or any matchers that depend on it, you may
# encounter a CouldNotSetAttributeError. This exception is raised if the
# encounter an AttributeChangedValueError. This exception is raised if the
# matcher, in attempting to set a value on the attribute, detects that
# the value set is different from the value that the attribute returns
# upon reading it back.
Expand Down Expand Up @@ -86,7 +86,7 @@ module ActiveModel
# it do
# foo = Foo.new
# foo.bar = "baz"
# # This will raise a CouldNotSetAttributeError since `foo.bar` is now "123"
# # This will raise an AttributeChangedValueError since `foo.bar` is now "123"
# expect(foo).not_to allow_value(nil).for(:bar)
# end
# end
Expand All @@ -108,7 +108,7 @@ module ActiveModel
# describe Foo do
# it do
# foo = Foo.new
# # This will raise a CouldNotSetAttributeError since `foo.bar` is now "123"
# # This will raise an AttributeChangedValueError since `foo.bar` is now "123"
# expect(foo).not_to allow_value("abc123").for(:bar)
# end
# end
Expand All @@ -118,32 +118,25 @@ module ActiveModel
#
# describe Foo do
# # Assume that `attr` is a string
# # This will raise a CouldNotSetAttributeError since `attr` typecasts `[]` to `"[]"`
# # This will raise an AttributeChangedValueError since `attr` typecasts `[]` to `"[]"`
# it { should_not allow_value([]).for(:attr) }
# end
#
# So when you encounter this exception, you have a couple of options:
#
# * If you understand the problem and wish to override this behavior to
# get around this exception, you can add the
# `ignoring_interference_by_writer` qualifier like so:
# Fortunately, if you understand why this is happening, and wish to get
# around this exception, it is possible to do so. You can use the
# `ignoring_interference_by_writer` qualifier like so:
#
# it do
# should_not allow_value([]).
# for(:attr).
# ignoring_interference_by_writer
# end
#
# * Note, however, that the above option will not always cause the test to
# pass. In this case, this is telling you that you don't need to use
# `allow_value`, or quite possibly even the validation that you're
# testing altogether. In any case, we would probably make the argument
# that since it's clear that something is responsible for sanitizing
# incoming data before it's stored in your model, there's no need to
# ensure that sanitization places the model in a valid state, if such
# sanitization creates valid data. In terms of testing, the sanitization
# code should probably be tested, but not the effects of that
# sanitization on the validness of the model.
# Please note, however, that this qualifier won't magically cause your
# test to pass. It may just so happen that the final value that ends up
# being set causes the model to fail validation. In that case, you'll have
# to figure out what to do. You may need to write your own test, or
# perhaps even remove your test altogether.
#
# #### Qualifiers
#
Expand Down Expand Up @@ -274,9 +267,9 @@ module ActiveModel
#
# ##### ignoring_interference_by_writer
#
# Use `ignoring_interference_by_writer` if you've encountered a
# CouldNotSetAttributeError and wish to ignore it. Please read the Caveats
# section above for more information.
# Use `ignoring_interference_by_writer` to bypass an
# AttributeChangedValueError that you have encountered. Please read the
# Caveats section above for more information.
#
# class Address < ActiveRecord::Base
# # Address has a zip_code field which is a string
Expand All @@ -286,16 +279,16 @@ module ActiveModel
# describe Address do
# it do
# should_not allow_value([]).
# for(:zip_code).
# ignoring_interference_by_writer
# for(:zip_code).
# ignoring_interference_by_writer
# end
# end
#
# # Minitest (Shoulda)
# class AddressTest < ActiveSupport::TestCase
# should_not allow_value([]).
# for(:zip_code).
# ignoring_interference_by_writer
# for(:zip_code).
# ignoring_interference_by_writer
# end
#
# @return [AllowValueMatcher]
Expand All @@ -313,6 +306,7 @@ def allow_value(*values)
# @private
class AllowValueMatcher
include Helpers
include Qualifiers::IgnoringInterferenceByWriter

attr_reader(
:after_setting_value_callback,
Expand All @@ -325,10 +319,10 @@ class AllowValueMatcher
attr_writer :failure_message_preface, :values_to_preset

def initialize(*values)
super
@values_to_set = values
@options = {}
@after_setting_value_callback = -> {}
@ignoring_interference_by_writer = false
@expects_strict = false
@expects_custom_validation_message = false
@context = nil
Expand Down Expand Up @@ -387,15 +381,6 @@ def expects_strict?
@expects_strict
end

def ignoring_interference_by_writer(value = true)
@ignoring_interference_by_writer = value
self
end

def ignoring_interference_by_writer?
@ignoring_interference_by_writer
end

def _after_setting_value(&callback)
@after_setting_value_callback = callback
end
Expand Down Expand Up @@ -432,6 +417,10 @@ def failure_message
end
end

if include_attribute_changed_value_message?
message << "\n\n" + attribute_changed_value_message
end

Shoulda::Matchers.word_wrap(message)
end

Expand Down Expand Up @@ -497,9 +486,23 @@ def failure_message_when_negated
end
end

if include_attribute_changed_value_message?
message << "\n\n" + attribute_changed_value_message
end

Shoulda::Matchers.word_wrap(message)
end

def attribute_changed_value_message
<<-MESSAGE.strip
As indicated in the message above, :#{result.attribute_setter.attribute_name}
seems to be changing certain values as they are set, and this could have
something to do with why this test is failing. If you've overridden the writer
method for this attribute, then you may need to change it to make this test
pass, or do something else entirely.
MESSAGE
end

def description
ValidationMatcher::BuildDescription.call(self, simple_description)
end
Expand All @@ -512,8 +515,12 @@ def model
instance.class
end

def last_attribute_setter_used
result.attribute_setter
end

def last_value_set
result.attribute_setter.value_written
last_attribute_setter_used.value_written
end

protected
Expand Down Expand Up @@ -579,6 +586,11 @@ def attribute_setters_and_validators_for_values_to_set
)
end

def include_attribute_changed_value_message?
!ignore_interference_by_writer.never? &&
result.attribute_setter.attribute_changed_value?
end

def inspected_values_to_set
Shoulda::Matchers::Util.inspect_values(values_to_set).to_sentence(
two_words_connector: " or ",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,25 @@ def message
#{model.name} to #{value_written.inspect}, but when the attribute was
read back, it had stored #{value_read.inspect} instead.
This creates a problem because it means that the model is behaving in a way that
is interfering with the test -- there's a mismatch between the test that was
written and test that was actually run.
This creates a problem because it means that the model is behaving in a
way that is interfering with the test -- there's a mismatch between the
test that you wrote and test that we actually ran.
There are a couple of reasons why this could be happening:
* The writer method for :#{attribute_name} has been overridden and contains
custom logic to prevent certain values from being set or change which values
are stored.
* ActiveRecord is typecasting the incoming value.
* The writer method for :#{attribute_name} has been overridden so that
incoming values are changed in some way.
Regardless, the fact you're seeing this message usually indicates a larger
problem. Please file an issue on the GitHub repo for shoulda-matchers,
including details about your model and the test you've written, and we can point
you in the right direction:
If this exception makes sense to you and you wish to bypass it, try
adding the `ignoring_interference_by_writer` qualifier onto the end of
your matcher. If the test still does not pass after that, then you may
need to do something different.
https://github.com/thoughtbot/shoulda-matchers/issues
If you need help, feel free to ask a question on the shoulda-matchers
issues list:
http://github.com/thoughtbot/shoulda-matchers/issues
MESSAGE
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,23 @@ def self.set(args)
new(args).set
end

attr_reader :result_of_checking, :result_of_setting,
:value_written
attr_reader(
:attribute_name,
:result_of_checking,
:result_of_setting,
:value_written,
)

def initialize(args)
@args = args
@matcher_name = args.fetch(:matcher_name)
@object = args.fetch(:object)
@attribute_name = args.fetch(:attribute_name)
@value_written = args.fetch(:value)
@ignoring_interference_by_writer =
args.fetch(:ignoring_interference_by_writer, false)
@ignore_interference_by_writer = args.fetch(
:ignore_interference_by_writer,
Qualifiers::IgnoreInterferenceByWriter.new
)
@after_set_callback = args.fetch(:after_set_callback, -> { })

@result_of_checking = nil
Expand Down Expand Up @@ -125,10 +132,17 @@ def successfully_set?
set? && result_of_setting.successful?
end

def value_read
@_value_read ||= object.public_send(attribute_name)
end

def attribute_changed_value?
value_written != value_read
end

protected

attr_reader :matcher_name, :object, :attribute_name,
:after_set_callback
attr_reader :args, :matcher_name, :object, :after_set_callback

private

Expand All @@ -144,22 +158,14 @@ def attribute_exists?
end
end

def attribute_changed_value?
value_written != value_read
end

def value_read
@_value_read ||= object.public_send(attribute_name)
end

def ignoring_interference_by_writer?
!!@ignoring_interference_by_writer
def ignore_interference_by_writer
@ignore_interference_by_writer
end

def raise_attribute_changed_value_error?
attribute_changed_value? &&
!(attribute_is_an_enum? && value_read_is_expected_for_an_enum?) &&
!ignoring_interference_by_writer?
!ignore_interference_by_writer.considering?(value_read)
end

def attribute_is_an_enum?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class AttributeSetterAndValidator
:context,
:expected_message,
:expects_strict?,
:ignoring_interference_by_writer?,
:ignore_interference_by_writer,
:instance,
)

Expand All @@ -33,7 +33,7 @@ def attribute_setter
object: instance,
attribute_name: attribute_name,
value: value,
ignoring_interference_by_writer: ignoring_interference_by_writer?,
ignore_interference_by_writer: ignore_interference_by_writer,
after_set_callback: after_setting_value_callback
)
end
Expand Down
18 changes: 11 additions & 7 deletions lib/shoulda/matchers/active_model/disallow_value_matcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,21 @@ module ActiveModel
class DisallowValueMatcher
extend Forwardable

def_delegators :allow_matcher,
def_delegators(
:allow_matcher,
:_after_setting_value,
:attribute_to_set,
:description,
:expects_strict?,
:model,
:last_value_set,
:simple_description,
:failure_message_preface,
:failure_message_preface=,
:values_to_preset=
:ignore_interference_by_writer,
:last_attribute_setter_used,
:last_value_set,
:model,
:simple_description,
:values_to_preset=,
)

def initialize(value)
@allow_matcher = AllowValueMatcher.new(value)
Expand Down Expand Up @@ -51,8 +55,8 @@ def strict(strict = true)
self
end

def ignoring_interference_by_writer
allow_matcher.ignoring_interference_by_writer
def ignoring_interference_by_writer(value = :always)
allow_matcher.ignoring_interference_by_writer(value)
self
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ class NumericTypeMatcher
:expects_strict?,
:failure_message,
:failure_message_when_negated,
:ignore_interference_by_writer,
:ignoring_interference_by_writer,
:matches?,
:on,
:strict,
Expand Down
Loading

0 comments on commit 1189934

Please sign in to comment.