[No QA] method to randomly generate 64 bit int as string#10436
Conversation
flodnv
left a comment
There was a problem hiding this comment.
Can we add unit tests to this? 🤔
Co-authored-by: Carlos Martins <cmartins@expensify.com>
…ikar-64bitRandom merge with origin
stitesExpensify
left a comment
There was a problem hiding this comment.
LGTM! We can ignore the linter error since we know it's not getting used yet
|
@flodnv I tried to think of some unit tests for this, but couldn't come up with anything useful. Did you have anything in mind? |
|
We could at least verify that the generated number is in the range we expect it to be (and is a string) |
|
I think the lowest number we can get with this function is 1,000,000,000. I don't think it matters that much since 64 bits is very very big but I don't know if this was discussed so I wanted to bring it up. @tylerkaraszewski |
|
Good point! I think we should add first and then multiply on the left part, so the range is 0-MAX_64BIT_LEFT_HALF. For the right part we could multiply and then add so the range is 1-MAX_64BIT_RIGHT_HALF |
|
@srikarparsi can we add unit tests as Flo suggested? |
|
@srikarparsi I wanted to check in on this PR? We've had some great process thus far, so let's do everything we can to keep that same momentum this week. When do you think it'll be ready for re-review? |
|
I'll add unit tests, clean up the warnings and change the bounds by the end of today :) |
…ikar-64bitRandom fast forward
|
Updated! All comments addressed |
roryabraham
left a comment
There was a problem hiding this comment.
LGTM, I'm not worried about using BigInt in tests. If it passes locally and passes in GitHub Actions, then we're good.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
hmm this was breaking when I was running the desktop locally. Could be me though? |
|
@ctkochan22 there's a thread here if you want to follow |
|
🚀 Deployed to staging by @luacmartins in version: 1.1.90-0 🚀
|

Details
New Dot section of design doc: https://docs.google.com/document/d/125xZPDi7v6jmS-GKEcvcpdAwpDRPPGsq_iEOCl9RQFw/edit#bookmark=id.3ifommrba4g8
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/223137
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android