1355206 - Get layout/base/tests mochitests to run on Android

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Just to get the lay of the land:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a35de245fa00bd6d0028ecccd80ec900fea85b3c

Comment 3

[Ryan VanderMeulen [:RyanVM]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Attachment: 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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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 6

[Daniel Holbert [:dholbert]]

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 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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".

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Attachment: run layout/base/tests on Android and skip failing tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca702bb97510ee64bef8bc5e896c4c258dfb0fc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Green on Try.

Comment 13

[Daniel Holbert [: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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01c53212dde
Run layout/base/tests on Android and skip failing tests. r=dholbert

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d37ac1baa67f
https://hg.mozilla.org/mozilla-central/rev/ac47baff00af
https://hg.mozilla.org/mozilla-central/rev/d01c53212dde