Add tests for GBR to validate unvalidated contact method#72463
Conversation
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@mananjadhav are you around to review? 😇 |
|
@mananjadhav is out sick. I can help if needed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
Codecov Report❌ Patch coverage is
... and 71 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@mountiny thanks for valid comments! I checked how tests are done for Subscription and came to conclusion that my initial solution wasn't the best one. In this case it's better to test functions calculating if GBR/RBR should be shown instead of rendering whole components to check if the dots are there. RBR/GRB displayed on the bottom tab is controlled by App/tests/unit/useAccountTabIndicatorStatusTest.ts Lines 1 to 366 in d057ac4 |
|
@situchan with this refactoring, could you please test and @jnowakow can add QA steps as now its not only about changing tests? I think in general I agree that we should have unit tests for the logic, but we should also make sure to test the UI is rendering correctly eventually, so not sure if its a golden rule to not have them at all |
|
@mountiny I've updated PR description with tests steps
I agree that UI tests are important, but I don't think they make much sense in this case because they would just test whether React correctly renders the component given certain props. I would say, the purpose of testing GBR/RBR is to verify that, given certain data, we can accurately determine which Brick Road indicator to display. This verification is primarily handled by the hooks and utility functions that are currently under test. |
|
BUG: Not able to close magic code input modal after failing to send new magic code and clicking error dismiss button Screen.Recording.2025-10-16.at.2.25.31.PM.mov(This also happens on staging so not blocker) |
|
BUG: weird navigation transition animations and flickers on android chrome bug.mov(This also happens on staging so not blocker) |
situchan
left a comment
There was a problem hiding this comment.
Screenshots/Videos
Android: HybridApp
android.mov
Android: mWeb Chrome
mchrome.mov
iOS: HybridApp
ios.mov
iOS: mWeb Safari
msafari.mov
MacOS: Chrome / Safari
web1.mov
web2.mov
MacOS: Desktop
desktop.mov
That is correct, the assumption there is though that people won't change the logic for rendering it without updating the tests - which can happen and could still introduce the regression because we only tested logic of one method but not the component itself |
|
@situchan CAn you please report that in #expensify-bugs so we can get that fixed |
Then I can restore UI tests from 4851d10#diff-3f080a042a6e353496a53c78a4ee838b2cff246c199534bd3262b62256b61123, fill them according to comments and open another PR to make sure we'll never find ourselves in such situation 😇 |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.33-4 🚀
|
Explanation of Change
Adds test suite that verifies that GBR is correctly displayed on
ContactMethodsPage.Fixed Issues
$ #69920
PROPOSAL: N/A
Tests
Automated tests:
npm run test tests/unit/UserUtilsTest.tsManual tests:
Onyx.merge('loginList', {"<email-you-added-in-step-4> ": {errorFields: {field: "someError"}}});Offline tests
Onyx.merge('loginList', {"<email-you-added-in-step-4> ": {errorFields: {field: "someError"}}});QA Steps
(If tests are done on staging)
Onyx.merge('loginList', {"<email-you-added-in-step-4> ": {errorFields: {field: "someError"}}});PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop