Skip to content

fix(number): avoid rounding past inclusive upper bounds - #4145

Merged
ST-DDT merged 4 commits into
faker-js:nextfrom
shin4141:codex/faker-number-int-bounds
Oct 24, 2026
Merged

ST-DDT merged 4 commits into
faker-js:nextfrom
shin4141:codex/faker-number-int-bounds

Conversation

@shin4141

Copy link
Copy Markdown
Contributor

Summary

number.int() can exceed its inclusive max when adding a nonzero lower bound rounds the intermediate floating-point value up before Math.floor runs. For example, faker.number.int({ min: 1, max: 2, distributor: () => 1 - Number.EPSILON / 2 }) returns 3 on next before this change.

Flooring the scaled distributor result before adding the integer lower bound avoids that intermediate rounding. number.float() with multipleOf uses the same integer path, so it benefits from the fix as well. The change is one runtime line and two regression tests; it does not change the public API.

Tests

  • The two new regression tests fail on the original next implementation (3 instead of 2, and 1.5 instead of 1).
  • With this change, test/modules/number.spec.ts passes all 301 tests.
  • pnpm run preflight passes on Node 24.19.0 / pnpm 12.8.2: 56 test files passed, 54,952 tests passed, 484 expected failures, 687 skipped; TypeScript checks pass. This local run used Linux arm64. The first attempt used a shallow clone without release tags, which caused only the docs-version test to fail; fetching the release tags and rerunning the same preflight passed.

Scope

The regression cases cover a small offset range, a multipleOf range, a range near Number.MAX_SAFE_INTEGER, and a constrained float. This change does not add validation for out-of-contract custom distributors or numeric inputs beyond the tested cases. Hosted CI and maintainer review remain pending until a PR is opened.

@shin4141
shin4141 requested a review from a team as a code owner October 11, 2026 21:10
@netlify

netlify Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 827ba9e
🔍 Latest deploy log https://app.netlify.com/projects/fakerjs/deploys/6ada6ebfec71140f534a92cb
😎 Deploy Preview https://deploy-preview-4145.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.

@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 11, 2026
@codecov

codecov Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.18%. Comparing base (7422b5c) to head (827ba9e).
⚠️ Report is 1 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #4145   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files        1178     1178           
  Lines        3940     3940           
  Branches      697      711   +14     
=======================================
  Hits         3908     3908           
  Misses         28       28           
  Partials        4        4           
Files with missing lines Coverage Δ
src/modules/number/int.ts 100.00% <100.00%> (ø)
🚀 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.

ST-DDT
ST-DDT previously approved these changes Oct 11, 2026

@ST-DDT ST-DDT left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nicely spotted. ❤️

FFR:

1 - Number.EPSILON / 2 // 0.9999999999999999 // 1 - 2^-53
1 - Number.EPSILON  // 0.9999999999999998

Reproduction code:

await enableFaker();
faker.number.int({ min: 1, max: 2, distributor: () => 1 - Number.EPSILON / 2 }); // Previously 3, now 2
faker.number.int({ min: 1, max: 2, distributor: () => 1 - Number.EPSILON }); // 2 as before
faker.number.int({ min: 1, max: 3, distributor: () => 1 - Number.EPSILON / 2 }); // 3 as before

I was unable to find other edge cases for this, as this only seems to affect max - min = 1 cases and randomizer.next() = MAX and min != 0.

Comment thread test/modules/number.spec.ts Resolved
Comment thread test/modules/number.spec.ts Resolved
@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: pending triage Pending Triage labels Oct 11, 2026
@ST-DDT ST-DDT added this to the v11.0 milestone Oct 11, 2026
@ST-DDT

ST-DDT commented Oct 14, 2026

Copy link
Copy Markdown
Member

@shin4141 Can you please address the above comments?

@shin4141

Copy link
Copy Markdown
Contributor Author

Thanks for the pointers. I updated nearOne to return the existing MERSENNE_MAX_VALUE and reused nearOne in the Number.MAX_SAFE_INTEGER case. The production number.int() code is unchanged. I kept the constrained-float test's explicit near-one value as an independent regression check.

With the pre-fix int.ts, the two targeted tests still fail (3 versus 2, and 1.5 versus 1). With the PR implementation, the number test file passes 301/301 tests. The full local preflight also passes under Node 24.19.0 and pnpm 12.8.2 (56/56 test files, 54,952 tests passed, no type errors).

@ST-DDT ST-DDT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your comment above sounds a bit AI-ish.
Please trim it down to only relevant information.

Or: Dont force us to read something, you didnt bother writing.

Comment thread test/modules/number.spec.ts Resolved
@ST-DDT
ST-DDT added this pull request to the merge queue Oct 24, 2026
Merged via the queue into faker-js:next with commit 73d4104 Oct 24, 2026
23 checks passed
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 s: accepted Accepted feature / Confirmed bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants