Closed Bug 936720 Opened 9 years ago Closed 9 years ago

Move animated image operator tests from reftests to mochitests

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
firefox28 --- fixed
firefox-esr24 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

Bug 896684 is a top orange that involves the current tests for animated GIF/PNG operators (found in image/tests/reftest/animated) intermittently failing to complete the test animation. This only happens on the B2G emulator, which runs in a VM. The problem is that the reftest expects the animation to be complete after 1 second, and whether because of wonky VM timing or just random slowness in the test environment, we don't always hit that deadline.

Other tests in the codebase have solved this problem in similar cases by using a mochitest instead of a reftest and taking snapshots repeatedly until either the test passes or the test harness times out. This means that in most cases the test finishes as quickly as possible, but in the rare case where the test takes more time we keep retrying until it passes.

We should do the same thing for these tests. One or two new mochitests need to be added that perform the same task as the existing animation operator tests, and then the old tests need to be removed.
Summary: Move animate image operator tests from reftests to mochitests → Move animation image operator tests from reftests to mochitests
Summary: Move animation image operator tests from reftests to mochitests → Move animated image operator tests from reftests to mochitests
Here's the new test.
Attachment #829662 - Flags: review?(josh)
And here we remove the old tests.
Attachment #829663 - Flags: review?(josh)
Here's a try job to verify that the new mochitest works:

https://tbpl.mozilla.org/?tree=Try&rev=b7018fd0f20b
Here's a single-platform try job to verify that removing the reftests didn't break anything:

https://tbpl.mozilla.org/?tree=Try&rev=068fd5847440
Attachment #829662 - Flags: review?(josh) → review+
Comment on attachment 829663 [details] [diff] [review]
(Part 2) - Remove reftests for animated image operators.

Leave both sets (the old reftests and the new mochitests) to run in parallel for a bit to make sure we have all the issues sorted out? ie land this a little later
Attachment #829663 - Flags: review?(josh) → review+
Thanks for the review, Timothy! As per our discussion, it'd be a good idea to add the sync image decode flag when we do our drawing in the test. I'll add that before landing.
(In reply to Timothy Nikkel (:tn) from comment #5)
> Leave both sets (the old reftests and the new mochitests) to run in parallel
> for a bit to make sure we have all the issues sorted out? ie land this a
> little later

Sure, sounds good.
(In reply to Seth Fowler [:seth] from comment #6)
> Thanks for the review, Timothy! As per our discussion, it'd be a good idea
> to add the sync image decode flag when we do our drawing in the test. I'll
> add that before landing.

Actually this is the default; you need to explicitly request async decode. See here:

https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4462
Oh, good to know.
Attachment #829662 - Flags: checkin+
Pushed part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/957acbe8ca22

Setting needinfo to remind myself to push part 2 once part 1 has cooked for a while.
Flags: needinfo?(seth)
Whiteboard: [leave open]
All looks well. Pushed part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a3b4b9b786c
Whiteboard: [leave open]
Attachment #829663 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/8a3b4b9b786c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Per discussion with Ryan, we should uplift these as far back as they can go. To ESR24 ideally. (These are test-only patches, and the tests in question are ancient.)
Flags: needinfo?(seth)
Keywords: checkin-needed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.