Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test strict option on numericality matcher #620

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ def initialize(numericality_matcher, value, operator)
@strict = false
end

def strict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is already contained in ValidationMatcher so you don't need this here.

@strict = true
self
end

def for(attribute)
@attribute = attribute
self
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ def allowed_type
def diff_to_compare
2
end

def strict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can actually add this to NumericTypeMatcher instead of adding individually to EvenNumberMatcher, OddNumberMatcher, and OnlyIntegerMatcher.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just checked and this is already contained in ValidationMatcher so you can get rid of this. The only thing you need is the loop in ValidateNumericalityOfMatcher to set strict on each submatcher.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparison matcher is the only matcher that out of the numericality matchers that inherits from ValidationMatcher Should I make the others inherit from that matcher as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other matchers inherit from NumericTypeMatcher which already inherits from ValidationMatcher.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, misspoke here, they don't inherit from ValidationMatcher. We're actually trying to avoid inheriting from ValidationMatcher in the future. What you have here is fine.

@disallow_value_matcher.strict
self
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ def allowed_type
def diff_to_compare
2
end

def strict
@disallow_value_matcher.strict
self
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ def allowed_type
def diff_to_compare
1
end

def strict
@disallow_value_matcher.strict
self
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,13 @@ def initialize(attribute)
add_disallow_value_matcher
end

def strict
@submatchers.each do |submatcher|
submatcher.strict
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Line break here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hhm I can do it but it's inconsistent with the rest of this file

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this.

self
end

def only_integer
prepare_submatcher(
NumericalityMatchers::OnlyIntegerMatcher.new(@attribute)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ class << fake_matcher
end

context 'is_greater_than' do
it do
expect(instance_with_validations(greater_than: 1, strict: true)).to matcher.is_greater_than(1).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [107/80]

end

it do
expect(instance_with_validations(greater_than: 2))
.to matcher.is_greater_than(2)
Expand All @@ -39,6 +43,11 @@ class << fake_matcher
end

context 'greater_than_or_equal_to' do
it do
expect(instance_with_validations(greater_than_or_equal_to: 2, strict: true))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [82/80]

.to matcher.is_greater_than_or_equal_to(2).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the . on the previous line, together with the method call receiver.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true and same for the other warnings just like this. Not sure how the bad style crept through for other tests.

end

it do
expect(instance_with_validations(greater_than_or_equal_to: 2))
.to matcher.is_greater_than_or_equal_to(2)
Expand All @@ -61,6 +70,11 @@ class << fake_matcher
end

context 'less_than' do
it do
expect(instance_with_validations(less_than: 2, strict: true))
.to matcher.is_less_than(2).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the . on the previous line, together with the method call receiver.

end

it do
expect(instance_with_validations(less_than: 2))
.to matcher.is_less_than(2)
Expand All @@ -83,6 +97,11 @@ class << fake_matcher
end

context 'less_than_or_equal_to' do
it do
expect(instance_with_validations(less_than_or_equal_to: 2, strict: true))
.to matcher.is_less_than_or_equal_to(2).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the . on the previous line, together with the method call receiver.

end

it do
expect(instance_with_validations(less_than_or_equal_to: 2))
.to matcher.is_less_than_or_equal_to(2)
Expand All @@ -105,6 +124,11 @@ class << fake_matcher
end

context 'is_equal_to' do
it do
expect(instance_with_validations(equal_to: 0, strict: true))
.to matcher.is_equal_to(0).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the . on the previous line, together with the method call receiver.

end

it do
expect(instance_with_validations(equal_to: 0))
.to matcher.is_equal_to(0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@
it { expect(subject.diff_to_compare).to eq 2 }
end

describe '#strict' do
it 'sets the strict matcher' do
expect(validating_even_number(strict: true)).to subject.strict
end
end

context 'when the model has an even validation' do
it 'matches' do
match = subject
Expand Down Expand Up @@ -56,4 +62,4 @@ def not_validating_even_number
define_model(:example, attr: :string).new
end

end
end
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@
it { expect(subject.diff_to_compare).to eq 2 }
end

describe '#strict' do
it 'allows strict' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test names can only be so descriptive, but maybe it 'accepts a model that validates strictly'?

expect(validating_odd_number(strict: true)).to subject.strict
end
end

context 'when the model has an odd validation' do
it 'matches' do
match = subject
Expand Down Expand Up @@ -56,4 +62,4 @@ def not_validating_odd_number
define_model(:example, attr: :string).new
end

end
end
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@
it { expect(subject.diff_to_compare).to eq 1 }
end

describe '#strict' do
it 'sets strict properly' do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here -- maybe it 'accepts a model that validates strictly'?

expect(validating_only_integer(strict: true)).to subject.strict
end
end

context 'given an attribute that only allows integer values' do
it 'matches' do
match = subject
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,26 @@
end
end

context "with the strict option" do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer single-quoted strings when you don't need string interpolation or special symbols.

it "allows strict values for that attribute" do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reword this same as above

expect(validating_numericality(strict: true)).to matcher.strict
end

it "rejects when model does not enforce strict" do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be single quotes.

expect(validating_numericality).to_not matcher.strict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_not -> not_to

end

it "rejects with the ActiveRecord :not_a_number exception" do
the_matcher = matcher.strict
failure_message =
"Expected exception to include \"Attr is not a number\" when attr is set to \"abcd\""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [93/80]


expect do
expect(validating_numericality).to the_matcher
end.to fail_with_message_including(failure_message)
end
end

context 'with the only_integer option' do
it 'allows integer values for that attribute' do
expect(validating_numericality(only_integer: true)).to matcher.only_integer
Expand Down Expand Up @@ -200,17 +220,24 @@
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this test. I feel confident that greater_than is tested along with strict in comparison_matcher_spec.

expect(validating_numericality(strict: true, greater_than: 99))
.to matcher.is_greater_than(99).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place the . on the previous line, together with the method call receiver.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how the style above crept through, but yes, this should read

expect(validating_numericality(strict: true, greater_than: 99)).
  to matcher.is_greater_than(99).strict

end

it do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is fine. We are obviously not testing every possible combination here, but, I think this is okay.

I don't know how the style here crept through but it should be something like (I'm not sure if either of these lines fits, if not feel free to break them up):

record = validating_numericality(only_integer: true, greater_than: 18, less_than: 99)
expect(record).to matcher.only_integer.is_greater_than(18).is_less_than(99).strict

expect(validating_numericality(
only_integer: true,
greater_than: 18,
less_than: 99)
).to matcher.only_integer.is_greater_than(18).is_less_than(99)
less_than: 99,
strict: true)
).to matcher.only_integer.is_greater_than(18).is_less_than(99).strict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line is too long. [83/80]

end
end

Expand Down