982839 - Write a test to protect against the regression seen in bug 981804 (navigate out of fullscreen video)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect, P1)

Description

[Jason Smith [:jsmith]]

bug 981804 was a smoketest regression caught manually, but not by automation. Let's get a test covering this.

Comment 1

[Zac C (:zac)]

We have an existing youtube test which we can upgrade to do this extra action, I think it should be do-able!

However we cannot catch this in-tree because desktopb2g gets the desktop version of the site rather than the mobile one that the device gets. So the test case is invalid for desktopb2g.

Comment 2

[Zac C (:zac)]

OK I did some research into this and the control elements (eg fullscreen, play/stop, volume) come from XBL DOM and Marionette cannot interact with them. At the moment we're just tapping in the middle of the screen and getting lucky because play/pause is centred.

Unfortunately there's no other path to fullscreen like double tap.

It might be possible in the future for Marionette to reach these elements but now it seems like a bit of an edge case.

Comment 3

[Jason Smith [:jsmith]]

This is not an edge case. This was marked as a smoketest blocker for a reason & an automated test request was asked for post that bug. Reopening.

Comment 4

[Zac C (:zac)]

OK I will file a request for Marionette to be able to interact with XBL

Comment 5

[Martijn Wargers (dead)]

Bug 981804 is about device, so the test (or part of) doesn't necessarily have to be tested on desktop (but would of course be welcome).

Comment 6

[Martijn Wargers (dead)]

I have something in bug 1047168 that also has this incorporated as part of test.
https://moztrap.mozilla.org/manage/case/6073/ didn't actually contain this full screen test, but it has now been added.

Comment 7

[Martijn Wargers (dead)]

I had to remove the full screen part of the test, because the fullscreen permission dialog doesn't show up sometimes on the automation devices (I haven't seen this locally). I think this might be due to bug 1088178.

In any case, a workaround that might work is this:
        script = 'document.getElementById("permission-yes").click()'
        self.marionette.execute_script(script, special_powers=True)

If that works, total code that should then be added:
        player.tap_full_screen()
        self.marionette.switch_to_default_content()
        # Workaround for bug 1088178 which causes permission dialog to 
        # appear below the fullscreen video
        script = 'document.getElementById("permission-yes").click()'
        self.marionette.execute_script(script, special_powers=True)

        browser.switch_to_content()

        # This lines is for some reason necessary, because Marionette on device
        # lost connection to the element for some reason
        player = HTML5Player(self.marionette)

        Wait(self.marionette).until(lambda m: player.is_fullscreen)

        # After tapping full screen, the controls disappear, make them appear again
        player.invoke_controls()
        Wait(self.marionette).until(lambda m: player.controls_visible)
        player.tap_full_screen()
        Wait(self.marionette).until(lambda m: player.is_fullscreen is False)
        Wait(self.marionette).until(lambda m: player.controls_visible is False)

Comment 8

[Martijn Wargers (dead)]

Attachment: fullscreen

I'm trying it with the original way one more time. I couldn't reproduce any of the failures with it, locally (retried 10 times on desktop and on 319mb Flame device)

Kicked of a Jenkins try run with this:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/347/

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 8526078 [details] [review]
fullscreen

So this is not failing anymore, like it was in bug 1047168.
I don't know why it did cause problems while working on bug 1047168, but I learnt from this it's better to not work on 2 tests at the same time.

Comment 10

[Florin Strugariu [:Bebe]]

Comment on attachment 8526078 [details] [review]
fullscreen

except for one nit this looks OK

Comment 11

[Martijn Wargers (dead)]

Attachment: fullscreen2

I've updated the comment nit.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 8531621 [details] [review]
fullscreen2

r=me, with removal of the duplicated content switch

Comment 13

[Martijn Wargers (dead)]

Ok, removed the duplicated content switch, this can be merged.

Comment 14

[Florin Strugariu [:Bebe]]

https://github.com/mozilla-b2g/gaia/commit/5827a45d1e1e8c292fe78fe206745941be742e81

Comment 15

[Carsten Book [:Tomcat]]

sorry had to revert this for possible causing failures like https://treeherder.mozilla.org/logviewer.html#?job_id=968980&repo=b2g-inbound

Comment 16

[Zac C (:zac)]

I retriggered the Try on the last Try before the landed commit and the test failed 1 in 15 (approx):
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=6454157c6c6d

Comment 17

[Martijn Wargers (dead)]

I'm really out of ideas what could cause this intermittent failure. If anyone has ideas, let me know.
Otherwise, I guess I should setup a gaia environment on my linux vm, since the intermittent failure seems to be happening there.

Comment 18

