Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Add Pusher Automated Tests #1153
[No QA] Add Pusher Automated Tests #1153
Changes from 21 commits
f0a9e60
1c37dd5
d4e9ef1
29d02a1
5970909
c409ac3
ccc3778
68f1d37
478f094
918fa0a
781f82b
da7746e
f65b202
fa53652
6d59757
e8e0b3c
2f8dae6
39684fe
9704aa9
c6a235a
8e55d77
7298525
cd18cdb
2ae40c9
fb69894
84bbfb6
422ae85
739bbb7
fefa0ef
b895dce
acb4b80
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
This adds a little more clarity.
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.
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?
My thinking is just that it's odd to see
waitForPromisesToResolve()
being used when the promise returned fromOnyx.set()
should resolve and work as well (and it's more obvious what is happening because the test uses the same promise chain).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.
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 doingwaitForPromisesToResolve()
, but I was thinking that for consistency we might want to default towaitForPromisesToResolve()
. 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 whatwaitForPromisesToResolve()
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.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.
I'll change this for now since I think I did have it this way at one point.
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.
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.
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.
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.
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.
I can make this more explicit.
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.
It probably made more sense before when the thing immediately before this was
subscribeToReportComments()
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.
"this" could be referring to a number of things, so be more explicit
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.
This is also ambiguous. Is it the pusher callback, the onyx callback, or a different callback entirely?
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.
I can make this more explicit.
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.
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.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.
That's right. The callback should happen in realtime and then we set the data async via
Onyx
.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.
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 modifyREPORT_ACTION
).Suggestion:
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.
Yeah this is much better