Video mochitest test_seek5.html fails on Vista

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Audio/Video
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
mozilla1.9.1b2
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Mochitest /test/content/media/video/test/test_seek5.html fails on vista.

Test description: "Test for a seek, followed by another seek before the first is complete."
It passes on XP, and the error is "test timed out".
So the problem here is that the test has a timer which declares seek failure after 30 seconds. Unfortunately, Vista needs longer than 30 seconds to complete the seek (at least my laptop does...) so the fix for this would be to increase the timeout. The mochitests are slower on Vista anyway, I think due to the mochitest webserver running slower, and maybe due to slower IO.

The problem is that increasing the timeout may not fix this long term, it could fail intermittently in the future.
Created attachment 346146 [details] [diff] [review]
Increase timeout on test

Increases timeout coz Vista is slow.

Other test cases don't timeout on Vista, for me at least.
Assignee: nobody → chris
Attachment #346146 - Flags: superreview?(roc)
Attachment #346146 - Flags: review?(chris.double)
Created attachment 346156 [details] [diff] [review]
v1 with cruft removed.
Attachment #346146 - Attachment is obsolete: true
Attachment #346156 - Flags: superreview?(roc)
Attachment #346156 - Flags: review?(chris.double)
Attachment #346146 - Flags: superreview?(roc)
Attachment #346146 - Flags: review?(chris.double)
Attachment #346156 - Flags: superreview?(roc)
Attachment #346156 - Flags: superreview+
Attachment #346156 - Flags: review?(chris.double)
Attachment #346156 - Flags: review+
60 seconds sounds awfully long though. We really need to speed up that test somehow.
Created attachment 346179 [details] [diff] [review]
Patch 2 - increase timeouts on all media tests

This increases timeouts of all tests in media/content from 30 seconds to 60 seconds. They will only cause slowdown when our tests are failing, and should alleviate intermittent failures.
Attachment #346156 - Attachment is obsolete: true
Attachment #346179 - Flags: superreview+
Attachment #346179 - Flags: review+
Keywords: checkin-needed
Actually, we should take out these timeouts completely and rely on the standard mochitest timeout.

Also, we should not be using Math.random in test_seek7.html or anywhere else. Our tests need to be reproducible.
In test_seek7, do we do a new HTTP load on each seek, or do we coalesce?
We do a new load. In fact, we do *many* new loads as liboggplay roams around looking for the right position.
Given that, there's no guarantee whatsoever that test_seek7.html will ever converge (since it waits for 3 _completed_ seeks).  It'll pass only by accident if enough seeks are close enough that the load + seek finishes in under 10ms, no?
No, because if we start a seek while another seek is in progress, the first seek will complete and fire SeekEnded and then we'll begin the next seek (actually we'll seek directly to the *latest* value currentTime has been set to).

Comment 12

9 years ago
Someone needs to write a 'Mochitest for Dummies'. I used the timeouts as other code was doing that and I copied it. What is the 'standard mochitest timeout'? Can someone point me to a test that uses it for guidance?
> If we start a seek while another seek is in progress, the first
> seek will complete and fire SeekEnded and then we'll begin the next seek

Ah, ok.  Is there a reason to not start the next seek from SeekEnded, then?  Or is that the behavior we're testing here?

> What is the 'standard mochitest timeout'?

The script that starts the test build kills it if a certain amount of time passes with no output.  There is nothing you need to put in your test to take advantage of this: if the test takes too long, the test harness will kill the browser and report a timeout failure.
By the way, what other code was doing manual timeouts?

Comment 15

9 years ago
>Ah, ok.  Is there a reason to not start the next seek from SeekEnded, then?
>Or is that the behavior we're testing here?

That's the behaviour we're testing. It was duplicating a problem that Chris Pearce found doing exactly that - random seeks in a setTimeout.

> By the way, what other code was doing manual timeouts?

I'll take a look. It may have been a newsgroup post or something from the net.
(In reply to comment #13)
> > If we start a seek while another seek is in progress, the first
> > seek will complete and fire SeekEnded and then we'll begin the next seek
> 
> Ah, ok.  Is there a reason to not start the next seek from SeekEnded, then?  Or
> is that the behavior we're testing here?

Yeah. We need to test what happens when someone sets currentTime while a seek is in progress.
Created attachment 346321 [details] [diff] [review]
Patch v3 - Remove all timeouts from media tests
[Checkin: Comment 23]

This removes all timeouts from media tests.
Attachment #346179 - Attachment is obsolete: true
Attachment #346321 - Flags: review?(roc)
(In reply to comment #17)
> Created an attachment (id=346321) [details]
> Patch v3 - Remove all timeouts from media tests
> 
> This removes all timeouts from media tests.

Oh yeah, they still pass on Vista too. ;)

Updated

9 years ago
Blocks: 461896
Attachment #346321 - Flags: review?(roc) → review+

Comment 19

9 years ago
From: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225954087.1225958584.25116.gz#err0

*** 27734 INFO Running /tests/content/media/video/test/test_volume.html...
NEXT ERROR *** 27735 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_volume.html | Test timed out

Why would that timeout? It doesn't use setTimeout. So if that's timing out, maybe the use of setTimeout in the others isn't the issue?
Duplicate of this bug: 463469
(In reply to comment #19)
> Why would that timeout? It doesn't use setTimeout. So if that's timing out,
> maybe the use of setTimeout in the others isn't the issue?

This is an intermittent failure?

It looks like none of the is() tests ran in test_volume.html, so I assume it's blocked before the first is() test, in video.volume setter. I don't see how nsHTMLMediaElement::SetVolume() could block though. Maybe this is related to the problem kinetik found in bug 449315?
test_volume.html doesn't load any media, so it'll bail in PickMediaElement without initializing a decoder (and therefore without an audio backend), so I don't think it can be related to the problem I ran into with the wave tests.
Pushed b5f3b30402cb.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 24

9 years ago
Comment on attachment 346321 [details] [diff] [review]
Patch v3 - Remove all timeouts from media tests
[Checkin: Comment 23]

>   ok(thrown2, "Setting currentTime to invalid value of a function");

This should test thrown3.
Attachment #346321 - Attachment description: Patch v3 - Remove all timeouts from media tests → Patch v3 - Remove all timeouts from media tests [Checkin: Comment 23]
Attachment #346321 - Attachment is obsolete: true
Attachment #346321 - Attachment is obsolete: false
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1b2
This is another related loose end:

Chris Double, bug 462933 comment #19
> From:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225954087.1225958584.25116.gz#err0
> 
> *** 27734 INFO Running /tests/content/media/video/test/test_volume.html...
> NEXT ERROR *** 27735 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/media/video/test/test_volume.html | Test timed out
> 
> Why would that timeout? It doesn't use setTimeout. So if that's timing out,
> maybe the use of setTimeout in the others isn't the issue?
(In reply to comment #25)
> This is another related loose end:

I mean still unresolved.
Did comment 24 ever get addressed?
Not yet, but I've fixed it (and another test typo) as part of bug 469255.
Component: Video/Audio → Tracking
Component: Tracking → Video/Audio
You need to log in before you can comment on or make changes to this bug.