Closed Bug 1047168 Opened 5 years ago Closed 5 years ago

Modify test_play_youtube to play a video file locally

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zcampbell, Assigned: martijn.martijn)

References

Details

Attachments

(3 files, 11 obsolete files)

5.55 KB, patch
Details | Diff | Splinter Review
46.21 KB, text/plain
Details
46 bytes, text/x-github-pull-request
Bebe
: review+
gmealer
: review+
Details | Review
I discussed with Jason and he said that it would be OK to modify this test to load a locally served HTML file with an embedded video with the same check that it's playing.

This should be a lot more reliable and we can run it on CI - w00t.
Attached file github pr (obsolete) —
This should be a lot more reliable than the old test.

If you both r+ it then you can ask Dave or someone in #gaia to uplift it to v2.0 aswell.
Attachment #8465954 - Flags: review?(viorela.ioia)
Attachment #8465954 - Flags: review?(florin.strugariu)
Comment on attachment 8465954 [details] [review]
github pr

r+
Attachment #8465954 - Flags: review?(viorela.ioia) → review+
Attachment #8465954 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8465954 [details] [review]
github pr

Green on all platforms now, let's try again.
Attachment #8465954 - Flags: review?(viorela.ioia)
Attachment #8465954 - Flags: review?(dave.hunt)
Attachment #8465954 - Flags: review+
Comment on attachment 8465954 [details] [review]
github pr

See github comment.
Attachment #8465954 - Flags: review-
Comment on attachment 8465954 [details] [review]
github pr

Pull https://github.com/mozilla-b2g/gaia/pull/22419 looks good to me. Tests are passing on device & desktopb2g. r+
Attachment #8465954 - Flags: review?(viorela.ioia) → review+
Attachment #8465954 - Flags: review?(dave.hunt)
The mp4 file does not render on desktopb2g/TBPL. I'll try with 3gp and if that fails too then I'll revert to mp4 and disable the test for that configuration.
3gp doesn't work on Linux so I'll set it to mp4 and disable the mac test.
Attached file Github PR (obsolete) —
Jason requested a switch to MP4 from OGG. Had to exclude mac from the TBPL run in order to get the testing coverage with mp4 on linux/device.
Attachment #8465954 - Attachment is obsolete: true
Attachment #8467875 - Flags: review?(viorela.ioia)
Attachment #8467875 - Flags: review?(dave.hunt)
Comment on attachment 8467875 [details] [review]
Github PR

Lgtm, r+
Attachment #8467875 - Flags: review?(viorela.ioia) → review+
Comment on attachment 8467875 [details] [review]
Github PR

Could we split this test into one for each video format? That way we'd have more cross platform coverage. Also, please raise a bug where the formats are not supported by a platform.
Attachment #8467875 - Flags: review?(dave.hunt) → review-
I'd rather not; it's not really the test case to test all formats. As I understand it we're mostly interested in the html5 video tag rather than actual codec functionality.

Jason said the test case would be fine with mp4 or 3gp formats but I'll ni? him just for confirmation.
Flags: needinfo?(jsmith)
Comment on attachment 8467875 [details] [review]
Github PR

