Closed Bug 1261158 Opened 4 years ago Closed 4 years ago

Convert image/test/mochitest/chrome.ini mochitests to something other than mochitest-chrome where appropriate

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(16 files, 16 obsolete files)

3.89 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.34 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.12 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.16 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.87 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.83 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.86 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.16 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.88 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.23 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.83 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.48 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.48 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.83 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.09 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.31 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
The tests in image/test/mochitest/chrome.ini generally look like they should run in both the parent and child process in an e10s environment. However, they are currently chrome mochitests, and so the never really run in the child process. We should convert them to mochitest-plain or browser-chrome tests instead.
Little bit harder, but SpecialPowers.wrap to the rescue!
test_animation.html and test_animation2.html are disabled, so I left those alone. test_background_image_anim.html and test_bullet_animation.html were easy to convert. test_bug415761.html seems a bit harder, since it does some stuff with file copying, and I'm not sure how that will work with http:// URIs instead of chrome:// URIs. It's probably easier to convert it to a reftest instead.
Surprisingly, embedding XUL into these tests works just fine.
In this one I removed a load event listener from the main test file because the helper file (the one that is window.open'd) already has a onload=".." that does the same thing. This was causing the test to get run twice, and fail with a "finish() was called twice" error.
I can repro the test_xultree_animation failure when I run that test in isolation - somehow there is a JS error in textbox.xml which causes the code at [1] to call finish() again. This happens during shutdown *after* the test is done running. I'm investigating.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js?rev=b78ed68af2fb#1587
With the patch on bug 1261509 (which is needed to make test_xultree_animation.xhtml pass) the try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=170818a31237&group_state=expanded. I also updated the patches to remove some of my requestFlakyTimeout additions since they turned out to be unneccessary.
Attachment #8737754 - Flags: review?(tnikkel) → review+
Comment on attachment 8737755 [details] [diff] [review]
Part 2 - Convert test_animSVGImage2.html

>+[test_animSVGImage2.html]
>+skip-if = buildapp == 'b2g' || os == 'android'

I'm curious, why do we need to skip on b2g and android?
Attachment #8737755 - Flags: review?(tnikkel) → review+
Attachment #8737757 - Flags: review?(tnikkel) → review+
Attachment #8737758 - Flags: review?(tnikkel) → review+
Attachment #8737759 - Flags: review?(tnikkel) → review+
Attachment #8737760 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #37)
> I'm curious, why do we need to skip on b2g and android?

Well in the chrome.ini it has the skip-if in the DEFAULT section at the top of the file, so I carried it over into mochitest.ini. I suspect chrome mochitests are generally disabled on Fennec/B2G because their architecture is so different. We could probably enable these tests for Fennec at least as plain mochitests, but I didn't want to do that as part of this bug because it's expanding the scope. We can do that as a follow-up if you want.
Attachment #8737761 - Flags: review?(tnikkel) → review+
Attachment #8737762 - Flags: review?(tnikkel) → review+
Attachment #8737763 - Flags: review?(tnikkel) → review+
Attachment #8737764 - Flags: review?(tnikkel) → review+
Attachment #8737766 - Flags: review?(tnikkel) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #38)
> Well in the chrome.ini it has the skip-if in the DEFAULT section at the top
> of the file, so I carried it over into mochitest.ini.

Oh, I didn't see that.
Attachment #8737767 - Flags: review?(tnikkel) → review+
Attachment #8737768 - Flags: review?(tnikkel) → review+
Attachment #8737769 - Flags: review?(tnikkel) → review+
Attachment #8737770 - Flags: review?(tnikkel) → review+
Attachment #8737771 - Flags: review?(tnikkel) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> test_animation.html and test_animation2.html are disabled, so I left those
> alone.

I did a try push with all disabled imagelib tests re-enabled. These two didn't fail, same with test_undisplayed_iframe.html. I'm triggering a bunch more jobs to see if they fail intermittently.
Re-enabled the three disabled mochitest-chrome tests in bug 1262269. They will need to be converted too, I'll do that in another bug.
Thanks! I also uplifted these patches to aurora since the e10s folks prefer that any e10s-related test changes be uplifted to 47 as well.

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=f5f4bbaf8f02
Depends on: 1263474
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.