diff --git a/NEWS.md b/NEWS.md index 46b61254..c12baebf 100644 --- a/NEWS.md +++ b/NEWS.md @@ -33,6 +33,10 @@ * Add `allow_nil` option to `validate_numericality_of` so that you can validate that numeric values are validated only if a value is supplied. +* Fix `validate_numericality_of` so that test fails when the value with + `greater_than`, `greater_than_or_equal_to`, `less_than`, `less_than_or_equal_ + to` or `equal_to` is not appropriate. + # v 2.5.0 * Fix Rails/Test::Unit integration to ensure that the test case classes we are diff --git a/lib/shoulda/matchers/active_model/numericality_matchers/comparison_matcher.rb b/lib/shoulda/matchers/active_model/numericality_matchers/comparison_matcher.rb index 6a567793..cf1fb190 100644 --- a/lib/shoulda/matchers/active_model/numericality_matchers/comparison_matcher.rb +++ b/lib/shoulda/matchers/active_model/numericality_matchers/comparison_matcher.rb @@ -7,7 +7,11 @@ module Shoulda # :nodoc: # is_greater_than(6). # less_than(20)...(and so on) } class ComparisonMatcher < ValidationMatcher - def initialize(value, operator) + def initialize(numericality_matcher, value, operator) + unless numericality_matcher.respond_to? :diff_to_compare + raise ArgumentError, 'numericality_matcher is invalid' + end + @numericality_matcher = numericality_matcher @value = value @operator = operator @message = nil @@ -20,7 +24,7 @@ module Shoulda # :nodoc: def matches?(subject) @subject = subject - disallows_value_of(value_to_compare, @message) + all_bounds_correct? end def with_message(message) @@ -33,14 +37,23 @@ module Shoulda # :nodoc: private - def value_to_compare - case @operator - when :> then [@value, @value - 1].sample - when :>= then @value - 1 - when :== then @value + 1 - when :< then [@value, @value + 1].sample - when :<= then @value + 1 - end + def comparison_combos + allow = :allows_value_of + disallow = :disallows_value_of + checker_types = + case @operator + when :> then [allow, disallow, disallow] + when :>= then [allow, allow, disallow] + when :== then [disallow, allow, disallow] + when :< then [disallow, disallow, allow] + when :<= then [disallow, allow, allow] + end + diffs_to_compare.zip(checker_types) + end + + def diffs_to_compare + diff = @numericality_matcher.diff_to_compare + [diff, 0, -diff] end def expectation @@ -52,6 +65,12 @@ module Shoulda # :nodoc: when :<= then "less than or equal to" end end + + def all_bounds_correct? + comparison_combos.all? do |diff, checker_type| + __send__(checker_type, @value + diff, @message) + end + end end end end diff --git a/lib/shoulda/matchers/active_model/numericality_matchers/even_number_matcher.rb b/lib/shoulda/matchers/active_model/numericality_matchers/even_number_matcher.rb index a9a59718..5b3138ee 100644 --- a/lib/shoulda/matchers/active_model/numericality_matchers/even_number_matcher.rb +++ b/lib/shoulda/matchers/active_model/numericality_matchers/even_number_matcher.rb @@ -15,6 +15,10 @@ module Shoulda # :nodoc: def allowed_type 'even numbers' end + + def diff_to_compare + 2 + end end end end diff --git a/lib/shoulda/matchers/active_model/numericality_matchers/numeric_type_matcher.rb b/lib/shoulda/matchers/active_model/numericality_matchers/numeric_type_matcher.rb index fa21d61a..58085f26 100644 --- a/lib/shoulda/matchers/active_model/numericality_matchers/numeric_type_matcher.rb +++ b/lib/shoulda/matchers/active_model/numericality_matchers/numeric_type_matcher.rb @@ -20,6 +20,10 @@ module Shoulda # :nodoc: raise NotImplementedError end + def diff_to_compare + raise NotImplementedError + end + def failure_message @disallow_value_matcher.failure_message end diff --git a/lib/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher.rb b/lib/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher.rb index e06c87b6..fb6e5783 100644 --- a/lib/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher.rb +++ b/lib/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher.rb @@ -15,6 +15,10 @@ module Shoulda # :nodoc: def allowed_type 'odd numbers' end + + def diff_to_compare + 2 + end end end end diff --git a/lib/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher.rb b/lib/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher.rb index f6e68fda..71c4dd2f 100644 --- a/lib/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher.rb +++ b/lib/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher.rb @@ -14,6 +14,10 @@ module Shoulda # :nodoc: def allowed_type 'integers' end + + def diff_to_compare + 1 + end end end end diff --git a/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb index 3c5ff013..6ffd0851 100644 --- a/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_numericality_of_matcher.rb @@ -26,58 +26,68 @@ module Shoulda # :nodoc: class ValidateNumericalityOfMatcher NUMERIC_NAME = 'numbers' NON_NUMERIC_VALUE = 'abcd' + DEFAULT_DIFF_TO_COMPARE = 0.000_000_000_001 + attr_reader :diff_to_compare def initialize(attribute) @attribute = attribute @submatchers = [] - + @diff_to_compare = DEFAULT_DIFF_TO_COMPARE add_disallow_value_matcher end def only_integer - add_submatcher(NumericalityMatchers::OnlyIntegerMatcher.new(@attribute)) + prepare_submatcher( + NumericalityMatchers::OnlyIntegerMatcher.new(@attribute) + ) self end def allow_nil - add_submatcher(AllowValueMatcher.new(nil).for(@attribute).with_message(:not_a_number)) - self - end - - def is_greater_than(value) - add_submatcher(NumericalityMatchers::ComparisonMatcher.new(value, :>).for(@attribute)) - self - end - - def is_greater_than_or_equal_to(value) - add_submatcher(NumericalityMatchers::ComparisonMatcher.new(value, :>=).for(@attribute)) - self - end - - def is_equal_to(value) - add_submatcher(NumericalityMatchers::ComparisonMatcher.new(value, :==).for(@attribute)) - self - end - - def is_less_than(value) - add_submatcher(NumericalityMatchers::ComparisonMatcher.new(value, :<).for(@attribute)) - self - end - - def is_less_than_or_equal_to(value) - add_submatcher(NumericalityMatchers::ComparisonMatcher.new(value, :<=).for(@attribute)) + prepare_submatcher( + AllowValueMatcher.new(nil) + .for(@attribute) + .with_message(:not_a_number) + ) self end def odd - odd_number_matcher = NumericalityMatchers::OddNumberMatcher.new(@attribute) - add_submatcher(odd_number_matcher) + prepare_submatcher( + NumericalityMatchers::OddNumberMatcher.new(@attribute) + ) self end def even - even_number_matcher = NumericalityMatchers::EvenNumberMatcher.new(@attribute) - add_submatcher(even_number_matcher) + prepare_submatcher( + NumericalityMatchers::EvenNumberMatcher.new(@attribute) + ) + self + end + + def is_greater_than(value) + prepare_submatcher(comparison_matcher_for(value, :>).for(@attribute)) + self + end + + def is_greater_than_or_equal_to(value) + prepare_submatcher(comparison_matcher_for(value, :>=).for(@attribute)) + self + end + + def is_equal_to(value) + prepare_submatcher(comparison_matcher_for(value, :==).for(@attribute)) + self + end + + def is_less_than(value) + prepare_submatcher(comparison_matcher_for(value, :<).for(@attribute)) + self + end + + def is_less_than_or_equal_to(value) + prepare_submatcher(comparison_matcher_for(value, :<=).for(@attribute)) self end @@ -115,10 +125,27 @@ module Shoulda # :nodoc: add_submatcher(disallow_value_matcher) end + def prepare_submatcher(submatcher) + add_submatcher(submatcher) + if submatcher.respond_to?(:diff_to_compare) + update_diff_to_compare(submatcher) + end + end + + def comparison_matcher_for(value, operator) + NumericalityMatchers::ComparisonMatcher + .new(self, value, operator) + .for(@attribute) + end + def add_submatcher(submatcher) @submatchers << submatcher end + def update_diff_to_compare(matcher) + @diff_to_compare = [@diff_to_compare, matcher.diff_to_compare].max + end + def submatchers_match? failing_submatchers.empty? end @@ -150,7 +177,12 @@ module Shoulda # :nodoc: end def submatcher_comparison_descriptions - @submatchers.inject([]){|m, s| m << s.comparison_description if s.respond_to?(:comparison_description); m } + @submatchers.inject([]) do |arr, submatcher| + if submatcher.respond_to? :comparison_description + arr << submatcher.comparison_description + end + arr + end end end end diff --git a/spec/shoulda/matchers/active_model/numericality_matchers/comparison_matcher_spec.rb b/spec/shoulda/matchers/active_model/numericality_matchers/comparison_matcher_spec.rb index d8692b78..4ea3016e 100644 --- a/spec/shoulda/matchers/active_model/numericality_matchers/comparison_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/numericality_matchers/comparison_matcher_spec.rb @@ -1,33 +1,129 @@ require 'spec_helper' describe Shoulda::Matchers::ActiveModel::NumericalityMatchers::ComparisonMatcher do - it_behaves_like 'a numerical submatcher' do - subject { described_class.new(0, :>) } + subject { described_class.new(matcher, 0, :>) } + + it_behaves_like 'a numerical submatcher' + + context 'when initialized without correct numerical matcher' do + it 'raises an argument error' do + fake_matcher = matcher + class << fake_matcher + undef_method :diff_to_compare + end + expect do + described_class.new(fake_matcher, 0, :>) + end.to raise_error ArgumentError + end end context 'is_greater_than' do - it { expect(instance_with_validations(greater_than: 2)).to matcher.is_greater_than(2) } - it { expect(instance_without_validations).not_to matcher.is_greater_than(2) } + it do + expect(instance_with_validations(greater_than: 2)) + .to matcher.is_greater_than(2) + end + + it do + expect(instance_with_validations(greater_than: 1.5)) + .not_to matcher.is_greater_than(2) + end + + it do + expect(instance_with_validations(greater_than: 2.5)) + .not_to matcher.is_greater_than(2) + end + + it do + expect(instance_without_validations).not_to matcher.is_greater_than(2) + end end context 'greater_than_or_equal_to' do - it { expect(instance_with_validations(greater_than_or_equal_to: 2)).to matcher.is_greater_than_or_equal_to(2) } - it { expect(instance_without_validations).not_to matcher.is_greater_than_or_equal_to(2) } + it do + expect(instance_with_validations(greater_than_or_equal_to: 2)) + .to matcher.is_greater_than_or_equal_to(2) + end + + it do + expect(instance_with_validations(greater_than_or_equal_to: 1.5)) + .not_to matcher.is_greater_than_or_equal_to(2) + end + + it do + expect(instance_with_validations(greater_than_or_equal_to: 2.5)) + .not_to matcher.is_greater_than_or_equal_to(2) + end + + it do + expect(instance_without_validations) + .not_to matcher.is_greater_than_or_equal_to(2) + end end context 'less_than' do - it { expect(instance_with_validations(less_than: 2)).to matcher.is_less_than(2) } - it { expect(instance_without_validations).not_to matcher.is_less_than(2) } + it do + expect(instance_with_validations(less_than: 2)) + .to matcher.is_less_than(2) + end + + it do + expect(instance_with_validations(less_than: 1.5)) + .not_to matcher.is_less_than(2) + end + + it do + expect(instance_with_validations(less_than: 2.5)) + .not_to matcher.is_less_than(2) + end + + it do + expect(instance_without_validations) + .not_to matcher.is_less_than(2) + end end context 'less_than_or_equal_to' do - it { expect(instance_with_validations(less_than_or_equal_to: 2)).to matcher.is_less_than_or_equal_to(2) } - it { expect(instance_without_validations).not_to matcher.is_less_than_or_equal_to(2) } + it do + expect(instance_with_validations(less_than_or_equal_to: 2)) + .to matcher.is_less_than_or_equal_to(2) + end + + it do + expect(instance_with_validations(less_than_or_equal_to: 1.5)) + .not_to matcher.is_less_than_or_equal_to(2) + end + + it do + expect(instance_with_validations(less_than_or_equal_to: 2.5)) + .not_to matcher.is_less_than_or_equal_to(2) + end + + it do + expect(instance_without_validations) + .not_to matcher.is_less_than_or_equal_to(2) + end end context 'is_equal_to' do - it { expect(instance_with_validations(equal_to: 0)).to matcher.is_equal_to(0) } - it { expect(instance_without_validations).not_to matcher.is_equal_to(0) } + it do + expect(instance_with_validations(equal_to: 0)) + .to matcher.is_equal_to(0) + end + + it do + expect(instance_with_validations(equal_to: -0.5)) + .not_to matcher.is_equal_to(0) + end + + it do + expect(instance_with_validations(equal_to: 0.5)) + .not_to matcher.is_equal_to(0) + end + + it do + expect(instance_without_validations) + .not_to matcher.is_equal_to(0) + end end context 'with_message' do @@ -45,7 +141,10 @@ describe Shoulda::Matchers::ActiveModel::NumericalityMatchers::ComparisonMatcher { operator: :<=, value: 4, expectation: 'less than or equal to 4' }, ].each do |h| context "with :#{h[:operator]} as operator and #{h[:value]} as value" do - subject { described_class.new(h[:value], h[:operator]).comparison_description } + subject do + described_class.new(matcher, h[:value], h[:operator]) + .comparison_description + end it { should eq h[:expectation] } end end diff --git a/spec/shoulda/matchers/active_model/numericality_matchers/even_number_matcher_spec.rb b/spec/shoulda/matchers/active_model/numericality_matchers/even_number_matcher_spec.rb index f9ca318f..199e09dc 100644 --- a/spec/shoulda/matchers/active_model/numericality_matchers/even_number_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/numericality_matchers/even_number_matcher_spec.rb @@ -10,6 +10,10 @@ describe Shoulda::Matchers::ActiveModel::NumericalityMatchers::EvenNumberMatcher expect(subject.allowed_type).to eq 'even numbers' end + describe '#diff_to_compare' do + it { expect(subject.diff_to_compare).to eq 2 } + end + context 'when the model has an even validation' do it 'matches' do match = subject diff --git a/spec/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher_spec.rb b/spec/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher_spec.rb index 2735f2ec..b4aebd5d 100644 --- a/spec/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/numericality_matchers/odd_number_matcher_spec.rb @@ -10,6 +10,10 @@ describe Shoulda::Matchers::ActiveModel::NumericalityMatchers::OddNumberMatcher expect(subject.allowed_type).to eq 'odd numbers' end + describe '#diff_to_compare' do + it { expect(subject.diff_to_compare).to eq 2 } + end + context 'when the model has an odd validation' do it 'matches' do match = subject diff --git a/spec/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher_spec.rb b/spec/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher_spec.rb index d6504611..35378bf4 100644 --- a/spec/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/numericality_matchers/only_integer_matcher_spec.rb @@ -10,6 +10,10 @@ describe Shoulda::Matchers::ActiveModel::NumericalityMatchers::OnlyIntegerMatche expect(subject.allowed_type).to eq 'integers' end + describe '#diff_to_compare' do + it { expect(subject.diff_to_compare).to eq 1 } + end + context 'given an attribute that only allows integer values' do it 'matches' do match = subject diff --git a/spec/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb b/spec/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb index 61006f8a..346b7c63 100644 --- a/spec/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb @@ -22,28 +22,35 @@ describe Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher do the_matcher.matches?(define_model(:example, attr: :string).new) - expect(the_matcher.failure_message_when_negated).to include 'Did not expect errors to include "is not a number"' + expect(the_matcher.failure_message_when_negated) + .to include 'Did not expect errors to include "is not a number"' end it 'rejects with the ActiveRecord :not_an_integer message' do the_matcher = matcher.only_integer - expect { + expect do expect(not_validating_numericality).to the_matcher - }.to fail_with_message_including('Expected errors to include "must be an integer"') + end.to fail_with_message_including( + 'Expected errors to include "must be an integer"' + ) end it 'rejects with the ActiveRecord :odd message' do the_matcher = matcher.odd - expect { + expect do expect(not_validating_numericality).to the_matcher - }.to fail_with_message_including('Expected errors to include "must be odd"') + end.to fail_with_message_including( + 'Expected errors to include "must be odd"' + ) end it 'rejects with the ActiveRecord :even message' do the_matcher = matcher.even - expect { + expect do expect(not_validating_numericality).to the_matcher - }.to fail_with_message_including('Expected errors to include "must be even"') + end.to fail_with_message_including( + 'Expected errors to include "must be even"' + ) end end @@ -71,9 +78,11 @@ describe Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher do it 'rejects with the ActiveRecord :not_an_integer message' do the_matcher = matcher.only_integer - expect { + expect do expect(validating_numericality).to the_matcher - }.to fail_with_message_including('Expected errors to include "must be an integer"') + end.to fail_with_message_including( + 'Expected errors to include "must be an integer"' + ) end end @@ -88,9 +97,11 @@ describe Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher do it 'rejects with the ActiveRecord :odd message' do the_matcher = matcher.odd - expect { + expect do expect(validating_numericality).to the_matcher - }.to fail_with_message_including('Expected errors to include "must be odd"') + end.to fail_with_message_including( + 'Expected errors to include "must be odd"' + ) end end @@ -105,9 +116,123 @@ describe Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher do it 'rejects with the ActiveRecord :even message' do the_matcher = matcher.even - expect { + expect do expect(validating_numericality).to the_matcher - }.to fail_with_message_including('Expected errors to include "must be even"') + end.to fail_with_message_including( + 'Expected errors to include "must be even"' + ) + end + end + + context 'with multiple options together' do + context 'the success cases' do + it do + expect(validating_numericality(only_integer: true, greater_than: 18)) + .to matcher.only_integer.is_greater_than(18) + end + + it do + expect(validating_numericality(even: true, greater_than: 18)) + .to matcher.even.is_greater_than(18) + end + it do + expect(validating_numericality(odd: true, less_than_or_equal_to: 99)) + .to matcher.odd.is_less_than_or_equal_to(99) + end + + it do + expect(validating_numericality( + only_integer: true, + greater_than: 18, + less_than: 99) + ).to matcher.only_integer.is_greater_than(18).is_less_than(99) + end + end + + context 'the failure cases with different validators' do + it do + expect(validating_numericality(even: true, greater_than: 18)) + .not_to matcher.only_integer.is_greater_than(18) + end + + it do + expect(validating_numericality(greater_than: 18)) + .not_to matcher.only_integer.is_greater_than(18) + end + + it do + expect( + validating_numericality(even: true, greater_than_or_equal_to: 18) + ).not_to matcher.even.is_greater_than(18) + end + + it do + expect(validating_numericality(odd: true, greater_than: 18)) + .not_to matcher.even.is_greater_than(18) + end + + it do + expect(validating_numericality( + odd: true, + greater_than_or_equal_to: 99 + ) + ).not_to matcher.odd.is_less_than_or_equal_to(99) + end + + it do + expect(validating_numericality( + only_integer: true, + greater_than_or_equal_to: 18, + less_than: 99 + ) + ).not_to matcher.only_integer.is_greater_than(18).is_less_than(99) + end + end + + context 'the failure cases with wrong values' do + it do + expect(validating_numericality(only_integer: true, greater_than: 19)) + .not_to matcher.only_integer.is_greater_than(18) + end + + it do + expect(validating_numericality(only_integer: true, greater_than: 17)) + .not_to matcher.only_integer.is_greater_than(18) + end + + it do + expect(validating_numericality(even: true, greater_than: 20)) + .not_to matcher.even.is_greater_than(18) + end + + it do + expect(validating_numericality(even: true, greater_than: 16)) + .not_to matcher.even.is_greater_than(18) + end + + it do + expect(validating_numericality(odd: true, less_than_or_equal_to: 101)) + .not_to matcher.odd.is_less_than_or_equal_to(99) + end + + it do + expect(validating_numericality(odd: true, less_than_or_equal_to: 97)) + .not_to matcher.odd.is_less_than_or_equal_to(99) + end + + it do + expect(validating_numericality(only_integer: true, + greater_than: 19, + less_than: 99)) + .not_to matcher.only_integer.is_greater_than(18).is_less_than(99) + end + + it do + expect(validating_numericality(only_integer: true, + greater_than: 18, + less_than: 100)) + .not_to matcher.only_integer.is_greater_than(18).is_less_than(99) + end end end @@ -142,26 +267,45 @@ describe Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher do end context 'with only integer option' do - it { expect(matcher.only_integer.description).to eq 'only allow integers for attr' } + it do + expect(matcher.only_integer.description) + .to eq 'only allow integers for attr' + end end [:odd, :even].each do |type| context "with #{type} option" do - it { expect(matcher.__send__(type).description).to eq "only allow #{type} numbers for attr" } + it do + expect(matcher.__send__(type).description) + .to eq "only allow #{type} numbers for attr" + end end end - [:is_greater_than, :is_greater_than_or_equal_to, :is_less_than, :is_less_than_or_equal_to, + [:is_greater_than, + :is_greater_than_or_equal_to, + :is_less_than, + :is_less_than_or_equal_to, :is_equal_to ].each do |comparison| context "with #{comparison} option" do - it { expect(matcher.__send__(comparison, 18).description). - to eq "only allow numbers for attr which are #{comparison.to_s.sub('is_','').gsub('_', ' ')} 18" } + it do + expect(matcher.__send__(comparison, 18).description) + .to eq( + 'only allow numbers for attr which are ' + + "#{comparison.to_s.sub('is_', '').gsub('_', ' ')} 18" + ) + end end end context 'with odd, is_greater_than_or_equal_to option' do - it { expect(matcher.odd.is_greater_than_or_equal_to(18).description). - to eq "only allow odd numbers for attr which are greater than or equal to 18" } + it do + expect(matcher.odd.is_greater_than_or_equal_to(18).description) + .to eq( + 'only allow odd numbers for attr ' + + 'which are greater than or equal to 18' + ) + end end context 'with only integer, is_greater_than and less_than_or_equal_to option' do diff --git a/spec/support/shared_examples/numerical_type_submatcher.rb b/spec/support/shared_examples/numerical_type_submatcher.rb index e3dd7c91..94e73aa7 100644 --- a/spec/support/shared_examples/numerical_type_submatcher.rb +++ b/spec/support/shared_examples/numerical_type_submatcher.rb @@ -3,7 +3,14 @@ require 'spec_helper' shared_examples 'a numerical type submatcher' do it 'implements the allowed_type method' do expect(subject).to respond_to(:allowed_type).with(0).arguments + expect { subject.allowed_type }.not_to raise_error end + + it 'implements the diff_to_compare' do + expect(subject).to respond_to(:diff_to_compare).with(0).arguments + expect { subject.diff_to_compare }.not_to raise_error + end + it 'returns itself when given a message' do expect(subject.with_message('some message')).to eq subject end