Cited the bug number in tbpl-manifest.ini
Attachment #8467875 - Flags: review- → review?(dave.hunt)
(In reply to Zac C (:zac) from comment #13)
> I'd rather not; it's not really the test case to test all formats. As I
> understand it we're mostly interested in the html5 video tag rather than
> actual codec functionality.
> 
> Jason said the test case would be fine with mp4 or 3gp formats but I'll ni?
> him just for confirmation.

Right. We only really need to test the hardware codecs here that YT uses, so 3GP & mp4 should be enough.
Flags: needinfo?(jsmith)
Do you mean now you want 2 tests, one for 3gp and one for mp4?
(In reply to Zac C (:zac) from comment #16)
> Do you mean now you want 2 tests, one for 3gp and one for mp4?

Yes.
No I'm not going to do two tests, it's a waste of time. We only had one test before and we'll only have one now.
(In reply to Zac C (:zac) from comment #18)
> No I'm not going to do two tests, it's a waste of time. We only had one test
> before and we'll only have one now.

Why? YT's videos used traditionally are mp4 (traditional format for videos on the mobile site) & 3gp formats (traditional format for videos for ads), so having automation representing test coverage for these codecs provides an accurate representation of what would be tested on YT's site.
because 2 tests are not twice as good as one, it adds more time to the test runs. I don't think it's worth it. and it's annoying to change the spec of the task right when it's almost finished.
(In reply to Zac C (:zac) from comment #20)
> because 2 tests are not twice as good as one, it adds more time to the test
> runs. I don't think it's worth it. and it's annoying to change the spec of
> the task right when it's almost finished.

What's the average amount of time to run the existing YT test & how does it compare to the existing mp4 test case right now? That could tell us how much time is gained or lost here. My hypothesis is that running two tests hitting a local hosted video file of different codecs is likely going to faster than the YT test case, mainly because YT requires hitting the network & loading a complex site.

I understand the concern with the spec change - that's my fault. I should have been clear up front about YT's codec needs up front. Note though that having coverage for both of those codecs does match what YT can do, so it will ensure that we've got the right test coverage here that models with YT does on it's mobile site.
It's also a case of platform coverage if each format is going to have to be skipped on certain platforms. We could even keep a single test and play multiple videos, skipping any that are not appropriate to the current platform. Note that my preference would always be for distinct tests, but I understand the overhead of doing this on B2G, and especially on device.
Comment on attachment 8467875 [details] [review]
Github PR

Please request review again once an agreement is reached on the scope of this test.
Attachment #8467875 - Flags: review?(dave.hunt)
I checked YouTube and it said the format is WebM anyway. Well whatever, I'll split it into 3gp/mp4 tests.
(In reply to Zac C (:zac) from comment #24)
> I checked YouTube and it said the format is WebM anyway. Well whatever, I'll
> split it into 3gp/mp4 tests.

Was that the YT mobile site or desktop site?
Just this page:
https://www.youtube.com/html5?gl=GB
Attachment #8467875 - Flags: review?(dave.hunt)
Attachment #8467875 - Flags: feedback?(jsmith)
Comment on attachment 8467875 [details] [review]
Github PR

Approach is fine, although there's a bug in the code here - the mp4 test has a coding error that causes it to playback a 3GP file instead of a mp4 file.
Attachment #8467875 - Flags: review-
Attachment #8467875 - Flags: feedback?(jsmith)
Attachment #8467875 - Flags: feedback+
Comment on attachment 8467875 [details] [review]
Github PR

Looks like this is failing on Gaia-Try with timeouts? Also, we're missing to mention the video format in a couple of places.
Attachment #8467875 - Flags: review?(dave.hunt) → review-
Weird, now 3gp and mp4 are failing on linux (can't test for Mac).
Now both file types are failing on Mac (only mp4 expected to fail) and 3gp failing on linux (as expected). A bit baffled, I'm going to shelve this until I'm back from PTO.
nb - someone else can take the job if they think they can get it working.
Assignee: nobody → martijn.martijn
Attached patch video.diff (obsolete) — Splinter Review
I was working on bug 982839, then I thought, let's make the youtube_video test run locally, because then it would work on desktop b2g too. 
So that meant I was accidentally also working on this bug.

This works locally on b2g desktop and device. 
But it's still more or less gambling on where the play/pause and full screen buttons are in the video controls (depends on fix for bug 983763).
I think I might be able to fix it by using utils.getChildrenForNode:
http://people.mozilla.org/~mwargers/tests/videos/simple_video.htm

Currently, there is bug 1086265, so in current trunk builds, this test does fail anyway.
I tried to squash the 2 commits into 1, using git rebase -i HEAD~2. git log showed only the 1 commit, but when I then pushed, I'm still seeing the 2 commits. I'm at a loss why that happens.

Anyway, this fixes this bug and also fixes bug 982839, adding a full screen test.
I also added a test that the mute button works correctly.

This patch makes it so that one always has the correct coordinates for the video control buttons, so that part should be very reliable now.

Perhaps, there should also be added a test that the slider works correctly, that part hasn't been tested yet.
Attachment #8508724 - Attachment is obsolete: true
Attachment #8512254 - Flags: review?(florin.strugariu)
Ok, this one has 1 commit message.
Attachment #8512254 - Attachment is obsolete: true
Attachment #8512254 - Flags: review?(florin.strugariu)
Attachment #8512260 - Flags: review?(florin.strugariu)
Attachment #8467875 - Attachment is obsolete: true
Comment on attachment 8512260 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25556

Looks OK.

I added some comments that should be addressed 

Also I should bot be the only reviewer of the pull
Attachment #8512260 - Flags: review?(viorela.ioia)
Attachment #8512260 - Flags: review?(robert.chira)
Attachment #8512260 - Flags: feedback?(dave.hunt)
(In reply to Florin Strugariu [:Bebe] from comment #35)
> Comment on attachment 8512260 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/25556
> 
> Looks OK.
> 
> I added some comments that should be addressed 

Thanks, I added some comment to your comments of what I'm not completely clear about.
For some reason, I wasn't able to push to the youtube2 branch.
To https://github.com/mwargers/gaia.git
 ! [rejected]        youtube2 -> youtube2 (non-fast-forward)
error: failed to push some refs to 'https://github.com/mwargers/gaia.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

So I've just made a youtube3 branch.
However, that branch shows on github: "This pull request contains merge conflicts that must be resolved. "
I don't know why that happens. I can make a new branch and apply the patch manually, if that is necessary.

+ # For some reason on device, the controls are still showing for a short while,
+ # after tapping full screen. Marionette device issue?
+ time.sleep(1)

This was actually not the case anymore, so I removed it, and the tests still pass (I repeated 10 times).

For the rest, I followed all review comments.
Attachment #8512260 - Attachment is obsolete: true
Attachment #8512260 - Flags: review?(viorela.ioia)
Attachment #8512260 - Flags: review?(robert.chira)
Attachment #8512260 - Flags: review?(florin.strugariu)
Attachment #8512260 - Flags: feedback?(dave.hunt)
Attachment #8514360 - Flags: review?(florin.strugariu)
QA Whiteboard: fxosqa-auto-s3, fxosqa-auto-points=8, fxosqa-auto-from-s2
Flags: in-qa-testsuite?(martijn.martijn)
Blocks: 1071587
Comment on attachment 8514360 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25658

This needs to be updated to make use of the new Wait api as mentioned in the gaia-ui-automation mailing list "Changes to how we wait_for things in Gaia tests":
https://mail.mozilla.org/private/gaia-ui-automation/2014-November/001588.html
Attachment #8514360 - Flags: review?(florin.strugariu)
Attachment #8514360 - Attachment is obsolete: true
Attachment #8517541 - Flags: review?(florin.strugariu)
Attachment #8517541 - Flags: review?(zcampbell)
Comment on attachment 8517541 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25849

r-: The test has failed on treeherder :(
Attachment #8517541 - Flags: review?(zcampbell) → review-
(In reply to Zac C (:zac) from comment #40)
> Comment on attachment 8517541 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/25849
> 
> r-: The test has failed on treeherder :(

This is apparently, the failure:
06:29:12 TEST-UNEXPECTED-ERROR | test_browser_play_video.py TestVideo.test_play_video | TimeoutException: TimeoutException: Timed out after 20.1 seconds
06:29:12 
06:29:12 
06:29:12 Traceback (most recent call last):
06:29:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette_test.py", line 264, in run
06:29:12     testMethod()
06:29:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_play_video.py", line 69, in test_play_video
06:29:12     permission.wait_for_permission_dialog_displayed()
06:29:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/apps/homescreen/regions/permission_dialog.py", line 17, in wait_for_permission_dialog_displayed
06:29:12     self.wait_for_element_displayed(*self._permission_dialog_locator)
06:29:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/apps/base.py", line 44, in wait_for_element_displayed
06:29:12     lambda m: m.find_element(by, locator).is_displayed())
06:29:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/wait.py", line 143, in until
06:29:12     cause=last_exc)
That error message seems to indicate that the permission dialog didn't appear at all.
And indeed, looking at the screenshot of the test shows that the video is displayed in full screen:
http://mozilla-releng-blobs.s3.amazonaws.com/blobs/gaia-try/sha512/94ba1ad056c060d99d1c88ebd00acee47aa922c040e426121d8fa9d10d2aa26f5d7fd0d5478512e10eb06008aa199c012d2c31a7b2154391451b474fe85757d4
So I guess the full-screen permission is turned on by default?
Ok, according to http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/282/console this happens intermittently.
I haven't seen this happening locally.

When this error is happening, the video is already in full-screen.
My guess is that the permission dialog gets rendered under the full-screen video in that case, just like the context dialog in full-screen video (bug 1088178).
But I can only guess, because I can't reproduce it locally at all.

For now, I can just comment out the full screen parts of the test and retry enabling those when bug 1088178 is fixed.
Regarding the failure in B2G Desktop OS X opt, it's failing on something that is used by the youtube test, too.

My guess is that the video is looping (playing again at the beginning, the video is rather short), which makes the is_video_playing method fail.
Ok, using a longer playing video now (20s) and resetting the video at the start again to make sure that this failure doesn't happen again on try on Mac.

I've removed the full screen part of the test for now.
Attachment #8517541 - Attachment is obsolete: true
Still failing on Mac :/
The logic of this test is not really different than from test_browser_play_youtube_video.py, only the video is fed from a local web server. Is the local web server on automation really slow or something?
Ok, there was an intermittent failure also on Flame with the previous patch.
This should take care of that by add a player.wait_for_video_loaded() before checking player.is_video_playing().
Local testing on the Flame makes the intermittent failure not appear anymore (10 repeats).
Also 10 repeats on desktop are passing.

If this is still failing on desktop automation, then I'll just disable it on desktop in the manifest.ini. That's not ideal (and I would like to know why that happens on desktop automation), but test_browser_play_youtube.py wasn't enabled anyway.
Attachment #8518557 - Attachment is obsolete: true
(In reply to Martijn Wargers [:mwargers] (QA) from comment #48)
> If this is still failing on desktop automation, then I'll just disable it on
> desktop in the manifest.ini. That's not ideal (and I would like to know why
> that happens on desktop automation), but test_browser_play_youtube.py wasn't
> enabled anyway.

Still failing on desktop automation, so I'll just add the fail-if = device == "desktop" back for test in manifest.ini and I'll file a new bug on getting rid of it.
Attachment #8518902 - Flags: review?(florin.strugariu)
Comment on attachment 8518902 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/25936

Ugh, try run on Flame-kk still shows an intermittent failure :(
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/292/console
Attachment #8518902 - Flags: review?(florin.strugariu)
Ok, bug 1071587 is about intermittent failures in test_play_youtube_video.py and that one has the same intermittent failure in self.assertTrue(player.is_video_playing()).
I think that player.is_video_playing() is not reliable, atm.
Zac, is_video_playing is not reliable, it's causing intermittent failures in test_play_youtube_video.py (bug 1071587) and in this bug, when trying to rewrite this test.
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/apps/search/regions/html5_player.py#69
I assume you want to keep the function, though, to be able to test that the video is actually playing.
But can we increase the time.sleep value to .5 or something?
Flags: needinfo?(zcampbell)
Do whatever you need to do to make the test reliable.
Flags: needinfo?(zcampbell)
QA Whiteboard: fxosqa-auto-s3, fxosqa-auto-points=8, fxosqa-auto-from-s2 → fxosqa-auto-s4, fxosqa-auto-points=8, fxosqa-auto-from-s3
Attachment #8518902 - Attachment is obsolete: true
Attachment #8521732 - Flags: review?(florin.strugariu)
Forin did a Jenkins run on the latest patch: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/311/console
Unfortunately, still failures:
07:35:45 TEST-UNEXPECTED-FAIL | test_browser_play_video.py TestVideo.test_play_video | AssertionError: False is not true
07:35:45 
07:35:45 Traceback (most recent call last):
07:35:45   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette_test.py", line 264, in run
07:35:45     testMethod()
07:35:45   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_play_video.py", line 53, in test_play_video
07:35:45     self.assertTrue(player.is_video_playing())

07:47:12 Traceback (most recent call last):
07:47:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette_test.py", line 264, in run
07:47:12     testMethod()
07:47:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_play_video.py", line 36, in test_play_video
07:47:12     player.wait_for_video_loaded()
07:47:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/apps/search/regions/html5_player.py", line 31, in wait_for_video_loaded
07:47:12     lambda m: int(self.root_element.get_attribute('readyState')) == 4)
07:47:12   File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/wait.py", line 143, in until
07:47:12     cause=last_exc)
07:47:12 TEST-INFO took 141377ms
Locally, I let it rerun 11 times and it passed all the time.

When I look at the 2 failures of the Jenkins runs, 1 time is_video_playing() is returning false and the second time wait_for_video_loaded is timing out.
Can it be that the local webserver is not working reliably or that pushing files onto the sd card is not working reliably?
Sorry, pushing files onto the sd card is actually not happening. Then my only guess is that the local web server is flaky.
Did you try the same test with a different video format?
(In reply to Zac C (:zac) from comment #58)
> Did you try the same test with a different video format?

I started out with the mp4 file, which had the same issue.

The screenshots is helpful for the 2nd failure: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/311/HTML_Report/
It shows the at the video still hasn't loaded after 10 seconds. That seems to indicate that the local webserver is very slow (in that case, at least). Florin suggested to up the value in wait_for_video_loaded() to 60 seconds.

The first failure screenshot at least shows that the video has loaded, but that's logical. In this case, player.is_video_playing() is returning false after tapping the play button, after having been paused (there checks that this is done correctly).
Attached patch logging.diffSplinter Review
I added some logging to see if there is a pattern somewhere, where this currentTime is not changed.
Attached file result.txt
This is a test run with this logging enabled

Everything before "expected not playing" is expected (coming from the pause test).

Before tapping the Pause button, the is_video_playing function always returns True.
After tapping the Pause button, then the failures can be seen. This is also the case with bug 1071587.

Usually, it's failing in the first couple of sleeps, but it's also sometimes quite some longer.
I filed bug 1100333 for this. I made a testcase: http://people.mozilla.org/~mwargers/tests/videos/VID_0001_video_page.htm 
And with that testcase, I sometimes can see the bug occurring.

I propose to just disable the is_video_playing check after tapping the Play button, because then, it is unreliable.
Attachment #8521732 - Attachment is obsolete: true
Attachment #8521732 - Flags: review?(florin.strugariu)
Attachment #8523834 - Flags: review?(florin.strugariu)
I triggered http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/327/ to test on the Flame device.
Sorry, previous one is wrong, new one: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/328/
Unfortunately, on Jenkins I see this failure 2 times in a 20 times loop:
http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/328/
Traceback (most recent call last):
  File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette_test.py", line 264, in run
    testMethod()
  File "/var/jenkins/1/workspace/flame-kk.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_play_video.py", line 58, in test_play_video
    'video resumed to play not from place where it was paused')
AssertionError: video resumed to play not from place where it was paused
Note that I tested this patch locally and did repeat it 30 times and I didn't see any failures. I can put the assertLessEqual call before the wait_for_video_loaded to see if that fixes this failure.
I'll also add the delay, self.acceptable_delay numbers to the assertLessEqual, so I at least know what numbers those give when it fails.

Also note that this might fail in combination with the assertTrue(player.is_video_playing()) call (I suspect so). Unfortunately, when assertTrue(player.is_video_playing()) is failing, the python test aborts after that, so I can never be sure.
In this case, it would have been nice if the test would have continued.
Attached file youtube9
Ok, when setting my Flame device to 319mb, it's much easier to trigger these kinds of failures.
It turns out that these janks in video playing are really happening (I filed bug 1100333 for that). So I've changed is_video_playing() to 2 seconds. That seems to work all right, not triggering intermittent failures. I've disabled it for after tapping Play again, because there it's much easier to cause a failure for some reason.

is_video_playing() was really to test whether the video would play fluently, but that's just not happening in practice.
Attachment #8523834 - Attachment is obsolete: true
Attachment #8523834 - Flags: review?(florin.strugariu)
Attachment #8524575 - Flags: review?(florin.strugariu)
Comment on attachment 8524575 [details] [review]
youtube9

Looking good and it's running as expected

Can we get a second review on this
Attachment #8524575 - Flags: review?(florin.strugariu) → review+
Attachment #8524575 - Flags: review?(gmealer)
Blocks: 1088178
Comment on attachment 8524575 [details] [review]
youtube9

Looks good to me.
Attachment #8524575 - Flags: review?(gmealer) → review+
merged in https://github.com/mozilla-b2g/gaia/commit/64704b265db4f8f16c72102fdff4c6b70d7f4292
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
QA Whiteboard: fxosqa-auto-s4, fxosqa-auto-points=8, fxosqa-auto-from-s3 → fxosqa-auto-s4, fxosqa-auto-points=8, fxosqa-auto-from-s3, fxosqa-auto-from-s2, fxosqa-auto-from-s1
Flags: in-qa-testsuite?(martijn.martijn) → in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.