Update numericality comarison matcher more accurate

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

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

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

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

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

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

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

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

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

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

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

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

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