Automated tests for Unread Indicators feature and final polish by marcaaron · Pull Request #10929 · Expensify/App

marcaaron · 2022-09-09T17:12:34Z

Details

There are several changes introduced in this PR, but most of them exist to enable UI testing for the entire app.

Several modules needed to be mocked
Prevent editing or deleting of report actions that are pendingAction === 'add' (this is temporary but I noticed this feature is pretty broken so disabled it for now until we are totally migrated to using reportActionID for reportAction_* values keys)
Report sequenceNumber were being set to clientID which made it impossible to mark a "pending" comment as unread. Instead we use a guessed sequenceNumber for this. I suspect this may need to change in the future or depend on reportActionID as well.
When deleting a message we were not optimistically adding in the type field (Fixed)
Strike-through for deleted messages also was not working correctly as the message was entirely unset and when going offline there was no comment to show with the strike-through. The message is entirely lost when we do this so I moved it to a previousMessage field that we can use as a fallback.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/228076
$ #10799
$ #10753

Tests

Automated tests were added, but these testing flows laid out in the Unread Indicators doc should be performed.

Testing Unread LHN Status, “New Messages” badge, and New Line Indicator

Start a chat between User A and User B.
Ensure User A is not looking at the chat.
Send a message as User B
Verify the LHN row shows in bold for User A to indicate unread messages
Visit the chat as User A and verify the LHN row no longer shows in bold
Verify there is a “----- New” line above the message sent by User B
Scroll up and verify that the “New Messages” button appears floating above the content.
Verify that tapping this button will scroll you to the bottom of the chat but not clear the “---- New” line marker.
Mark a comment as “unread”
Verify the “---- New” line marker appears
Mark as unread comments above and below the existing new marker
Verify the marker appears in the correct position each time
While the new marker is active leave a comment as User A
Verify the new marker and new messages badge both disappear
Mark another comment as unread.
Navigate away from the chat
Navigate back to the chat
Verify the chat in the LHN shows as read
Verify the New Line marker and new messages badge is still present
Navigate away one more time and back
Verify the new marker and new messages badge both disappear
Mark another comment as unread.
Navigate away from the chat
Navigate back to the chat
Minimize the app and reopen it
Verify the new marker and new message badge both disappear
Switch to a mobile view (small screen) and mark a message as unread
Tap the header to reveal the LHN
Verify the chat in the LHN shows as “unread”
Tap the chat
Verify the new marker and badge are present
Tap the header to reveal the LHN
Verify the chat in the LHN shows as “read”
Tap the chat
Verify the new marker and badge are no longer present.
Scroll up in the chat so that last comment is out of view
Leave a comment as User B
Verify the “new messages badge” shows for User A
Scroll down and verify the “new line marker” shows for User A in the correct position.
Scroll back up and leave another comment as User B
Scroll down as User A
Verify the new marker is in the same position as it was before

Testing Deleted Comment Edge Case

Start a chat between User A and User B.
Ensure User A is not looking at the chat.
Send a message as User B.
Navigate to the chat as User A
Mark the most recent message as “unread”
As User B delete that message
Verify that the report no longer shows as “unread” for User A and that the new marker has disappeared.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Same as Tests above

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

melvin-bot · 2022-09-09T17:12:37Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

Add mock Fix UrbanAirship mock Get main drawer to render rename to navigation test Just render the sidebar links for now Fix tests and make ReportScreen render Add report action and verify the action renders Test for report-action-item visibility Mock disconnect for Pusher add way to set initial url for testing; Test for drawer status Test for the unread indicator Mock local notification and make sure only one notification appears upgrade test library dependencies and use act to get rid of weird warning undo test version bumps Fix tests Add comment Test behavior of unread indicator in report and sidebar Rename back to unread indicators Test new messages badge indicator refactor and improve Report screen visibility check Update mock for AppState Clear new line indicator when report page is in view and we return from background remove extra logs add JSDocs Check the LHN lastMessageText Fix ReportTest Check for pendingAction when deleting comment Fix deleted comments not showing strikethrough while offline Stop allowing deletion or editing of pendingAction add comments as users cannot be updated correctly via sequenceNumber Rollback some changes Use sequenceNumber for handledReportActions for now Remove log add doc

marcaaron · 2022-09-13T17:22:35Z

This one is testing well for me locally, but going to wait until the Auth PR for reportActionCount is deployed before taking off draft (as I am testing against that branch locally).

tgolen · 2022-09-13T18:40:47Z

+    Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, {
+        [USER_B_EMAIL]: TestHelper.buildPersonalDetails(USER_B_EMAIL, USER_B_ACCOUNT_ID, 'B'),
+    });
+    await waitForPromisesToResolve();


Again here, adding await to each of the merge() methods should allow this waitForPromisesToResolve() to be removed.

Ah in this case, I think we are not just waiting for Onyx.merge() but also some fun stuff that "eventually happens" after the merge.

Hm, maybe this is the cause to some of my test failures that I'm seeing on my new sidebarlinks PR. I need to explore it further.

marcochavezf

Nice! 🔥 Just a few comments

…docs

…nstead

…work. Use regular promises in the actual tests themselves

marcaaron · 2022-09-16T04:38:01Z

Reworked the tests here a bunch based on @tgolen's feedback. Feeling pretty good about the changes. The one thing I'm not 100% sure on is the usage of act(). When I tried to refactor it to not use async/await it stopped working and I saw errors that seemed to suggest we had to use async/await. Here's what I've got for an alternative...

App/tests/utils/waitForPromisesToResolveWithAct.js

Lines 1 to 21 in 7b76284

    
           import {act} from '@testing-library/react-native'; 
        
           import waitForPromisesToResolve from './waitForPromisesToResolve'; 
        
           /** 
        
            * This method is necessary because react-navigation's NavigationContainer makes an internal state update when parsing the 
        
            * linkingConfig to get the initialState. This throws some warnings related to async code if we do not wrap this call in an act(). 
        
            * 
        
            * See: https://callstack.github.io/react-native-testing-library/docs/understanding-act/#asynchronous-act 
        
            * 
        
            * This apparently will not work unless we use async/await because of some kind of voodoo magic inside the react-test-renderer 
        
            * so we are suppressing our lint rule and avoiding async/await everywhere else in our tests. 
        
            * 
        
            * @returns {Promise} 
        
            */ 
        
           // eslint-disable-next-line @lwc/lwc/no-async-await 
        
           export default async function waitForPromisesToResolveWithAct() { 
        
               // eslint-disable-next-line @lwc/lwc/no-async-await 
        
               await act(async () => { 
        
                   await waitForPromisesToResolve(); 
        
               }); 
        
           }

PauloGasparSv

My dev env is really buggy (not sending notifications correctly, duplicating messages, only working correctly while debugging) this past week so for now I'll approve the code for the changes I've read and I'll leave the tests for the other reviewers.

neil-marcellini

I've only looked at this a bit so far, but it's really cool! For now, I have a few questions:

neil-marcellini · 2022-09-16T16:43:52Z

+ *
+ * @returns {RenderAPI}
+ */
+function signInAndGetAppWithUnreadChat() {


NAB: For future tests it would be nice to have a signInAndGetApp function, maybe in TestHelpers.js. I imagine most ui tests in the future will need to start with that.

Great point. I think there's also a chance that a lot of UI tests could not need this since they might test more isolated individual components and not flows that require navigation. I think this would be good to add in a future PR though once there is a second test that needs this behavior.

tgolen

Wow, those tests are looking great now!

tgolen · 2022-09-16T19:11:55Z

@@ -0,0 +1 @@
+export default {};


Would it be OK if we also make a requirement that any mock file also should have an explanation for why it's mocked? I think that would be super helpful.

I think that sounds great. The answer won't always be the same so it is good context to add for sure and also will help people understand all the reasons why something could or should be mocked.

But also would say we can do that in a cleanup step.

tgolen · 2022-09-16T19:14:48Z

+// The main app uses a NativeModule called BootSplash to show/hide a splash screen. Since we can't use this in the node environment
+// where tests run we simulate a behavior where the splash screen is always hidden (similar to web which has no splash screen at all).


❤️ I love this comment. It makes it so clear why the mock is necessary!

tgolen · 2022-09-16T19:28:34Z

+                // Scroll and verify that the new messages badge is also hidden
+                scrollUpToRevealNewMessagesBadge(renderedApp);
+                expect(isNewMessagesBadgeVisible(renderedApp)).toBe(false);
+            });


