1396232 - test_videocontrols_orientation.html is going to permafail when Gecko 57 merges to Beta on 2017-09-20

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Permafailing test on the next merge day.

https://treeherder.mozilla.org/logviewer.html#?job_id=127931816&repo=try

TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in fullscreen initially - got undefined, expected null
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_orientation.html | Test timed out.
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in fullscreen initially - got undefined, expected null
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_orientation.html | Test timed out.

The patch below should be enough to reproduce the build configuration used to find this bug.
https://hg.mozilla.org/try/rev/70cb6436d870bc25344b98e8ec3ed1be9cf9afc6

Comment 1

[Ryan VanderMeulen [:RyanVM]]

BTW, you might consider annotating this test as |run-if = toolkit == 'android'| instead of the skip-if so the intent is a bit clearer. Just a suggestion, though :)

Comment 2

[Ray Lin[:ralin]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> BTW, you might consider annotating this test as |run-if = toolkit ==
> 'android'| instead of the skip-if so the intent is a bit clearer. Just a
> suggestion, though :)

Thank you Ryan :D

----
I think we would need to explicitly pref on "media.videocontrols.lock-video-orientation"[0] in mochitest to pass on beta.


[0] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/mobile/android/app/mobile.js#940-942

Comment 3

[John Lin [:jhlin][:jolin]]

Ryan, thanks for the suggestion.

Looks like `document.fullscreenElement` is hidden behind pref "full-screen-api.unprefix.enabled", which is off on beta & release. Will turn it on in the test to address this problem.

Comment 4

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1396232 - enable prefs needed for orientation lock test.

Also, use |run-if| instead of |skip-if| to filter the test.

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

Comment 5

[Ray Lin[:ralin]]

Comment on attachment 8904162 [details]
Bug 1396232 - enable prefs needed for orientation lock test.

https://reviewboard.mozilla.org/r/175940/#review180972

Thanks for the quick fix.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf3b29276f2e
enable prefs needed for orientation lock test. r=ralin

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This is still failing for me even with the patch from this bug included in the run.

https://treeherder.mozilla.org/logviewer.html#?job_id=128341250&repo=try

TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in fullscreen initially - got undefined, expected null
  SimpleTest.is@SimpleTest/SimpleTest.js:312:5
  runTest@toolkit/content/tests/widgets/test_videocontrols_orientation.html:38:3
TEST-PASS | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in landscape
TEST-PASS | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in landscape
TEST-UNEXPECTED-FAIL | toolkit/content/tests/widgets/test_videocontrols_orientation.html | undefined assertion name - got undefined, expected [object HTMLVideoElement]
  SimpleTest.is@SimpleTest/SimpleTest.js:312:5
  runTest/window.screen.orientation.onchange@toolkit/content/tests/widgets/test_videocontrols_orientation.html:45:5
TEST-PASS | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should be in landscape
TEST-PASS | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in landscape
TEST-PASS | toolkit/content/tests/widgets/test_videocontrols_orientation.html | should not be in landscape
TEST-OK | toolkit/content/tests/widgets/test_videocontrols_orientation.html | took 7644ms

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[John Lin [:jhlin][:jolin]]

Attachment: Use prefixed fullscreen element in the test so it works for all channels.

Sorry for causing the trouble. Turns out my local test for the previous patch was run on incorrect (i.e. w/o Ryan's build config patch) version of Fennec.

I created this patch that uses the prefixed mozFullscreenElement instead of standard fullscreenElement and it should work in all channels. Please help review it. Thanks a lot.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8904492 [details] [diff] [review]
Use prefixed fullscreen element in the test so it works for all channels.

Looks good on Try! Thanks for adding the trailing newline to the manifest too.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8904492 [details] [diff] [review]
Use prefixed fullscreen element in the test so it works for all channels.

Review of attachment 8904492 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!! Thank you John, Ryan :D

Comment 12

[John Lin [:jhlin][:jolin]]

Thanks a lot for your help, Ryan and Ray!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/565627232edd
Use prefixed fullscreen element in orientation lock test. r=ralin

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/565627232edd