1113219 - Intermittent test_browser_play_video.py TestVideo.test_play_video | AssertionError: False is not true

Status: RESOLVED FIXED

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

Description

[Ryan VanderMeulen [:RyanVM]]

07:49:14 INFO - TEST-START | test_browser_play_video.py TestVideo.test_play_video
07:49:45 ERROR - TEST-UNEXPECTED-FAIL | test_browser_play_video.py TestVideo.test_play_video | AssertionError: False is not true
07:49:45 INFO - Traceback (most recent call last):
07:49:45 INFO - File "/builds/slave/talos-slave/test/build/venv/lib/python2.7/site-packages/marionette/marionette_test.py", line 268, in run
07:49:45 INFO - testMethod()
07:49:45 INFO - File "/builds/slave/talos-slave/test/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_play_video.py", line 37, in test_play_video
07:49:45 INFO - self.assertTrue(player.is_video_playing())
07:49:45 INFO - TEST-INFO took 30256ms

Comment 1

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2014-12-18T10:20:56
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=mozilla-inbound&job_id=4761356
repository: mozilla-inbound
who: rvandermeulen[at]mozilla[dot]com
machine: talos-mtnlion-r5-026
buildname: b2g_macosx64 mozilla-inbound opt test gaia-ui-test-functional-2
revision: bf93cbec0450

TEST-UNEXPECTED-FAIL | test_browser_play_video.py TestVideo.test_play_video | AssertionError: False is not true
Return code: 10

Comment 2

[Martijn Wargers (dead)]

Yeah, I kinda knew this stayed prone to failure:
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/apps/search/regions/html5_player.py#114
This always used to fail intermittently.
I already made it much more loose.

I filed bug 1100333 for a case, where it would cause even more often these intermittent failures.
But I guess we should just remove this whole is_video_playing thing and replace it with is_playing.

Comment 3

[Martijn Wargers (dead)]

The only thing that baffles me is that it happened on b2g desktop, where I would think the machines are sufficiently fast, that this kind of jank wouldn't happen.

Comment 4

[Martijn Wargers (dead)]

Attachment: is_playing

Geo, I heard that at some point, you still wanted to make use of is_video_playing again, so I didn't remove it in this patch. Perhaps you want a comment somewhere about that? That we want to use is_video_playing test again, once it doesn't cause intermittent failures.
(although we talked about this, and I got the impression we agreed more or less, that whole test is kind of prone to causing potential failures)

Comment 5

[Martijn Wargers (dead)]

I kicked of a bunch of extra try runs: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=1381e58987cc

Comment 6

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

Comment on attachment 8539392 [details] [review]
is_playing

Re: keeping is_video_playing(), not necessary. That was more that it would be nice to have the method as a more black box way, but I agree on falling back on this.

Maybe something to check on, though:

http://www.w3.org/TR/html5/embedded-content-0.html#potentially-playing

...says that "potentially playing" means paused = false, *ended = false*, and some stuff about errors and controllers that we're probably not well equipped to check.

is_playing only checks attribute paused != true. So it's another one that will potentially misbehave for short videos. Might want to update it to paused != true && ended != true, at the very least.

I'm OK with this going in, and we can follow up with app object changes if necessary. But you know the most about that part of the code at this point and it might be a good idea for you to check it out and confirm what needs to be done there.

Comment 7

[Martijn Wargers (dead)]

(In reply to Geo Mealer [:geo] ON PTO UNTIL 1/5/15 from comment #6)
> Maybe something to check on, though:
> 
> http://www.w3.org/TR/html5/embedded-content-0.html#potentially-playing
> 
> ...says that "potentially playing" means paused = false, *ended = false*,
> and some stuff about errors and controllers that we're probably not well
> equipped to check.

Yeah, although I think with looking at readyState = 4, the test is following all the rules where a video is actually playing, so that's it's not potentially playing.
But we have to think of this test only as a UI test, where we're testing the video controls UI, not whether the video is actually playing correctly (which has been demonstrated to be intermittently failing).

> is_playing only checks attribute paused != true. So it's another one that
> will potentially misbehave for short videos. Might want to update it to
> paused != true && ended != true, at the very least.

When the video has ended, I think it would cause more test failures, because the video controls become then visible.
I'd rather not add these checks, I would first like to see if it would happen at all, that kind of intermittent failure.
Normally, this test would finish within 5 seconds, the video is taking 20 seconds. I think that's a long enough buffer.
Note, that this intermittent doesn't seem to happen often, either. I don't think we should spend energy on a potential problem that might not be happening at all.

Comment 8

[Martijn Wargers (dead)]

Updated pull request.
Treeherder try is passing with 20 retries: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=8f0adbf3e700

Comment 9

[Martijn Wargers (dead)]

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

Comment 10

[Martijn Wargers (dead)]

Comment on attachment 8539392 [details] [review]
is_playing

Jenkins adhoc run all green.

Comment 11

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

I'll follow up with the review in a moment, but wanted to comment on this separately, since it won't affect the review.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)

> Yeah, although I think with looking at readyState = 4, the test is following
> all the rules where a video is actually playing, so that's it's not
> potentially playing.

Doesn't matter what the test is doing. I'm talking strictly in terms of the app object, and those methods must stand alone for any possible test that uses it. It currently defines "is_playing()" as simply paused=false, and doesn't consider readyState at all. 

For that matter, neither does the test. It considers wait_for_video_loaded() as a black box, and it's not allowed to know that it's really readyState--even if you did write it. :)

> But we have to think of this test only as a UI test, where we're testing the
> video controls UI, not whether the video is actually playing correctly
> (which has been demonstrated to be intermittently failing).

Sure, the test should only consider the UI (or more accurately, the app object) as a black box. But that means the app object has to do the right thing. Since the app object in this case queries the back end instead of checking the UI, that means the back end query has to be exactly right.

> 
> > is_playing only checks attribute paused != true. So it's another one that
> > will potentially misbehave for short videos. Might want to update it to
> > paused != true && ended != true, at the very least.
> 
> When the video has ended, I think it would cause more test failures, because
> the video controls become then visible.

> I'd rather not add these checks, I would first like to see if it would
> happen at all, that kind of intermittent failure.

It's something to do in a separate bug anyway, but a fixing an API that's off-spec doesn't need to have a failure associated to be put back into spec. 

The types of problems this would cause would be pretty subtle anyway; you'd get back an is_playing of True, do something with the test based on that assumption, and it'd turn out the video ended in the background. Maybe the test doesn't use the controls, and it fouls something much less obvious--or worse, passes by mistake.

> Normally, this test would finish within 5 seconds, the video is taking 20
> seconds. I think that's a long enough buffer.
> Note, that this intermittent doesn't seem to happen often, either. I don't
> think we should spend energy on a potential problem that might not be
> happening at all.

And again, the app object can't be considered just in terms of the test using it, aside from possibly scope. It's a general API, and has to be independently correct.

Comment 12

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

Comment on attachment 8539392 [details] [review]
is_playing

Looks fine.

Comment 13

[Florin Strugariu [:Bebe]]

https://github.com/mozilla-b2g/gaia/commit/f590cab748497823ea0ea5c3bc5677dd1f47461e