Closed Bug 650557 Opened 9 years ago Closed 6 years ago

test_canvas.html doesn't really need to set 2.5 and 5 second timeouts

Categories

(Core :: Canvas: 2D, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: ehsan, Unassigned)

References

Details

Attachments

(1 file)

I don't see any reason why the test really wants such magic timeout numbers.  I caught this over in bug 649012.
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #526521 - Flags: review?(jmuizelaar)
No longer blocks: 640912
Blocks: FlakyTimeout
Comment on attachment 526521 [details] [diff] [review]
Patch (v1)

># HG changeset patch
># Parent 7ad68dea9c88a0fd623c580aeb0c1fcab89e8802
># User Ehsan Akhgari <ehsan@mozilla.com>
>Bug 650557 - test_canvas.html doesn't really need to set 2.5 and 5 second timeouts
>
>
>diff --git a/content/canvas/test/test_canvas.html b/content/canvas/test/test_canvas.html
>--- a/content/canvas/test/test_canvas.html
>+++ b/content/canvas/test/test_canvas.html
>@@ -2832,17 +2832,17 @@ var isDone_test_2d_drawImage_animated_ap
> 
> function test_2d_drawImage_animated_apng() {
> 
> deferTest();
> setTimeout(wrapFunction(function () {
>     ctx108.drawImage(document.getElementById('anim-gr_1.png'), 0, 0);
>     isPixel(ctx108, 50,25, 0,255,0,255, 2);
> 	isDone_test_2d_drawImage_animated_apng = true;
>-}), 5000);
>+}), 0);

These change the tests so that it's not testing what it's supposed to be anymore.
This is testing that drawImage draws the first frame of an animated image. The large timeout is there to ensure that
the image has already animated. Plus these tests are derived from an upstream source, so I'm more hesitant to muck with them.
Attachment #526521 - Flags: review?(jmuizelaar) → review-
Do we know what these tests are really waiting on, so that we can wait for that event, and not just an arbitrary amount of time?
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> Do we know what these tests are really waiting on, so that we can wait for
> that event, and not just an arbitrary amount of time?

Jeff?  :-)
Flags: needinfo?(jmuizelaar)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> Do we know what these tests are really waiting on, so that we can wait for
> that event, and not just an arbitrary amount of time?

I assume they are waiting for the animation which is specified in ms to happen.
Flags: needinfo?(jmuizelaar)
Assignee: ehsan → nobody
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #3)
> > Do we know what these tests are really waiting on, so that we can wait for
> > that event, and not just an arbitrary amount of time?
> 
> I assume they are waiting for the animation which is specified in ms to
> happen.

So it is expected to happen in a specified # of ms?  Is there an event fired when the animation is done that could be used as an alternative to waiting?
Flags: needinfo?(jmuizelaar)
(In reply to Andrew Overholt [:overholt] from comment #6)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #3)
> > > Do we know what these tests are really waiting on, so that we can wait for
> > > that event, and not just an arbitrary amount of time?
> > 
> > I assume they are waiting for the animation which is specified in ms to
> > happen.
> 
> So it is expected to happen in a specified # of ms?  Is there an event fired
> when the animation is done that could be used as an alternative to waiting?

I believe so yes. There are no animation events.
Flags: needinfo?(jmuizelaar)
I don't think there's anything to be done here.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.