-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
[No QA] method to randomly generate 64 bit int as string #10436
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add unit tests to this? 🤔
Co-authored-by: Carlos Martins <[email protected]>
…ikar-64bitRandom merge with origin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is 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 Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android