Get layout/base/tests mochitests to run on Android

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dholbert, Assigned: RyanVM)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

Apparently we've pretty much never run the mochitests in layout/base/tests on Android.

Right now, this exclusion happens via a "skip-if" line at the top of layout/base/tests/mochitest.ini:
http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/layout/base/tests/mochitest.ini#2-3

...but previously it happened via an exclusion in testing/mochitest/android.json:
http://searchfox.org/mozilla-central/rev/b70337e19a136ae911afc40e3aa0866973765834/testing/mochitest/android.json#331

...and before that (2012-ish) this directory simply wasn't on the whitelist of tests/directories that opted in for android testing.  (we changed from a whitelist to the above-linked android.json blacklist in bug 761125)


I suspect/hope we don't need to exclude this folder of tests anymore.  (Perhaps we still need to exclude specific tests if they're slow, but not the whole folder.)
Note that some of these tests have B2G special cases in them. And:
 (A) That probably indicates that they're fine to run on Android as well (if they worked on B2G, they're likely to also work on Android)
 (B) We probably want to convert the B2G special cases (in test_event_target_radius.html in particular) to be an Android special-case instead.
(Assignee)

Comment 3

a year ago
Checkpoint #2 - this is still expected to have failures, albeit a lot less. I also ended up making a Part 0 patch that cleans up the manifest. I will probably get review for that and land it ASAP if the Try run doesn't show any obvious fallout since I expect it to be bitrot-prone.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f2900eef18790c3c16e15c63ed1a07d284744cc
(Assignee)

Comment 4

a year ago
Created attachment 8857228 [details] [diff] [review]
clean up layout/base/tests mochitest manifest

I want to get this landed ASAP since I suspect it'll be highly bitrot-prone. This doesn't make any functional changes to what runs yet, just makes the manifest a lot easier to look at and work with for the subsequent patch.

Try runs for sanity:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e9ccbfe67aad8dc7a1591ed2227f82ba41f6b8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09e02a596c8279ac1478a2979684aba7598c1a1a
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #8857228 - Flags: review?(dholbert)
(Assignee)

Comment 5

a year ago
Comment on attachment 8857228 [details] [diff] [review]
clean up layout/base/tests mochitest manifest

Looking at the final product might be easier from a reviewing standpoint too:
https://hg.mozilla.org/try/file/09e02a596c82/layout/base/tests/mochitest.ini
Comment on attachment 8857228 [details] [diff] [review]
clean up layout/base/tests mochitest manifest

Review of attachment 8857228 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

Sadly, it's pretty hard to validate that nothing's getting lost/duplicated/accidentally-removed here, since so much is moving around. :( It would be much easier to validate this it were split into 2 pieces:
 part 1: reorder the [test_stuff.html] listings to be in alphabetical order
 part 2: distributes the needlessly-global "support-files" amongst the now-nicely-sorted [test_stuff] lines.

Each of those would be much easier to validate independently, I think...  Would you be up for splitting it up like that?

::: layout/base/tests/mochitest.ini
@@ +60,5 @@
> +[test_bug667512.html]
> +[test_bug677878.html]
> +[test_bug696020.html]
> +[test_bug582771.html]
> +[test_bug583889.html]

This section right around here seems out of order -- you've got e.g.
[test_bug588174.html]
[test_bug607529.html]
...and then a few lines lower it jumps back into the 5*** range:
[test_bug582771.html]
Comment on attachment 8857228 [details] [diff] [review]
clean up layout/base/tests mochitest manifest

Per IRC, my afore-suggested split would probably not be worth it, so never mind about that.  I reviewed this by piping the file with & without the patch through "sort", and comparing the results in a merge tool. (to be sure no lines got accidentally lost/introduced)

The changes look sane. (The diffs are mostly from support-file-on-its-own-line being converted to support-files = filename.)

If you'd be up for splitting the non-reordering/redistributing edits (the "skip-if" changes) into their own patch, I would encourage you to do that (from the perspective of separating code-movement/rearrangement from code-edits).  But, it's not a huge deal; r=me either way.
Attachment #8857228 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

a year ago
Per our IRC discussion, I split the two skip-if changes into another patch to show a clearer decision that went into the changes. I confirmed that they folded up into the same patch as the original monolithic version. Thanks for the quick review!
Keywords: leave-open

Comment 9

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d37ac1baa67f
Clean up the test sorting in layout/base/tests/mochitest.ini. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac47baff00af
Clean up some redundant annotations in layout/base/tests/mochitest.ini. r=dholbert
(Assignee)

Comment 10

a year ago
I'm expecting this to be green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d18572d6dccaf434f272a0af0cf6ccbf85a6f2d

I've been poking at test_event_target_radius.html and haven't been able to get it to pass even though I'm pretty sure I got the right substitute for the B2G detection line. Basically, it ends up hitting a version of bug 1349169 (or times out if I remove the special-casing entirely). At this point, I'm inclined to spin off fixing & re-enabling to its own bug along with the other ones marked "Bug TBD".
(Assignee)

Comment 12

a year ago
Comment on attachment 8857512 [details] [diff] [review]
run layout/base/tests on Android and skip failing tests

Green on Try.
Attachment #8857512 - Flags: review?(dholbert)
Comment on attachment 8857512 [details] [diff] [review]
run layout/base/tests on Android and skip failing tests

Review of attachment 8857512 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8857512 - Flags: review?(dholbert) → review+

Comment 14

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01c53212dde
Run layout/base/tests on Android and skip failing tests. r=dholbert
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d37ac1baa67f
https://hg.mozilla.org/mozilla-central/rev/ac47baff00af
https://hg.mozilla.org/mozilla-central/rev/d01c53212dde
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.