From a24a060d1bec28260fe2776ebe384d9bb4996888 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 6 Mar 2019 09:33:51 -0700 Subject: [PATCH 1/3] Fix floating point arithmetic bug in range value validation --- src-docs/src/i18ntokens.json | 2 +- src-docs/src/views/range/range.js | 1 + src-docs/src/views/range/range_example.js | 1 + src/components/form/range/range_track.js | 4 +- src/services/index.ts | 2 +- src/services/number/number.test.tsx | 47 ++++++++++++++++++++++- src/services/number/number.ts | 19 +++++++++ 7 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src-docs/src/i18ntokens.json b/src-docs/src/i18ntokens.json index 28dedc536eb..c223018497d 100644 --- a/src-docs/src/i18ntokens.json +++ b/src-docs/src/i18ntokens.json @@ -735,4 +735,4 @@ }, "filepath": "src/components/toast/toast.js" } -] +] \ No newline at end of file diff --git a/src-docs/src/views/range/range.js b/src-docs/src/views/range/range.js index 3762d8cc4cd..82bedc2fcfe 100644 --- a/src-docs/src/views/range/range.js +++ b/src-docs/src/views/range/range.js @@ -32,6 +32,7 @@ export default class extends Component { id={makeId()} min={100} max={200} + step={0.05} value={this.state.value} onChange={this.onChange} showLabels diff --git a/src-docs/src/views/range/range_example.js b/src-docs/src/views/range/range_example.js index 27ef79e7002..d92373dead0 100644 --- a/src-docs/src/views/range/range_example.js +++ b/src-docs/src/views/range/range_example.js @@ -113,6 +113,7 @@ export const RangeControlExample = { snippet: ` 0) { + if (!isEvenlyDivisibleBy(value - this.props.min, this.props.step !== undefined ? this.props.step : 1)) { + // if ((value - this.props.min) % this.props.step > 0) { throw new Error(`The value of ${value} is not included in the possible sequence provided by the step of ${this.props.step}.`); } // Return the value if nothing fails diff --git a/src/services/index.ts b/src/services/index.ts index 60faa4b159a..191507da4bc 100644 --- a/src/services/index.ts +++ b/src/services/index.ts @@ -33,7 +33,7 @@ export { formatText, } from './format'; -export { isWithinRange } from './number'; +export { isEvenlyDivisibleBy, isWithinRange } from './number'; export { Pager } from './paging'; diff --git a/src/services/number/number.test.tsx b/src/services/number/number.test.tsx index af8a51aa993..5bff8106c32 100644 --- a/src/services/number/number.test.tsx +++ b/src/services/number/number.test.tsx @@ -1,4 +1,4 @@ -import { isWithinRange } from './number'; +import { isEvenlyDivisibleBy, isWithinRange } from './number'; describe('numbers', () => { test('isWithinRange', () => { @@ -18,4 +18,49 @@ describe('numbers', () => { expect(isWithinRange(0, 100, -10)).toBe(false); expect(isWithinRange(0, 100, '')).toBe(false); }); + + describe('isEvenlyDivisibleBy', () => { + it('correctly computes for integers', () => { + expect(isEvenlyDivisibleBy(10, 1)).toBe(true); + expect(isEvenlyDivisibleBy(10, 2)).toBe(true); + expect(isEvenlyDivisibleBy(10, 10)).toBe(true); + + expect(isEvenlyDivisibleBy(10, 3)).toBe(false); + expect(isEvenlyDivisibleBy(1, 3)).toBe(false); + }); + + it('correctly computes for decimals', () => { + expect(isEvenlyDivisibleBy(1, 0.1)).toBe(true); + expect(isEvenlyDivisibleBy(1, 0.2)).toBe(true); + expect(isEvenlyDivisibleBy(1, 0.25)).toBe(true); + expect(isEvenlyDivisibleBy(1, 0.5)).toBe(true); + expect(isEvenlyDivisibleBy(1, 0.3)).toBe(false); + expect(isEvenlyDivisibleBy(1, 0.51)).toBe(false); + expect(isEvenlyDivisibleBy(1, 0.9)).toBe(false); + + expect(isEvenlyDivisibleBy(3, 0.5)).toBe(true); + expect(isEvenlyDivisibleBy(3, 1.5)).toBe(true); + expect(isEvenlyDivisibleBy(3, 0.61)).toBe(false); + expect(isEvenlyDivisibleBy(3, 2.9)).toBe(false); + + expect(isEvenlyDivisibleBy(2.75, 0.25)).toBe(true); + expect(isEvenlyDivisibleBy(2.75, 0.55)).toBe(true); + expect(isEvenlyDivisibleBy(2.75, 1.375)).toBe(true); + expect(isEvenlyDivisibleBy(2.75, 0.1)).toBe(false); + expect(isEvenlyDivisibleBy(2.75, 0.5)).toBe(false); + expect(isEvenlyDivisibleBy(2.75, 1.374)).toBe(false); + expect(isEvenlyDivisibleBy(2.75, 1.376)).toBe(false); + }); + + it('returns false when factor is 0', () => { + expect(isEvenlyDivisibleBy(1, 0)).toBe(false); + expect(isEvenlyDivisibleBy(100, 0)).toBe(false); + expect(isEvenlyDivisibleBy(-100, 0)).toBe(false); + expect(isEvenlyDivisibleBy(1.5, 0)).toBe(false); + }); + + it('handles known floating point error cases', () => { + expect(isEvenlyDivisibleBy(1, 0.05)).toBe(true); + }); + }); }); diff --git a/src/services/number/number.ts b/src/services/number/number.ts index 27e02f917c6..1a3943ab750 100644 --- a/src/services/number/number.ts +++ b/src/services/number/number.ts @@ -10,3 +10,22 @@ export const isWithinRange = ( const val = Number(value); return Number(min) <= val && val <= Number(max); }; + +// 1e-6 covers up to 10,000,000,000 factored by a decimal +const EPSILON = 1e-6; +export function isEvenlyDivisibleBy(num: number, factor: number) { + const remainder = num % factor; + + // due to floating point issues the remainder needs to be within a margin instead of exactly 0 + // 1 % 0.1 === 0.09999999999999995 + // 1000000000 % 0.1 === 0.09999994448884877 + // 1 % 0.05 === 0.04999999999999995 + + // Compare the smaller of (remainder, factor - remainder) to EPISOLON + return ( + Math.min( + remainder, // remainder may be smallest, it is 0 in the well-formed case + Math.abs(factor - remainder) // otherwise the positive difference between factor and remainder + ) < EPSILON + ); +} From f72c3d2aa914d5510702063d42545beb3104952b Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 6 Mar 2019 09:39:22 -0700 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa79be5d83e..bef0ac8defa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ - Added `.eui-fullWidth` utility class ([#1665](https://github.com/elastic/eui/pull/1665)) - Added `EuiPopoverFooter` and converted `EuiPopoverTitle` to TS ([#1666](https://github.com/elastic/eui/pull/1666)) +**Bug fixes** + +- Fixed floating point arithmetic bug in `EuiRangeTrack`'s value validation ([#1687](https://github.com/elastic/eui/pull/1687)) + ## [`9.0.1`](https://github.com/elastic/eui/tree/v9.0.1) **Bug fixes** From 527630f0155702087348c4838eab085a9b1b54a5 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Wed, 6 Mar 2019 10:10:22 -0700 Subject: [PATCH 3/3] PR feedback --- src/components/form/range/range_track.js | 1 - src/services/number/number.test.tsx | 4 ++++ src/services/number/number.ts | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/components/form/range/range_track.js b/src/components/form/range/range_track.js index a2c4d800b24..6ada9753f5e 100644 --- a/src/components/form/range/range_track.js +++ b/src/components/form/range/range_track.js @@ -20,7 +20,6 @@ export class EuiRangeTrack extends Component { } // Error out if the value doesn't line up with the sequence of steps if (!isEvenlyDivisibleBy(value - this.props.min, this.props.step !== undefined ? this.props.step : 1)) { - // if ((value - this.props.min) % this.props.step > 0) { throw new Error(`The value of ${value} is not included in the possible sequence provided by the step of ${this.props.step}.`); } // Return the value if nothing fails diff --git a/src/services/number/number.test.tsx b/src/services/number/number.test.tsx index 5bff8106c32..a52f9f93c02 100644 --- a/src/services/number/number.test.tsx +++ b/src/services/number/number.test.tsx @@ -37,6 +37,10 @@ describe('numbers', () => { expect(isEvenlyDivisibleBy(1, 0.3)).toBe(false); expect(isEvenlyDivisibleBy(1, 0.51)).toBe(false); expect(isEvenlyDivisibleBy(1, 0.9)).toBe(false); + expect(isEvenlyDivisibleBy(1000000, 0.00001)).toBe(true); + expect(isEvenlyDivisibleBy(1000000, 0.00002)).toBe(true); + expect(isEvenlyDivisibleBy(1000000, 0.00005)).toBe(true); + expect(isEvenlyDivisibleBy(15000000, 0.000075)).toBe(true); expect(isEvenlyDivisibleBy(3, 0.5)).toBe(true); expect(isEvenlyDivisibleBy(3, 1.5)).toBe(true); diff --git a/src/services/number/number.ts b/src/services/number/number.ts index 1a3943ab750..30000de3c5f 100644 --- a/src/services/number/number.ts +++ b/src/services/number/number.ts @@ -21,7 +21,7 @@ export function isEvenlyDivisibleBy(num: number, factor: number) { // 1000000000 % 0.1 === 0.09999994448884877 // 1 % 0.05 === 0.04999999999999995 - // Compare the smaller of (remainder, factor - remainder) to EPISOLON + // Compare the smaller of (remainder, factor - remainder) to EPSILON return ( Math.min( remainder, // remainder may be smallest, it is 0 in the well-formed case