1414173 - Fix failure in toolkit/content/tests/widgets relying on non-comformant Promise handling

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

This is a follow-up of the try result in bug 1193394 comment 48:
(M16 in linux64-debug-build: https://treeherder.mozilla.org/logviewer.html#?job_id=141558531&repo=try)

[test_bug1319301.html]
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_bug1319301.html | controlsSpacer should fadeout once video starts playing - got null, expected "true"

[test_videocontrols.html]
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Error: Got event: seeking, expected: volumechange
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Error: Got event: seeked, expected: volumechange
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Error: Got event: volumechange, expected: seeking
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Error: Got event: volumechange, expected: seeking
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Error: Got event: seeking, expected: seeked
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Error: Got event: seeking, expected: volumechange
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols.html | Test timed out.

[test_videocontrols_error.html]
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_error.html | statusOverlay should show when errorNoSource
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_error.html | statusOverlay should have correct error state: errorNoSource - got null, expected "errorNoSource"
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_error.html | should show error icon when errorNoSource - got "throbber", expected "error"

[test_videocontrols_video_noaudio.html]
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_video_noaudio.html | undefined assertion name - got null, expected "true"
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_video_noaudio.html | undefined assertion name - got null, expected "true"

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Ray, would you like to take this? I looked at test_bug1319301.html and I think we would just need to put a setTimeout(..., 0) in the problematic testcase.

Bevis, would that be the correct solution here?

Comment 2

[Ray Lin[:ralin]]

Yes, I'll apply Bevis' patch to look into the failures.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Yes, setTimeout(..., 0) could fix this problem and thanks for help on this, Ray!

For example, in test_bug1319301.html, 2 listeners to the "play" event are added individually by videocontrols in mozSystemGroup and by 2nd testcase in test_bug1319301.html.
Without the fix in bug 1193394, 
the resolved callback to run 3rd test case is executed at the end of current task (both listeners are already done).
With the fix in bug 1193394, 
1. the same resolved callback is executed right after the listener of 2nd testcase is returned.
2. then, the "fadeout" attribute of the controlsSpacer is set by the listener of videocontrols in SystemGroup.
(Note: the listeners in SystemGroup will always be done after the ones in content.)

Hence, setTimeout() helps to remove the dependency of the executing order of these listeners for verifying the "fadeout" attribute of the controlsSpacer.

Comment 4

[Ray Lin[:ralin]]

Thank for clarification, Bevis.

I've fixed test_bug1319301.html on local. So, IIUC, the fix is like adding a setTimeout(or SimpleTest.executeSoon) would force the test case execute after a js callback instead of resolving right after listener, therefore, we can ensure all the listeners(including systemGroup) be fully executed before moving on in content? Just want to make sure I'm doing something reasonable. Thanks.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Per offline discussion, setTimeout(...,0) force the script to enqueue a new task to verify the result.
This ensures that all the listeners in current tasks are done before verifying the test result.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: mt_test_fixes.diff

FYI, here is a WIP from Olli while fixing 1193394 earlier in Jan.
I found that test_bug1319301.html, test_videocontrols_error.html and test_videocontrols_video_noaudio.html has been included. :)

Comment 7

[Ray Lin[:ralin]]

Thanks, it's really helpful and glad to know we have more representative API: TestUtils.waitForTick() can use here.

Comment 8

[Ray Lin[:ralin]]

Wups, I later realized that TestUtils.waitForTick() is for bc :P, I'll stick to SimpleTest.executeSoon() though.

Comment 9

[Ray Lin[:ralin]]

Attachment: Bug 1414173 - Explicitly enqueue task to fix the non-conformant promising handling failures in video controls mochitests.

Review commit: https://reviewboard.mozilla.org/r/197188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197188/

Comment 10

[Ray Lin[:ralin]]

This patch is tested locally with/without bug 1193394's patch applied, and the results look good. 

Jared, could you help me to view it? Thanks.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8925962 [details]
Bug 1414173 - Explicitly enqueue task to fix the non-conformant promising handling failures in video controls mochitests.

https://reviewboard.mozilla.org/r/197188/#review203474

Comment 12

[Ray Lin[:ralin]]

Thanks Jared :D

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ead14db67820
Explicitly enqueue task to fix the non-conformant promising handling failures in video controls mochitests. r=jaws

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ead14db67820