Skip to content

fix(number): keep decimal multipleOf floats exact and within bounds - #4162

Open
kevin9327 wants to merge 5 commits into
faker-js:nextfrom
kevin9327:fix/float-decimal-multipleof
Open

kevin9327 wants to merge 5 commits into
faker-js:nextfrom
kevin9327:fix/float-decimal-multipleof

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

faker.number.float() with a decimal multipleOf that is not a power of ten (0.3, 0.25, 1.5, ...) divides by 1 / multipleOf. Floating-point error in that division gives values that are not the intended multiples, and some of them fall below min:

faker.number.float({ min: 0.9, max: 1.8, multipleOf: 0.3 });
// can return 0.8999999999999999 (< min) and 1.7999999999999998

For a decimal step, this change scales by the step's own number of decimal places instead. It picks an integer multiple of the scaled step, then divides once, so 3 × 0.3 becomes 9 / 10 = 0.9. Steps of the form 10^-n and non-decimal steps keep the existing code path. The new test fails on next and passes with the fix.

AI-assisted: I directed the investigation and reviewed and verified the change myself.

@kevin9327
kevin9327 requested a review from a team as a code owner October 19, 2026 03:52
@netlify

netlify Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fbf3f64
🔍 Latest deploy log https://app.netlify.com/projects/fakerjs/deploys/6ad75bc714e3cb156a3cb830
😎 Deploy Preview https://deploy-preview-4162.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.14%. Comparing base (dc60fc2) to head (fbf3f64).

Files with missing lines Patch % Lines
src/modules/number/float.ts 90.90% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             next    #4162   +/-   ##
=======================================
  Coverage   99.13%   99.14%           
=======================================
  Files        1177     1177           
  Lines        3939     3957   +18     
  Branches      697      717   +20     
=======================================
+ Hits         3905     3923   +18     
+ Misses         30       29    -1     
- Partials        4        5    +1     
Files with missing lines Coverage Δ
src/modules/number/module.ts 100.00% <ø> (ø)
src/modules/number/float.ts 95.55% <90.90%> (-4.45%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@matthewmayer

Copy link
Copy Markdown
Contributor

Although this handles “nice” literals like 0.3 or 1.5 with matching bounds, it isn’t a full fix, and it can regress other bounds. For example:

faker.number.float({ min: 1.1, max: 1.3, multipleOf: 0.05 })
// → [1.15, 1.2, 1.25, 1.3]  // 1.1 is now missing; it was previously reachable

String(multipleOf).split('.') also feels fragile — e.g. 1e-7 doesn’t stringify with a .

For a more general fix I’d:

  • Detect when min, max, and multipleOf are finite-length decimals (i.e. not values like Math.PI or 1/3)
  • Find a shared integer scale, multiply everything by it, and round
  • Call faker.number.int(), then divide once

@ST-DDT

ST-DDT commented Oct 19, 2026

Copy link
Copy Markdown
Member

I also considered "tricking" the engine to collapse these values by itself.

E.g. by using +1-1 or /10*10

But I havent looked any deeper into this yet.

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent s: pending triage Pending Triage m: number Something is referring to the number module labels Oct 19, 2026
@kevin9327

kevin9327 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

@matthewmayer you're right on both points. Reworked in f31bab0 along the lines you suggested:

  • decimalPlaces() finds each value's number of decimal places numerically, as the smallest d (up to 15) with Math.round(x * 10^d) / 10^d === x, so 3e-7 gives 7 and no string parsing is involved. Math.PI and 1 / 3 have no finite form and return Infinity.
  • When min, max and multipleOf are all finite decimals, they're scaled by the shared power of ten, rounded to integers, passed to number.int, and the result is divided once. Your example { min: 1.1, max: 1.3, multipleOf: 0.05 } now yields [1.1, 1.15, 1.2, 1.25, 1.3], and there's a test for it.
  • 10^-n steps (and so fractionDigits), integer-only inputs and non-decimal values keep the existing path, so their output and error messages don't change.

@ST-DDT re the "+1-1" / "/10*10" idea: I think that would only round away the representation error after the fact. It wouldn't bring back a bound that ceil(min * factor) already skipped (1.1 in the example above), which scaling all three values together does.

Comment thread src/modules/number/float.ts Outdated
@ST-DDT ST-DDT removed the s: pending triage Pending Triage label Oct 19, 2026
@ST-DDT ST-DDT added this to the v11.0 milestone Oct 19, 2026
Comment thread src/modules/number/float.ts
Comment thread src/modules/number/float.ts Outdated
Comment thread src/modules/number/float.ts Outdated
Comment thread src/modules/number/float.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: bug Something isn't working m: number Something is referring to the number module p: 1-normal Nothing urgent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants