[No QA] Add Pusher Automated Tests #1153

marcaaron · 2021-01-01T18:42:53Z

Details

Second attempt to make Pusher tests. Closed the first PR here #771

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/145497

Tests

Automated

Tested On

❌

Screenshots

❌

github-actions · 2021-01-01T18:43:08Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

src/libs/actions/Report.js

AndrewGable

Looking a whole lot better! Adding @tgolen for review since he had comments on the last PR.

tgolen

This looks like a good use for the tests, but I'm concerned that it's only testing half the flow. What about including more of the flow such as, using Report.addAction(), testing that an optimistic action is created in Onyx, then the pusher event happens, and verify that the optimistic action was replaced by the real action?

This comment is more of a NAB, but I also really like when tests conform to the format:

// Given <initial_condition>
...  code that sets initial condition
// When <something_happens>
... code that does something
// Then <expectation>
... code that performs the assertion

I know we use this pattern a lot in Chronos and I think it's really great because it provides lots of context for someone else that has to come in to maintain the test and understand the "why" behind everything that was done. Since we only have a few tests written so far, I think it would be a good time to adopt that same format, but if you are not sold on it, I can pull it out into a separate discussion.

tgolen · 2021-01-11T21:08:07Z

tests/actions/ReportTest.js

+ Onyx.set(ONYXKEYS.SESSION, {accountID: 1, email: 'test@test.com'});
+ return waitForPromisesToResolve()


I still haven't quite gotten the hang of these, so forgive me if this isn't how it's meant to be done... is the test that has been written any different than doing this?

return Onyx.set(ONYXKEYS.SESSION, {accountID: 1, email: 'test@test.com'})

My thinking is just that it's odd to see waitForPromisesToResolve() being used when the promise returned from Onyx.set() should resolve and work as well (and it's more obvious what is happening because the test uses the same promise chain).

That makes sense! And I wouldn't mind updating it, but want to walk you through my thought process here to see if you have similar thoughts...

We could wait for Onyx.set() instead of doing waitForPromisesToResolve(), but I was thinking that for consistency we might want to default to waitForPromisesToResolve(). Even though unnecessary it might make it easier to understand how these tests are structured and deal with async code that doesn't necessarily have an "await-able" promise. Maybe this practice actually makes it harder to tell that's what waitForPromisesToResolve() is for?

Another thought I keep having is that if "everything async returned a Promise" (or even just the async "actions") then the test code might be more straight-forward and it would be more obvious to tell what exactly we are waiting for.

I'll change this for now since I think I did have it this way at one point.

OK, you brought up some really good points. What I'd like to avoid is creating a situation where people get too promise-happy and just return promises from everything. That essentially promotes a ton of promise chains, and that was what plagued the application from an early age. I had to do a lot of work to remove all those promise chains and I'd hate to go back down that path. So, now that I think about it, I think your original solution here is actually superior.

tests/actions/ReportTest.js

tgolen · 2021-01-11T21:10:01Z

tests/actions/ReportTest.js

+
+ // Once this happens we should see the comment get processed by the callback and added to the
+ // storage so we must wait for promises to resolve again and then verify the data is in Onyx.
+ return waitForPromisesToResolve();


I can see how maybe this is necessary to prevent any race conditions with the local data stored in reportActions not being up-to-date with Onyx yet.

That's right. The callback should happen in realtime and then we set the data async via Onyx.

tests/actions/ReportTest.js

marcaaron · 2021-01-12T06:31:02Z

Alright, so I started to write a much better test for Report.addAction(), but it was so much better than it ended up revealing a bug in react-native-onyx. Fixed the bug over there and also cleaned up the main tests in that repo.

https://github.com/Expensify/react-native-onyx/pull/36/files

I know we use this pattern a lot in Chronos and I think it's really great because it provides lots of context for someone else that has to come in to maintain the test and understand the "why" behind everything that was done

Yeah that does sound pretty organized. I'll check out the Chronos tests and investigate.

tgolen · 2021-01-12T16:22:42Z

tests/actions/ReportTest.js

+
+ // When using the Pusher mock the act of calling Pusher.isSubscribed will create a
+ // channel already in a subscribed state. These methods are normally used to prevent
+ // duplicated subscriptions, but we don't need them for this test.


Suggested change

// duplicated subscriptions, but we don't need them for this test.

// duplicated subscriptions, but we don't need them for this test so forcing them to return false will make the testing less complex.

This adds a little more clarity.

tgolen · 2021-01-12T16:23:44Z

tests/actions/ReportTest.js

+ },
+ }))
+ .then(() => {
+ // This is a fire and forget response, but one it completes we should be able to verify that we


Suggested change

// This is a fire and forget response, but one it completes we should be able to verify that we

// This is a fire and forget response, but once it completes we should be able to verify that we

tgolen · 2021-01-12T16:27:10Z

tests/actions/ReportTest.js

+ expect(resultAction).toEqual(REPORT_ACTION);
+ })
+ .then(() => {
+ // Now that we are subscribed we need to simulate a reportComment action Pusher event.


// Now that we are subscribed

Subscribed to what? Is this referring to the original Pusher subscription or to Onyx? This reads a little weird because I would expect the thing before this to be code that does subscribing, but the thing before this is adding the report action.

I can make this more explicit.

It probably made more sense before when the thing immediately before this was subscribeToReportComments()

tgolen · 2021-01-12T16:28:06Z

tests/actions/ReportTest.js

+ reportAction: REPORT_ACTION,
+ });
+
+ // Once this happens we should see the comment get processed by the callback and added to the


// Once this happens

"this" could be referring to a number of things, so be more explicit

the callback

This is also ambiguous. Is it the pusher callback, the onyx callback, or a different callback entirely?

I can make this more explicit.

tgolen · 2021-01-12T16:31:39Z

tests/actions/ReportTest.js

+
+ // Verify that our action is no longer in the loading state
+ REPORT_ACTION.loading = false;
+ expect(resultAction).toEqual(REPORT_ACTION);


Using a different assertion might make it a little more clear what's being tested and prevents the need to modify the contents of REPORT_ACTION (which is not quite intuitive as to why it's necessary to modify REPORT_ACTION).

Suggestion:

expect(resultAction.loading).toEqual(false);

Yeah this is much better

tgolen · 2021-01-12T16:36:01Z

tests/actions/ReportTest.js

+ })
+ .then(() => {
+ const resultAction = reportActions[ACTION_ID];
+ expect(resultAction).toEqual(REPORT_ACTION);


Adding this assertion will help identify that yes, this is definitely an optimistic comment, and not the final result.

expect(resultAction.loading).toEqual(true);

This, of course, is already implied in the current assertion, but it's an extra mental jump that has to be made which isn't totally obvious.

Yep that makes sense to me. I think it'll be a more interesting test when the clientID stuff comes along. But this is mostly what we are looking for now and proves that the action was handled from the Pusher payload.

Yeah, this will look cool once the clientID stuff happens, and it will be a perfect use for these tests!

tgolen

Looks great, thanks for those improvements!

marcaaron · 2021-01-14T02:05:19Z

Taking this one off hold .

marcaaron · 2021-01-14T04:57:05Z

Also added a README over here if y'all are interested

#1236

AndrewGable

Great improvement 👍

marcaaron and others added 13 commits November 6, 2020 08:46

change name

f0a9e60

fix created being in the wrong order

1c37dd5

fix style

d4e9ef1

fix style then get rid of async

29d02a1

fix style

5970909

Merge remote-tracking branch 'origin' into marcaaron-pusher-tests

c409ac3

fix tests to use Onyx

ccc3778

fix conflicts

68f1d37

fix API.js

478f094

tests broke somehow

918fa0a

fix conflicts

781f82b

Fix this test up

da7746e

remove some things

f65b202

marcaaron self-assigned this Jan 1, 2021

add comment

fa53652

marcaaron requested review from AndrewGable January 1, 2021 18:49

marcaaron marked this pull request as ready for review January 1, 2021 18:50

marcaaron requested a review from a team as a code owner January 1, 2021 18:50

botify requested review from nickmurray47 and removed request for a team January 1, 2021 18:50

marcaaron requested review from a team and removed request for nickmurray47 January 1, 2021 18:50

botify requested review from nickmurray47 and removed request for a team January 1, 2021 18:50

marcaaron removed the request for review from nickmurray47 January 1, 2021 18:50

fix conflicts

6d59757

AndrewGable reviewed Jan 7, 2021

View reviewed changes

src/libs/actions/Report.js Outdated Show resolved Hide resolved

fix conflicts

e8e0b3c

AndrewGable previously approved these changes Jan 11, 2021

View reviewed changes

AndrewGable requested a review from tgolen January 11, 2021 18:21

tgolen requested changes Jan 11, 2021

View reviewed changes

marcaaron added 2 commits January 11, 2021 17:12

Merge remote-tracking branch 'origin' into marcaaron-pusher-tests

7298525

Fix up comment. Use Onyx.set instead of waitForPromisesToResolve()

cd18cdb

marcaaron dismissed AndrewGable’s stale review via cd18cdb January 12, 2021 03:32

Improve tests. Add TestHelper.

2ae40c9

marcaaron changed the title ~~[No QA] Add Pusher Automated Tests~~ [HOLD][No QA] Add Pusher Automated Tests Jan 12, 2021

fix lint

fb69894

tgolen requested changes Jan 12, 2021

View reviewed changes

Respond to Tims feedback.

84bbfb6

tgolen previously approved these changes Jan 12, 2021

View reviewed changes

update react-native-onyx

422ae85

marcaaron dismissed tgolen’s stale review via 422ae85 January 13, 2021 22:01

marcaaron changed the title ~~[HOLD][No QA] Add Pusher Automated Tests~~ [No QA] Add Pusher Automated Tests Jan 13, 2021

marcaaron added 2 commits January 13, 2021 12:57

dont test strict equal since the timestamp will make the test flaky

739bbb7

remove momnet

fefa0ef

marcaaron requested review from AndrewGable and tgolen January 14, 2021 02:04

AndrewGable previously approved these changes Jan 14, 2021

View reviewed changes

marcaaron added 2 commits January 15, 2021 14:58

fix conflicts

b895dce

add comment

acb4b80

marcaaron dismissed AndrewGable’s stale review via acb4b80 January 16, 2021 01:00

AndrewGable approved these changes Jan 18, 2021

View reviewed changes

AndrewGable merged commit d1cff08 into master Jan 18, 2021

AndrewGable deleted the marcaaron-pusher-tests branch January 18, 2021 19:32

github-actions bot locked and limited conversation to collaborators Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[No QA] Add Pusher Automated Tests #1153

[No QA] Add Pusher Automated Tests #1153

marcaaron commented Jan 1, 2021

github-actions bot commented Jan 1, 2021 •

edited

Loading

AndrewGable left a comment

tgolen left a comment

tgolen Jan 11, 2021

marcaaron Jan 12, 2021

marcaaron Jan 12, 2021

tgolen Jan 12, 2021

tgolen Jan 11, 2021

marcaaron Jan 12, 2021

marcaaron commented Jan 12, 2021

tgolen Jan 12, 2021

tgolen Jan 12, 2021

tgolen Jan 12, 2021

marcaaron Jan 12, 2021

marcaaron Jan 12, 2021

tgolen Jan 12, 2021

tgolen Jan 12, 2021

marcaaron Jan 12, 2021

tgolen Jan 12, 2021

marcaaron Jan 12, 2021

tgolen Jan 12, 2021

marcaaron Jan 12, 2021

tgolen Jan 12, 2021

tgolen left a comment

marcaaron commented Jan 14, 2021

marcaaron commented Jan 14, 2021

AndrewGable left a comment

		Onyx.set(ONYXKEYS.SESSION, {accountID: 1, email: 'test@test.com'});
		return waitForPromisesToResolve()

	// duplicated subscriptions, but we don't need them for this test.
	// duplicated subscriptions, but we don't need them for this test so forcing them to return false will make the testing less complex.

	// This is a fire and forget response, but one it completes we should be able to verify that we
	// This is a fire and forget response, but once it completes we should be able to verify that we

[No QA] Add Pusher Automated Tests #1153

[No QA] Add Pusher Automated Tests #1153

Conversation

marcaaron commented Jan 1, 2021

Details

Fixed Issues

Tests

Tested On

Screenshots

github-actions bot commented Jan 1, 2021 • edited Loading

AndrewGable left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

marcaaron commented Jan 14, 2021

marcaaron commented Jan 14, 2021

AndrewGable left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2021 •

edited

Loading