Closed Bug 970925 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: vaibhav1994)

References

Details

(Whiteboard: [leave open])

Attachments

(4 files, 9 obsolete files)

87.76 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
26.21 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
10.52 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
34.62 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
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.
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?
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.
Assignee: nobody → vaibhavmagarwal
(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.
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.
Flags: needinfo?(gbrown)
Attached patch manifest.patch (obsolete) — Splinter Review
Attachment #8374812 - Flags: review?(jmaher)
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.
Attachment #8374812 - Flags: review?(jmaher) → review-
(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'
Attached patch Cleaned up manifest.patch (obsolete) — Splinter Review
Attachment #8374856 - Flags: review?(jmaher)
This patch converts some testing/mochitest/android.json into skip-if statements in mochitest.ini files
Attachment #8374875 - Flags: review?(jmaher)
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.
Attachment #8374875 - Flags: review?(jmaher) → review-
Attached patch 6 files that need attention (obsolete) — Splinter Review
combine the changes in this patch with your patch and we should be good!
Attachment #8374812 - Attachment is obsolete: true
Attachment #8374856 - Attachment is obsolete: true
Attachment #8374856 - Flags: review?(jmaher)
Flags: needinfo?(gbrown)
Attached patch Updated manifest.patch (obsolete) — Splinter Review
Attachment #8374940 - Flags: review?(jmaher)
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.
Attachment #8374940 - Flags: review?(jmaher) → review+
Attachment #8374875 - Attachment is obsolete: true
Attachment #8374940 - Attachment is obsolete: true
Attachment #8375676 - Flags: review?(jmaher)
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.
Attachment #8375676 - Flags: review?(jmaher) → review-
Attached patch manifest.patchSplinter Review
Hey sorry for the trivial mistake. I have updated the patch.
Attachment #8375676 - Attachment is obsolete: true
Attachment #8376276 - Flags: review?(jmaher)
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.
Attachment #8376276 - Flags: review?(jmaher) → review+
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.
Attachment #8374925 - Attachment is obsolete: true
Attachment #8376695 - Flags: review?(gbrown)
Whiteboard: [leave open]
Attachment #8376695 - Attachment is obsolete: true
Attachment #8376695 - Flags: review?(gbrown)
Attachment #8378362 - Flags: review?(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.
Attachment #8378362 - Flags: review?(gbrown) → review+
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 :)
Attached patch android.patch (obsolete) — Splinter Review
Made changes to android.json and androidx86.json to remove the directories and add skip-ifs to [default] section.
Attachment #8380633 - Flags: review?(jmaher)
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
Attachment #8380633 - Flags: review?(jmaher) → review+
Attachment #8380633 - Attachment is obsolete: true
Attachment #8386942 - Flags: review+
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!
Attached patch androidupdates.patch (obsolete) — Splinter Review
Updated patch to remove remaining entries of android.json except for "robocop".
Attachment #8389128 - Flags: review?(jmaher)
Removed the entries from android.json and updated fix for harness-sanity excluetests and commas in the .json files.
Attachment #8389128 - Attachment is obsolete: true
Attachment #8389128 - Flags: review?(jmaher)
Attachment #8389231 - Flags: review?(jmaher)
Comment on attachment 8389231 [details] [diff] [review]
androidupdates.patch

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

thanks for putting this together.
Attachment #8389231 - Flags: review?(jmaher) → review+
All the patches related to this bug have been submitted and pushed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 984523
You need to log in before you can comment on or make changes to this bug.