Closed Bug 1113219 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: RyanVM, Assigned: martijn.martijn)

References

()

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

46 bytes, text/x-github-pull-request
gmealer
: review+
Bebe
: review+
Details | Review
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
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.
Assignee: nobody → martijn.martijn
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.
Attached file 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)
Attachment #8539392 - Flags: review?(gmealer)
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.
Attachment #8539392 - Flags: review?(gmealer) → review+
(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.
Updated pull request.
Treeherder try is passing with 20 retries: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=8f0adbf3e700
Comment on attachment 8539392 [details] [review]
is_playing

Jenkins adhoc run all green.
Attachment #8539392 - Flags: review?(gmealer)
Attachment #8539392 - Flags: review?(florin.strugariu)
Attachment #8539392 - Flags: review+
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 on attachment 8539392 [details] [review]
is_playing

Looks fine.
Attachment #8539392 - Flags: review?(gmealer) → review+
Attachment #8539392 - Flags: review?(florin.strugariu) → review+
https://github.com/mozilla-b2g/gaia/commit/f590cab748497823ea0ea5c3bc5677dd1f47461e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.