970925 - convert testing/mochitest/android.json into skip-if statements in mochitest.ini files

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in our source tree, we have an older manifest format: testing/mochitest/android.json, and we need to use our manifest format that we have adopted for desktop tests: mochitest.ini.  

We will need to take each entry in the android.json and annotate the appropriate mochitest.ini file with a:
skip-if = toolkit == 'android'

We also need to retain any comments in the android.json so we can keep as much context around why these tests are skipped for the android platform.

Once this is done, we will need to keep android.json around, but for the most part keep it as an empty file:
{
"runtests": {},
"excludetests": {}
}

Currently our buildbot runners require this and we will need to adjust them via mozharness scripts once we have a cleaned out file along with correctly annotated mochitest.ini files.

Comment 1

[Geoff Brown [:gbrown]]

We also have testing/mochitest/androidx86.json, a copy of android.json with a few dozen additions, used for the Android x86 emulator tests. 

Also, I was just preparing to add a new android2_3.json file, for the Android 2.3 emulator tests, which have a dozen or so different failures...best to hold off on that?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

I know we have the android_x86.json file, I would like to do a diff of that and add the entries- a different bug.  We need to figure out how to specify x86 via mochitest.ini.

As for 2.3, how can we differentiate that in mochitest.ini?  The good news is we should be able to have the core android stuff annotated in mochitest.ini and then pass in android_x86.json which only has the unique failures for x86 until we get this all sorted out.

Comment 3

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #2)
>  We need to figure out how to specify
> x86 via mochitest.ini.

I think that is: os == "android" && processor == "x86"

> As for 2.3, how can we differentiate that in mochitest.ini?

I think that is: os == "android" && android_version == "10"

...but there may be complications -- see bug 970409.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

flaw in logic - maybe the likes of :gbrown or :ted can help me out here.

if we skip-if a test due to "toolkit == 'android'", that makes sense.  What if that test works fine on android x86 but not android [arm]?

it is easy to ensure all files in the android_x86.json are annotated in the .ini files, but now we have 300+ tests which are ignored by default just because we match 'android'.

should we diff android.json and android_x86.json to see what is unique in both files, then manually hack on those?  Right now I see 100 differences with 'diff android.json android_x86.json'.  If I remove instances of 'x86 only', then I have 17 differences, that isn't so bad.

I would like thoughts from others before going too far.

Comment 5

[Vaibhav (:vaibhav1994)]

Attachment: manifest.patch

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8374812 [details] [diff] [review]
manifest.patch

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

r- only for the missing comments, the rest are nits that need the be adjusted by hand.

::: dom/network/tests/mochitest.ini
@@ +3,1 @@
>  skip-if = toolkit == "gonk"

since this already as a skip-if for gonk, we need to make it:
skip-if = toolkit == 'gonk' || toolkit == 'android'

::: layout/base/tests/mochitest.ini
@@ +198,1 @@
>  skip-if = toolkit == "win"

another one here:
skip-if = toolkit == 'win' || toolkit == 'android'

@@ +449,1 @@
>  skip-if = true || (toolkit == "cocoa") # Bug 688128, bug 539356

and:
skip-if = true || (toolkit == 'android' || toolkit == "cocoa") # Bug 688128, bug 539356

::: testing/mochitest/android.json
@@ +32,5 @@
> +  "dom/imptests/webapps/WebStorage/tests/submissions/Infraware/test_storage_local_key.html": "",
> +  "dom/tests/mochitest/dom-level2-html": "",
> +  "toolkit/components/passwordmgr/test/test_privbrowsing_perwindowpb.html": "",
> +  "dom/tests/mochitest/geolocation/test_timeoutWatch.html": "",
> +  "dom/inputmethod": "",

we need to retain the comments from the original file in here.  I know this is autogenerated, it should be a simple adjustment to get those in here.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Joel Maher (:jmaher) from comment #4)
> should we diff android.json and android_x86.json to see what is unique in
> both files, then manually hack on those?  Right now I see 100 differences
> with 'diff android.json android_x86.json'.  If I remove instances of 'x86
> only', then I have 17 differences, that isn't so bad.

I agree, although I'd take the simple path here:
* Tests that are present in both manifests just get skip-if = os == 'android'
* Tests that are present in just android_x86.json get skip-if = os == 'android' && processor == 'x86'
* Tests that are present in just androi.json get skip-if = os == 'android' && processor == 'arm'

Comment 8

[Vaibhav (:vaibhav1994)]

Attachment: Cleaned up manifest.patch