[Zac C (:zac)]

I'll try and replicate it locally.

Comment 19

[Martijn Wargers (dead)]

Attachment: fullscreen4

Ok, I added a small time.sleep(.25) call in the problematic part. Hopefully this will fix the intermittent fail.

Comment 20

[Martijn Wargers (dead)]

I kicked of a whole sleigh of try runs with that: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=f92dba0b9b15

Comment 21

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #19)
> Created attachment 8534721 [details] [review]
> fullscreen4
> 
> Ok, I added a small time.sleep(.25) call in the problematic part. Hopefully
> this will fix the intermittent fail.

To pile on this a bit, this came as a result of a discussion where we thought about why the button wasn't getting pressed. 

Since the code is very simple, and since we weren't able to replicate at all manually on Linux/B2G Desktop, best guess was that there might be a small delay after when it's visible and before it can receive the user event. That would result in a race condition, and an intermittent.

With that in mind, adding a 25ish ms sleep after the conditional wait seemed like something good to try.

Comment 22

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

> With that in mind, adding a 25ish ms sleep after the conditional wait seemed
> like something good to try.

Urk, meant 250 ms. 25 ms is a bit aggressive :)

Comment 23

[Martijn Wargers (dead)]

Still failing sometimes :(

Comment 24

[Zac C (:zac)]

I think this is a race between the wait for the tap and the controls fading out.

The more code (and hence time) you can remove between those two steps, the better, IMO. Adding a sleep to it there will make it more likely that the controls will fade out before it attempts to tap.

It's also odd that this test takes ~90 seconds to run on treeherder but 30s to run locally.

I haven't got a solution right now.

Comment 25

[Martijn Wargers (dead)]

Ok, I've now a linux virtual machine and I can get sometimes failures there. Except it's failing on player.invoke_controls(), line 84, waiting on the controls to appear.
There are cases where the videocontrols fail to appear after tapping, see bug 1088127.

Since this point seems to be where the intermittent fails are coming from, I might just programmatically let the videocontrols there, instead of by tapping.

Comment 26

[Martijn Wargers (dead)]

Ok, updated my pull request by doing that, results should come up here: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=d2b53ff245f8 (not sure about that, gaia-try server seems busted).
This passed 51 times on my local linux vm, where with the previous pull request, it had failed 4 times out of 50.

Comment 27

[Martijn Wargers (dead)]

But it basically means that bug 981804 is not tested correctly.

Comment 28

[Martijn Wargers (dead)]

It might be that in the Mozilla office, these intermittent failures on ubuntu vm don't occur (I haven't been able to reproduce them thus far. When I got these 4 failures, I was in the hotel.
It would be great if I somehow could mimick a slow/flaky wifi connection or something (I believe marionette is sending all commands through wifi, right?).

Comment 29

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #27)
> But it basically means that bug 981804 is not tested correctly.

Ok, that may not be the case, it might still test that bug correctly, I was thinking about a different bug.
But it's not ideal, of course.
Treeherder try results seem to be green with this approach (with 30 runs): https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9bcdb6b46252

Comment 30

[Martijn Wargers (dead)]

I think I've now seen this issue occurring locally on Ubuntu VM. When it happens, the video controls show up and then immediately disappear.

Comment 31

[Martijn Wargers (dead)]

Comment on attachment 8534721 [details] [review]
fullscreen4

Ok, I think this is the way to go for now.
Once b2g desktop is out and Mulet is in, we could recheck if this intermittent failure is still out there without this workaround.

Comment 32

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 8534721 [details] [review]
fullscreen4

With all the time we've spent on this, I think it's fine to do it this way and move on. Might make sense to file a bug for re-evaluating it later, as you say, but that's no blocker to getting the code merged. LGTM.

Comment 33

[Florin Strugariu [:Bebe]]

Comment on attachment 8534721 [details] [review]
fullscreen4

with the condition we add a comment and log a bug to revisit this

Comment 34

[Martijn Wargers (dead)]

(In reply to Florin Strugariu [:Bebe] from comment #33)
> Comment on attachment 8534721 [details] [review]
> fullscreen4
> 
> with the condition we add a comment and log a bug to revisit this

Ok, added a comment with bug number, bug 1111734.

Comment 35

[Martijn Wargers (dead)]

I'll wait for that tryserver result from treeherder.
I already tested this 62 times in https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9bcdb6b46252 (from comment 29) and they all passed. So I don't expect any intermittent failures of of this.

Comment 36

[Martijn Wargers (dead)]

Merged: https://github.com/mozilla-b2g/gaia/commit/d76a376dcb6d3a71d8d317a9a8929ed61ed7116f