Get layout/base/tests mochitests to run on Android

RESOLVED FIXED in Firefox 55



a year ago
a year ago


(Reporter: dholbert, Assigned: RyanVM)



Firefox Tracking Flags

(firefox55 fixed)



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

...but previously it happened via an exclusion in testing/mochitest/android.json:

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

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.

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:
Assignee: nobody → ryanvm
Attachment #8857228 - Flags: review?(dholbert)

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:
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.
...and then a few lines lower it jumps back into the 5*** range:
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+

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
Clean up the test sorting in layout/base/tests/mochitest.ini. r=dholbert
Clean up some redundant annotations in layout/base/tests/mochitest.ini. r=dholbert

Comment 10

a year ago
I'm expecting this to be green.

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

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

Comment 14

a year ago
Pushed by
Run layout/base/tests on Android and skip failing tests. r=dholbert


a year ago
Keywords: leave-open

Comment 15

a year ago
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.