Comment 9

[Vaibhav (:vaibhav1994)]

Attachment: Cleaned and Updated manifest.patch

This patch converts some testing/mochitest/android.json into skip-if statements in mochitest.ini files

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8374875 [details] [diff] [review]
Cleaned and Updated manifest.patch

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

I found 6 files that required combining skip-if statements while building locally.

::: content/html/document/test/mochitest.ini
@@ +57,1 @@
>  skip-if = true # Disabled permanently (bug 559932).

another case of dual skip-if, this hsould be:
skip-if = true || toolkit == 'android' # Disabled permanently (bug 559932).

::: dom/events/test/mochitest.ini
@@ +37,1 @@
>  skip-if = os == "win" # Intermittent failures, bug 921693

this needs to be combined into a single line.

::: dom/workers/test/mochitest.ini
@@ +133,1 @@
>  skip-if = (os == "win") || (os == "mac")

this needs to be combined.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: 6 files that need attention

combine the changes in this patch with your patch and we should be good!

Comment 12

[Vaibhav (:vaibhav1994)]

Attachment: Updated manifest.patch

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8374940 [details] [diff] [review]
Updated manifest.patch

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

thanks!  this is great.  I will run this on try server to ensure all is green.

Comment 14

[Vaibhav (:vaibhav1994)]

Attachment: Changes done to manifest.patch to update 'Harness_sanity' and 'robocop'

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8375676 [details] [diff] [review]
Changes done to manifest.patch to update 'Harness_sanity' and 'robocop'

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

r- as we won't build with the double skip-if in this patch.  I edited it locally prior to pushing to try and everything is green!  My development computer crashed, so I can't just pull, verify it is good and land.  Can you fix this one error in your patch and verify it applies and builds on a most recent tree?  Otherwise I will land this by Monday.

::: layout/base/tests/mochitest.ini
@@ +444,5 @@
>  [test_bug644768.html]
>  skip-if = toolkit == "win"
>  [test_flush_on_paint.html]
> +skip-if = toolkit == 'android'
> +skip-if = true || (toolkit == 'android') || (toolkit == "cocoa") # Bug 688128, bug 539356

this is correctly editing the existing line, but adding an additional skip-if.

Comment 16

[Vaibhav (:vaibhav1994)]

Attachment: manifest.patch

Hey sorry for the trivial mistake. I have updated the patch.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8376276 [details] [diff] [review]
manifest.patch

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

thanks.  Did you get a chance to see if this applies and builds locally to an updated mozilla-inbound tree?  I am not on my normal irc client, so find me in #ateam if you have questions.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: unique files to androidx86.json (1.0)

This patch is the differences that I found in androidx86.json.  Based on the comments in the tests, this seems legit.  :gbrown, if you could verify this works for x86 mochitests- I would appreciate it.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

android.json changes are inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f79959add4b3

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f79959add4b3

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: unique files to x86 + left over files in android.json (2.0)

Comment 22

[Geoff Brown [:gbrown]]

Comment on attachment 8378362 [details] [diff] [review]
unique files to x86 + left over files in android.json (2.0)

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

This version works fine -- verified on Cedar.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

androidx86.json is inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eba59bbc076

will continue to leave this open as we have another round of cleanup to do, although round 2 will be MUCH smaller in scope :)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7eba59bbc076

Comment 25

[Vaibhav (:vaibhav1994)]

Attachment: android.patch

Made changes to android.json and androidx86.json to remove the directories and add skip-ifs to [default] section.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8380633 [details] [diff] [review]
android.patch

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

this is great, I got this to pass on try!  I had to handle some bitrot

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated android directories for bitrot

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20b52b864f7f

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

pushed a hotfix as content/canvas/test/webgl was ignored, but not subdirectories of it.  This caused a failure on armv6:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aed8a1c3b28

here is the failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35744101&tree=Mozilla-Inbound

good news is we can run more tests on these builds if we want to!

Comment 30

[Vaibhav (:vaibhav1994)]

Attachment: androidupdates.patch

Updated patch to remove remaining entries of android.json except for "robocop".

Comment 31

[Vaibhav (:vaibhav1994)]

Attachment: androidupdates.patch

Removed the entries from android.json and updated fix for harness-sanity excluetests and commas in the .json files.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8389231 [details] [diff] [review]
androidupdates.patch

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

thanks for putting this together.

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f4d6270e24

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91f4d6270e24

Comment 35

[Vaibhav (:vaibhav1994)]

All the patches related to this bug have been submitted and pushed.