Numerical matchers #244

Closed
wants to merge 10 commits into from

8 participants

@TiagoCardoso1983

Correction to the previous pull request ( #236 ) which was breaking the ruby 1.8 tests

@drapergeek
thoughtbot, inc. member

@TiagoCardoso1983 Maybe I'm misunderstanding something here. In order for me to use this matcher do I have to actually set the attribute by hand?

@TiagoCardoso1983

Well, yes, according to the AR, you say something like

validates :num, :numericality => {:greater_than => 5 }

and the spec should work accordingly

 should validate_numericality_of(:num).is_greater_than(5)  
@bryanrite

👍

@mxie
thoughtbot, inc. member

@TiagoCardoso1983 Yes, you are correct about that in a sense. However, I believe @drapergeek (and correct me if I'm wrong) is referring to the before blocks in the spec where you did:

before { subject.attr = 2 }

and the like. Our specs typically don't set attributes like that and test the matcher against those values. The specs should just verify that your matcher actually does the validate the way it's supposed to, e.g. that

validate_numericality_of(:attr).is_less_than_or_equal_to(4)

actually does exactly

validates_numericality_of :attr, :less_than_or_equal_to => 4

Check out spec/shoulda/matchers/active_model/validate_numericality_of_matcher_spec.rb for reference.

@conradwt

Has this issue been resolved? I ask the question because I'm still seeing the following message:

     Failure/Error: it { should validate_numericality_of(:attr).is_less_than_or_equal_to(4) }
     NoMethodError:
       undefined method `is_less_than_or_equal_to' for #<Shoulda::Matchers::ActiveModel::ValidateNumericalityOfMatcher:0x007fa896c72c70>
@mxie
thoughtbot, inc. member

@conradwt We're still waiting for @TiagoCardoso1983 to fix the PR. This will be resolved when they do.

@TiagoCardoso1983

Sorry for the lateness, was out of the internets for a while. I don't understand exactly the spec issue, but I tried to rewrite it in a more similar way to the other specs. If this doesn't solve the issue, please be more explicit in what it is wrong.

@drapergeek
thoughtbot, inc. member

@TiagoCardoso1983 I think you're still misunderstanding us. Having to set the attribute to a specific value in your test doesn't operate the way all our other tests work. The code should actually set this attribute without it having to happen in the test. The user shouldn't be responsible for that.

@TiagoCardoso1983

Sorry, I'm really misunderstanding because I still don't see it. How should the spec look like then? I'm testing the matcher against all possible options already.

@conradwt

Can someone send @TiagoCardoso1983 an example of one of the numeric matchers and the way it should be written as a spec? Another option would be for someone to pair with him to knock this out being that @TiagoCardoso1983 started this task.

@drapergeek
thoughtbot, inc. member

@TiagoCardoso1983
The fact that you have to set subject.attr = 1 in your tests is what concerns me. Given that case, if you were using the full method then your tests could not pass using this implementation:

 it { should validate_numericality_of(:age).greater_than(18) }

because you would have to set your subject to equal 18.

Does this make more sense? If you could write a spec that uses the full matcher I think you would understand why this is an issue.

Tiago Cardoso i think the problem was that i was assign explicitly in the it clause…
…. hope this is more in sync with the other specs
9fb69af
@TiagoCardoso1983

Again, sorry for the lateness. I perceived you had a problem with me explicitly assigning the attribute before the match, so I assumed you wanted a more idempotent way of instantiating objects. Please report back to me if this is still not in sync with what you want.

@fny

@TiagoCardoso1983

You're shouldn't define explicit numerical cases, but rather check that the correct validation is being implemented irrespective of the values involved.

Consider the shoulda's current test case for a validation requiring an odd number. It doesn't use any digits. Instead it creates a dummy model instance with the validation on the fly...

 def validating_numericality(options = {})
    define_model :example, :attr => :string do
      validates_numericality_of :attr, options
    end.new
  end

...and tests to check that the matcher exists:

    it 'allows odd number values for that attribute' do
      validating_numericality(:odd => true).should matcher.odd
    end

    it 'rejects when the model does not enforce odd number values' do
      validating_numericality.should_not matcher.odd
    end
@TiagoCardoso1983

Just noticed I can't really call a should_not matcher on top of a numerical validation. This:

it { should_not validate_numericality_of(:attr)

breaks because some negative_failure_message is not defined. Doesn't have anything to do with the matchers I inserted. Should I open an issue on this?

@fny

I think you need a failure_message_for_should and a failure_message_for_should_not. Check out the only_integer_matcher as an example.

@TiagoCardoso1983

Not on the version of the gem I'm working on, these methods do not exist.

@mxie
thoughtbot, inc. member

@fny Thanks for your explanation! You're exactly right about what we're looking for in the specs.

@TiagoCardoso1983 The failure_message_for_should and failure_message_for_should_not methods were originally missing from the validate_numericality_ofmatcher, as well as from its submatchers. However, they should now be in place as of version 2.1.0. Try rebasing your branch and see if they appear now.

Tiago Cardoso added some commits May 20, 2013
Tiago Cardoso Merge branch 'master' into numerical_matchers
Conflicts:
	lib/shoulda/matchers/active_model.rb
0f68727
Tiago Cardoso made the spec more readable 5ae1dd5
Tiago Cardoso added allowed_types method, which apparently has to be defined everyw…
…here, also submatchers, returning usually the same result as the main matcher
ab27a97
@TiagoCardoso1983

Is it now good to go?

@mcmire mcmire commented on the diff May 20, 2013
lib/shoulda/matchers/active_model/comparison_matcher.rb
@@ -0,0 +1,56 @@
+module Shoulda # :nodoc:
+ module Matchers
+ module ActiveModel # :nodoc:
+
@mcmire
mcmire added a note May 20, 2013

Would you mind tightening up this whitespace a bit? Just remove the empty lines before and after the class comment and right before def initialize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mcmire mcmire commented on an outdated diff May 20, 2013
...ulda/matchers/active_model/comparison_matcher_spec.rb
+ end
+
+ context 'with a model with a less than or equal to validation' do
+ subject { comparison_instance(:less_than_or_equal_to => 2) }
+ it { should matcher.is_less_than_or_equal_to(2) }
+ end
+
+ def comparison_instance(options = {})
+ define_model :example, :attr => :string do
+ validates_numericality_of :attr, options
+ attr_accessible :attr
+ end.new
+ end
+
+ def matcher
+ validate_numericality_of(:attr)#.send("is_#{match}", val)
@mcmire
mcmire added a note May 20, 2013

Remove this comment here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mcmire

@TiagoCardoso1983 I left a couple of comments.

You don't seem to have any opposite tests for the validation -- that is, what if you have a model that doesn't validate_numericality_of(:attr) and you attempt to validate_numericality_of(:attr).is_greater_than(2)?

What if you do this... have contexts for the different qualifiers (is_greater_than, is_less_than, etc.), and then inside of those contexts, test that instance_with_validation(:whatever => 2).should matcher.whatever(2) and instance_without_validations.should_not matcher.whatever(2).

Additionally, can you fold the subject into the test itself, and give a description to each test?

For instance you might have something like:

context 'is_greater_than' do
  it "accepts an instance with a validation" do
    instance_with_validation(:greater_than => 2).should matcher.is_greater_than(2)
  end

  it "rejects an instance without a validation" do
    instance_without_validations.should_not matcher.is_greater_than(2)
  end
end

I think if you do that we should be good to go.

@mcmire mcmire commented on an outdated diff May 21, 2013
lib/shoulda/matchers/active_model/comparison_matcher.rb
@@ -0,0 +1,56 @@
+module Shoulda # :nodoc:
+ module Matchers
+ module ActiveModel # :nodoc:
+
+ # Examples:
+ # it { should validate_numericality_of(:attr).
+ # is_greater_than(6).
+ # less_than(20)...(and so on) }
+
+
+ class ComparisonMatcher < ValidationMatcher
+
+ def initialize(value, operator)
+ @value = value
+ @options = { :operator => operator }
@mcmire
mcmire added a note May 21, 2013

Is there a reason to store this in @options? Can you not store this in simply @operator instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@TiagoCardoso1983

@mcmire , I don't see the use case there. less_than and greater_than are options of the numericality validation, they should be preceded by a numericality validation, I guess.

@mcmire

@TiagoCardoso1983 Right, I don't expect people to try to use is_greater_than, etc. when the model doesn't have a validation on it, but if you're writing test-first you expect such a test to fail and then you add the validation and then it should pass. That's the use case.

@mcmire

@TiagoCardoso1983 Okay so I think we're almost ready to merge this. Do you mind squashing your commits as it makes sense and/or rebasing if necessary before I do so?

@TiagoCardoso1983

Everything's up-to-date, no need to rebase. I didn't understand what you meant by "squashing commits", but I'd say it is good to go.

@drapergeek
thoughtbot, inc. member

@TiagoCardoso1983, as a rule when we're pulling in a feature, we 'squash' all the commits down into a singlle commit; @mcmire should be able to do this for you before he merges it in so its not a big deal. Thanks for putting this together!

@mcmire

Merged!

@mcmire mcmire closed this May 24, 2013
@mcmire mcmire added a commit that referenced this pull request May 24, 2013
@mcmire mcmire Update NEWS in conjunction with #244 111dc5a
@bdwain

Is this in the current build (2.1.0) of the gem yet? Or not until 2.2.0? I installed 2.1.0 and get errors saying is_greater_than is not defined (and all of the other new matchers)

@fny

The current v2.1.0 release on Rubygems does not contain the comparison matchers. You'll have to install the gem directly from the repo for now.

# in your Gemfile
gem 'shoulda-matchers', github: 'thoughtbot/shoulda-matchers'
@bdwain

That's what I figured. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment