Page MenuHomePhabricator

Return promises from action thunks
Closed, ResolvedPublic3 Story Points

Description

Ronseal.

Currently, to unit test asynchronous actions we intercept all calls to wait and return our own promises that we then manipulate directly. This approach has issues as any call to jQuery.Promise#then et. al. returns a new instance of jQuery.Promise. If we were to return the promises from the action thunks, then this wouldn't be the case.

AC

  • The linkDwell thunk returns a promise.
  • The abandon thunk returns a promise.
  • The unit tests for the actions are updated to
    • Unnecessary setTimeout(..., 0) and wait(0) in the tests are removed
    • Unnecessary promise/deferreds coordination is removed in favor of just waiting for the promises from the functions to finish

Bonus AC

  • We can also remove the wait(0) in some of the tests since the action creators will return a promise that we can wait to, instead of having to hackily wait till next tick to check the assertions.
  • Can we remove .fail in actions.js#L123?
    • Clarify if we are using jQuery3 everywhere 2, as we couldn't remove it before since jquery3 wasn't rolled out everywhere.
    • Notify the team if jQuery3 is rolled out to all projects now

Related Objects

Event Timeline

phuedx created this task.Jul 17 2017, 1:06 PM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald TranscriptJul 17 2017, 1:06 PM
Jdlrobson triaged this task as Normal priority.Jul 17 2017, 5:12 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

We can also remove the wait(0) in some of the tests since the action creators will return a promise that we can wait to, instead of having to hackily wait till next tick to check the assertions.

Jdlrobson set the point value for this task to 3.Aug 22 2017, 4:28 PM

Can we remove .fail in actions.js#L123?

Niedzielski updated the task description. (Show Details)Sep 13 2017, 2:25 PM
Jdlrobson lowered the priority of this task from Normal to Low.Nov 1 2017, 5:37 PM

We reestimated this task as a 3 during today's Story Prioritization meeting.

Jdlrobson raised the priority of this task from Low to Normal.Jan 31 2018, 5:10 PM
MBinder_WMF updated the task description. (Show Details)Jan 31 2018, 5:11 PM

@ovasileva: The FTEs identified this task as ready to be worked on in the next iteration.

Jhernandez updated the task description. (Show Details)Jan 31 2018, 5:15 PM
Jhernandez updated the task description. (Show Details)

Change 410156 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/Popups@master] [WIP] Return promises from action thunks

https://gerrit.wikimedia.org/r/410156

Jdrewniak added a comment.EditedFeb 13 2018, 12:25 PM

I've marked my patch as WIP because I'm having trouble with this unit test, which essentially tests ABANDON_END_DELAY (the delay after which an event logging event is sent after the use hovers off a preview).

Regarding the jQuery 3 question, according to this task T124742 it looks like it's rolled out, do we need further investigation to verify that?

Scratch that, misunderstood the question: are we using jQuery 3 everywhere in our codebase.

As you found out, the jQuery 3 migration is complete so the answer to

Can we remove .fail in actions.js#L123?

Is Yes, it can now be migrated to use Promise methods like .catch if done appropriately.

See https://gerrit.wikimedia.org/r/c/375357/ for when we did the revert, so fixing that AC is basically restoring the change reverted in https://gerrit.wikimedia.org/r/c/375357/1/src/actions.js

Change 410156 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Return promises from action thunks

https://gerrit.wikimedia.org/r/410156

What are the QA steps here? Just a general "check that page previews still work"? Please specify.


Also, I'm not sure that the AC Unnecessary promise/deferreds coordination is removed in favor of just waiting for the promises from the functions to finish is fully done.

I'm happy to have a look and see if we can restructure these tests differently to remove that dance, let me know if I can take over this one.

There are also a couple of wait( 0 ) in the integration.test.js that are probably not necessary.

As I said, I'd be interested to take over, let me know.

@Jhernandez please do, there is quite a few of them used by the fetch method, so maybe some could be removed.

Jdrewniak reassigned this task from Jdrewniak to Jhernandez.Feb 15 2018, 1:54 PM
Jdrewniak added a subscriber: Jdrewniak.

Change 410911 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Don't leak deferreds out of functions

https://gerrit.wikimedia.org/r/410911

Jhernandez updated the task description. (Show Details)Feb 15 2018, 5:21 PM

Change 410911 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Don't leak deferreds out of functions

https://gerrit.wikimedia.org/r/410911

Change 411224 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Improve & fix action and integration tests

https://gerrit.wikimedia.org/r/411224

Jhernandez updated the task description. (Show Details)Feb 16 2018, 1:11 PM
Jhernandez updated the task description. (Show Details)Feb 16 2018, 1:16 PM

Patch for review up. I've locally QAd it didn't see any issues. I've also checked the AC from what I've seen done in the sources, and sent an email about the jQuery version to the team list.

Change 411224 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Improve & fix action and integration tests

https://gerrit.wikimedia.org/r/411224

Niedzielski closed this task as Resolved.Feb 20 2018, 7:02 PM
Niedzielski added a subscriber: Niedzielski.

We discussed this task in standup and believe to be resolved.