Closed Bug 1414173 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: bevis, Assigned: ralin)

References

Details

Attachments

(2 files)

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"
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?
Flags: needinfo?(ralin)
Flags: needinfo?(btseng)
Priority: -- → P2
Yes, I'll apply Bevis' patch to look into the failures.
Assignee: nobody → ralin
Flags: needinfo?(ralin)
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.
Flags: needinfo?(btseng)
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.
Flags: needinfo?(btseng)
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.
Flags: needinfo?(btseng)
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. :)
Thanks, it's really helpful and glad to know we have more representative API: TestUtils.waitForTick() can use here.
Wups, I later realized that TestUtils.waitForTick() is for bc :P, I'll stick to SimpleTest.executeSoon() though.
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 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
Attachment #8925962 - Flags: review?(jaws) → review+
Thanks Jared :D
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ead14db67820
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.