-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add minimum and maximum validation to integer and nat #370
Add minimum and maximum validation to integer and nat #370
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the contribution. Very appreciated 👍
I made some comments on the review regarding the fact that taking fc.integer(1, 0)
was not reporting any problems while it should have.
Please add yourself into the CONTRIBUTORS file ^^
Concerning bigint, float, double.. go ahead ^^ But let's do that in another review so that we can close the first issue.
@@ -59,6 +59,7 @@ function integer(max: number): ArbitraryWithShrink<number>; | |||
*/ | |||
function integer(min: number, max: number): ArbitraryWithShrink<number>; | |||
function integer(a?: number, b?: number): ArbitraryWithShrink<number> { | |||
if (a && b && a > b) throw new Error('fc.integer maximum value should be equal or greater than the minimum one'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend to check the following instead:
a !== undefined && b !== undefined && a > b
Current code has an issue if one of a
or b
is 0.
I found that with a property based test (see below).
Eg.: a = 1
and b = 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! In the new commit, I fixed the issue, but although I checked the code several times, I couldn't get why the property test was failing with if (a && b && a > b)
but not with if (a !== undefined && b !== undefined && a > b)
Any ideas why that happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! The reason of the bug is that when b = 0
the condition inside the if
statement becomes falsy: if (0) { /* never executed */ }
. So when we have a = 1
and b = 0
the statement is equivalent to if (1 && 0 && 1 > 0)
which evaluates to false
because of the && 0 &&
.
@@ -79,6 +80,7 @@ function nat(): ArbitraryWithShrink<number>; | |||
*/ | |||
function nat(max: number): ArbitraryWithShrink<number>; | |||
function nat(a?: number): ArbitraryWithShrink<number> { | |||
if (a && a < 0) throw new Error('fc.nat value should be greater than or equal to 0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, no bug here but check for undefined instead: a !== undefined && a < 0
@@ -181,6 +184,9 @@ describe('IntegerArbitrary', () => { | |||
}); | |||
}); | |||
describe('nat', () => { | |||
it('Should throw when the number is less than 0', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion, a property based test:
it('Should throw when the number is less than 0', () =>
fc.assert(
fc.property(fc.integer(-0x80000000, -1), n => {
expect(() => nat(n)).toThrowError();
})
));
-0x80000000
or Number.MIN_SAFE_INTEGER
@@ -146,6 +146,9 @@ describe('IntegerArbitrary', () => { | |||
return -log2(-min) <= g && g <= log2(max); | |||
}) | |||
)); | |||
it('Should throw when minimum number is greater than maximum one', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion, a property based test:
it('Should throw when minimum number is greater than maximum one', () =>
fc.assert(
fc.property(fc.integer(), fc.integer(), (a, b) => {
fc.pre(a !== b);
if (a < b) expect(() => integer(b, a)).toThrowError();
else expect(() => integer(a, b)).toThrowError();
})
));
This property detected the issue reported for: a = 1 and b = 0
Thanks a lot for the contribution. Please let me know if you plan to do the same fix for float, double and bigint. |
Fixes #293
@dubzzz I noticed that BigInt and Float arbitraries have the same issue. If this solution is valid for you, I can take care of those cases in this PR as well.