This looks great with the promises!!

tgolen · 2022-09-16T19:31:17Z

+                Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, {
+                    [USER_C_EMAIL]: TestHelper.buildPersonalDetails(USER_C_EMAIL, USER_C_ACCOUNT_ID, 'C'),
+                });
+                return waitForPromisesToResolve();


Something else I did in the LHN tests was to use return Onyx.multiSet() which would prevent you from needing to deal with three separate promises here and can remove another waitForPromisesToResolve.

But in that case, I think we also need to wrap them in act() to ensure all the component tree is updated properly before making assertions. I have yet to test that out fully.

That also assume that using set() is OK (which I think it usually is for tests).

Gonna leave this one - mainly because having a few different merges happen seems closer to what would happen in the UI.

tgolen · 2022-09-16T19:35:30Z

+import {act} from '@testing-library/react-native';
+import waitForPromisesToResolve from './waitForPromisesToResolve';
+
+/**


I love this comment, but what's not clear to me is when it should be used. Is there some guidance that can be added for that? Like... "Use when..." and "Do not use when..."

marcaaron · 2022-09-16T20:48:12Z

Updated. Thanks for the additional comments @tgolen and @neil-marcellini

melvin-bot · 2022-09-16T20:54:13Z

npm has a package.json file and a package-lock.json file. It seems you updated one without the other, which is usually a sign of a mistake. If you are updating a package make sure that you update the version in package.json then run npm install

tgolen

Looks great!

@marcochavezf

Gonna go for the merge here as it did not seem like there was any major pushback from @marcochavezf or @neil-marcellini

melvin-bot · 2022-09-16T22:51:06Z

@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

marcaaron · 2022-09-16T22:52:18Z

Oh shoot are we supposed to add a reviewer checklist in a comment or edit the description? I’m not sure 🥲

This was not an emergency. Just the contributor checklist check.

OSBotify · 2022-09-19T16:47:55Z

🚀 Deployed to staging by @marcaaron in version: 1.2.2-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-09-20T14:23:20Z

🚀 Deployed to production by @luacmartins in version: 1.2.2-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron self-assigned this Sep 9, 2022

marcaaron changed the title ~~[WIP] automated navigation e2e testing (exploration)~~ [WIP] automated navigation ui testing (exploration) Sep 11, 2022

marcaaron changed the title ~~[WIP] automated navigation ui testing (exploration)~~ [WIP] Automated tests for Unread Indicators feature Sep 12, 2022

marcaaron force-pushed the marcaaron-automatedUnreadTest branch from ca20849 to b199711 Compare September 13, 2022 14:16

remove unused import

f1dc534

marcaaron changed the title ~~[WIP] Automated tests for Unread Indicators feature~~ Automated tests for Unread Indicators feature and final polish Sep 13, 2022

neil-marcellini self-requested a review September 13, 2022 17:32

tgolen requested changes Sep 13, 2022

View reviewed changes

marcochavezf previously requested changes Sep 13, 2022

View reviewed changes

Comment thread src/pages/home/report/ReportActionsList.js

Comment thread tests/ui/UnreadIndicatorsTest.js Outdated

Comment thread tests/ui/UnreadIndicatorsTest.js Outdated

Comment thread tests/utils/TestHelper.js Outdated

marcaaron added 12 commits September 14, 2022 11:31

Make requested changes

3e90f5e

Remove docs from react-navigation setup and link to react-navigation …

973de5c

…docs

Clean up all testID and use accessibilityHint or accessibilityLabel i…

261c712

…nstead

Use accessibilityElementsHidden

ac9fe71

fix conflicts

8fc5b95

Undo unnecessary refactor of back button

68fed0c

Remove jest.setTimeout() by breaking test up into smaller chunks

efb1d62

Extract the one part of tests that actually does need async/await to …

1836742

…work. Use regular promises in the actual tests themselves

Cleanup Expensicons mock

c350685

Clean up function names

f51ed16

Add test for deleting comment

ff196b1

improve a couple of comments and fix some incorrect changes

7b76284

marcaaron marked this pull request as ready for review September 16, 2022 04:34

marcaaron requested a review from a team as a code owner September 16, 2022 04:34

marcaaron requested review from marcochavezf and tgolen September 16, 2022 04:34

melvin-bot Bot requested review from PauloGasparSv and removed request for a team September 16, 2022 04:35

marcaaron added 4 commits September 15, 2022 19:12

Merge remote-tracking branch 'origin' into marcaaron-automatedUnreadTest

7fedd6e

Must mock useDrawerStatus in LHNOrderTest

d4fbfd6

Fix icons and withDrawerState for LHNOrderTest

b6d9615

improve comment and add explanation for icon name setting

b3ab1ba

PauloGasparSv reviewed Sep 16, 2022

View reviewed changes

Comment thread src/libs/actions/Report.js

PauloGasparSv previously approved these changes Sep 16, 2022

View reviewed changes

neil-marcellini reviewed Sep 16, 2022

View reviewed changes

marcaaron mentioned this pull request Sep 16, 2022

Deleted messages are properly gone from the UI without re-appearing briefly #10892

Closed

93 tasks

tgolen requested changes Sep 16, 2022

View reviewed changes

Make some requested changes and improve comments further

6b8536a

marcaaron dismissed PauloGasparSv’s stale review via 6b8536a September 16, 2022 20:38

marcaaron requested review from neil-marcellini and tgolen September 16, 2022 20:44

tgolen approved these changes Sep 16, 2022

View reviewed changes

marcaaron merged commit 4562b5c into main Sep 16, 2022

marcaaron deleted the marcaaron-automatedUnreadTest branch September 16, 2022 22:51

melvin-bot Bot added the Emergency label Sep 16, 2022

marcaaron removed the Emergency label Sep 16, 2022

OSBotify mentioned this pull request Sep 19, 2022

Deploy Checklist: New Expensify 2022-09-19 #11108

Closed

8 tasks

sobitneupane mentioned this pull request Sep 22, 2022

Android - Console error on login page when Phone or email text-input is focused. - reported by @aneequeahmad #11194

Closed

trjExpensify mentioned this pull request Oct 31, 2022

[HOLD for payment 2022-11-29] [$1000] BUG: The text inside the composer input and the menu on the settings page got highlighted when triple-click on the last report chat reported by @mollfpr #12028

Closed

eVoloshchak mentioned this pull request Nov 28, 2022

[HOLD for payment 2022-11-24] [$250] BUG: when deleting the last message of a conversation (own messages) the chat is marked as unread for a moment #11688

Closed

		// The main app uses a NativeModule called BootSplash to show/hide a splash screen. Since we can't use this in the node environment
		// where tests run we simulate a behavior where the splash screen is always hidden (similar to web which has no splash screen at all).

Conversation

marcaaron commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

melvin-bot Bot commented Sep 9, 2022

Uh oh!

marcaaron commented Sep 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcochavezf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcaaron commented Sep 16, 2022

Uh oh!

Uh oh!

PauloGasparSv left a comment

Choose a reason for hiding this comment

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

marcaaron commented Sep 9, 2022 •

edited

Loading