Skip to content

test: enable test to generate Number.MAX_SAFE_INTEGER #3393

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.

Sign up for GitHub

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

Jump to bottom
Merged
ST-DDT merged 2 commits into from
Feb 9, 2025
Merged

test: enable test to generate Number.MAX_SAFE_INTEGER #3393

ST-DDT merged 2 commits into from
Feb 9, 2025

Conversation

@ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Feb 6, 2025 •
edited
Loading

I checked the codebase for todos and found this disabled test, so I fixed and enabled it.

The tests have been added in v8.4.0

And should have been enabled in v9.0.0-alpha.0 two months later

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: test m: number Something is referring to the number module labels Feb 6, 2025
@ST-DDT ST-DDT added this to the vAnytime milestone Feb 6, 2025
@ST-DDT ST-DDT requested review from a team February 6, 2025 23:56
@ST-DDT ST-DDT self-assigned this Feb 6, 2025
@netlify
Copy link

netlify bot commented Feb 6, 2025 •
edited
Loading

✅ Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 714db7e
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/67a8fe83d7d7740008a8f442
😎 Deploy Preview https://deploy-preview-3393.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@codecov
Copy link

codecov bot commented Feb 6, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (9e13953) to head (714db7e).
Report is 1 commits behind head on next.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #3393      +/-   ##
==========================================
- Coverage   99.97%   99.97%   -0.01%     
==========================================
  Files        2811     2811              
  Lines      216965   216965              
  Branches      941      940       -1     
==========================================
- Hits       216914   216912       -2     
- Misses         51       53       +2

see 1 file with indirect coverage changes

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 7, 2025

Just a little thingy I observed here in the PR: Looks like the Randomizer#next function is not sealed, at least not at TypeScript level 🤔 Should we think about it to mark it as readonly so someone needs to explicitly use // @ts-expect-error: force override?

@ST-DDT
Copy link
Member Author

ST-DDT commented Feb 7, 2025

We could do that 🤷‍♂️ , but we havent sealed any other object/module either.

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 7, 2025 •
edited
Loading

We could do that 🤷‍♂️ , but we havent sealed any other object/module either.

👀

@Shinigami92
Copy link
Member

Shinigami92 commented Feb 7, 2025

We could do that 🤷‍♂️ , but we havent sealed any other object/module either.

[img] 👀

I played a bit around with it, and it makes to many bad side-effects in tests, snapshots and other things like auto-doc-get. So I decided against touching it. 👌

@ST-DDT ST-DDT enabled auto-merge (squash) February 9, 2025 19:14
@ST-DDT ST-DDT merged commit 9c42195 into next Feb 9, 2025
23 checks passed
@ST-DDT ST-DDT deleted the test/number/return-max_safe_integer branch February 9, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@xDivisionByZerox xDivisionByZerox xDivisionByZerox approved these changes

Assignees

@ST-DDT ST-DDT

Labels

c: test m: number Something is referring to the number module p: 1-normal Nothing urgent

Projects

None yet

Milestone

vAnytime

Development

Successfully merging this pull request may close these issues.

4 participants

@ST-DDT @Shinigami92 @xDivisionByZerox