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

VERIFIED FIXED in Firefox 57

Status

()

defect
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: jhlin)

Tracking

55 Branch
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57+ verified)

Details

Attachments

(2 attachments)

[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
Flags: needinfo?(jolin)
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 :)
(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
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.
Flags: needinfo?(jolin)
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.
Attachment #8904162 - Flags: review?(ralin) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bf3b29276f2e
enable prefs needed for orientation lock test. r=ralin
Assignee: nobody → jolin
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
Flags: needinfo?(jolin)
Keywords: leave-open
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.
Flags: needinfo?(jolin)
Attachment #8904492 - Flags: review?(ralin)
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.
Attachment #8904492 - Flags: feedback+
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
Attachment #8904492 - Flags: review?(ralin) → review+
Keywords: leave-open
Thanks a lot for your help, Ryan and Ray!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/565627232edd
Use prefixed fullscreen element in orientation lock test. r=ralin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/565627232edd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.