Closed Bug 1251013 Opened 5 years ago Closed 4 years ago

Remove Android 9-specific annotations from test manifests

Categories

(Firefox for Android :: Testing, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(4 files)

Some tests are disabled specifically on Android 2.3. Once 2.3 is retired, we can remove those skip-if, fail-if annotations from manifests.
See Also: → 979921
While I'm at it...

 - "AndroidVersion == 15" likely means Android 4.0 tests on Pandaboards, no longer run -- these can be eliminated.
 - "AndroidVersion >= 15" is redundant, in that Android 15 is the minimum supported version now -- these can be replaced with "Android".
 - "AndroidVersion == 17" likely means Android 4.2 on x86, where we only run xpcshell now -- these can be eliminated.

Really, since most tests only run on Android 4.3, any AndroidVersion/android_version annotation can probably be simplified.
The scope of this bug expanded to update test manifests for Android - remove/simplify anything related to tegras, pandas, android 2.3, or android x86 (except xpcshell). I think that's good.

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

I'll update all the affected bugs once this lands.
Attachment #8731538 - Flags: review?(jmaher)
Attachment #8731540 - Flags: review?(jmaher)
Attachment #8731541 - Flags: review?(jmaher)
Attachment #8731542 - Flags: review?(jmaher)
I notice there's a failure in the try run on dom/canvas/test/reftest/drawFocusIfNeeded.html on OS X 10.10. This had the curious annotation "skip-if(Android&&AndroidVersion<15,8,500)" before...I wonder how that's interpretted.
Comment on attachment 8731538 [details] [diff] [review]
update manifests - robocop

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

out of curiosity, are we testing these disabled tests on non 4.3?  It seems most of them were disabled on 2.3 and 4.3, have they ever worked?  can we just get rid of them?
Attachment #8731538 - Flags: review?(jmaher) → review+
Comment on attachment 8731540 [details] [diff] [review]
update manifests - xpcshell

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

::: image/test/unit/xpcshell.ini
@@ +40,5 @@
>  [test_async_notification_animated.js]
>  [test_encoder_apng.js]
>  [test_encoder_png.js]
>  [test_imgtools.js]
>  # Bug 676968: test fails consistently on Android; crashes on 4.3 (bug 1156452)

can we clean this comment up?
Attachment #8731540 - Flags: review?(jmaher) → review+
Comment on attachment 8731541 [details] [diff] [review]
update manifests - reftests

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

lots of nits, mostly around bugs in the comments.

::: dom/canvas/test/reftest/reftest.list
@@ +153,5 @@
>  == stroketext-shadow.html stroketext-shadow-ref.html
>  
>  # focus rings
> +pref(canvas.focusring.enabled,true) skip-if(B2G) skip-if(winWidget) needs-focus == drawFocusIfNeeded.html drawFocusIfNeeded-ref.html
> +pref(canvas.customfocusring.enabled,true) skip-if(B2G) skip-if(Android) skip-if(winWidget) needs-focus == drawCustomFocusRing.html drawCustomFocusRing-ref.html

for drawFocusIfNeeded you removed all android bits, but for drawCustomFocusring you have a simplified skip-if(Android)

I think based on your try run, we should have skip-if(Android) on both.

::: gfx/tests/reftest/reftest.list
@@ +1,4 @@
>  # 468496-1 will also detect bugs in video drivers.
>  == 468496-1.html 468496-1-ref.html
>  fuzzy-if(winWidget,175,443) == 611498-1.html 611498-ref.html
> +skip-if(B2G) fuzzy-if(Android,8,1000) == 709477-1.html 709477-1-ref.html # bug 773482

is this a bug that we plan to fix?

::: layout/reftests/first-letter/reftest.list
@@ +34,5 @@
>  == 23605-6.html 23605-6-ref.html
>  != 229764-1.html 229764-ref.html
>  == 229764-2.html 229764-ref.html
>  == 329069-1.html 329069-1-ref.html
> +fails-if(Android) == 329069-2.html 329069-2-ref.html # Bug 999139

does this referenced bug have work items associated with it?

::: layout/reftests/font-face/reftest.list
@@ +6,5 @@
>  HTTP(..) == download-2.html download-2-ref.html
>  HTTP(..) != download-2.html about:blank
>  random-if(winWidget) HTTP(..) == download-2-big.html download-2-big-otf.html # bug 470713
>  HTTP(..) != download-2-big-otf.html about:blank
> +asserts-if(Android&&!asyncPan,4-8) HTTP(..) != download-3-notref.html download-3.html # bug 1019192, bug 936226

I think one of these bugs need to be removed from the comment.
Attachment #8731541 - Flags: review?(jmaher) → review+
Comment on attachment 8731542 [details] [diff] [review]
update manifests - mochitests

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

looking good, a few nits.

::: dom/html/test/mochitest.ini
@@ +403,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
>  [test_bug658746.html]
>  [test_bug659596.html]
>  [test_bug659743.xml]
> +skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage 

nit: trailing whitespace is added here.

::: dom/media/tests/mochitest/identity/mochitest.ini
@@ +2,4 @@
>  # won't run on b2g desktop tests - bug 1119993
>  # broken HTTPS on b2g emulator - bug 1135339
>  subsuite = media
> +skip-if = android_version == '18' || (buildapp == 'b2g' && toolkit != 'gonk') || (buildapp == 'b2g' && toolkit == 'gonk') || buildapp == 'mulet'

do we not have a bug for android version 18?
Attachment #8731542 - Flags: review?(jmaher) → review+
http://hg.mozilla.org/mozilla-central/rev/10cf6a630944 (bug 540456) introduced drawFocusIfNeeded.html (originally named drawSystemFocusRing.html) and drawCustomFocusRing.html and used "skip-if(Android&&AndroidVersion<15,8,500)", which appears to evaluate to skip-if(true): I think those 2 tests have never run on any platform. I'll update the manifest to skip on failing platforms and report a new bug.
(In reply to Joel Maher (:jmaher) from comment #8)
> out of curiosity, are we testing these disabled tests on non 4.3?  It seems
> most of them were disabled on 2.3 and 4.3, have they ever worked?  can we
> just get rid of them?

The mobile team does some local testing, so it's possible, but otherwise, no. :margaret is trying to determine if those tests can be removed.
(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 8731540 [details] [diff] [review]
> update manifests - xpcshell
> 
> Review of attachment 8731540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/test/unit/xpcshell.ini
> @@ +40,5 @@
> >  [test_async_notification_animated.js]
> >  [test_encoder_apng.js]
> >  [test_encoder_png.js]
> >  [test_imgtools.js]
> >  # Bug 676968: test fails consistently on Android; crashes on 4.3 (bug 1156452)
> 
> can we clean this comment up?

Yes - thanks! After reviewing both bugs, I'm going with simply:

# Bug 676968
(In reply to Joel Maher (:jmaher) from comment #10)
> ::: gfx/tests/reftest/reftest.list
> @@ +1,4 @@
> >  # 468496-1 will also detect bugs in video drivers.
> >  == 468496-1.html 468496-1-ref.html
> >  fuzzy-if(winWidget,175,443) == 611498-1.html 611498-ref.html
> > +skip-if(B2G) fuzzy-if(Android,8,1000) == 709477-1.html 709477-1-ref.html # bug 773482
> 
> is this a bug that we plan to fix?

Bug 773482 is "enable reftests on B2G". I question if it will be fixed, but I am reluctant to make changes unrelated to Android in this bug.

> ::: layout/reftests/first-letter/reftest.list
> @@ +34,5 @@
> >  == 23605-6.html 23605-6-ref.html
> >  != 229764-1.html 229764-ref.html
> >  == 229764-2.html 229764-ref.html
> >  == 329069-1.html 329069-1-ref.html
> > +fails-if(Android) == 329069-2.html 329069-2-ref.html # Bug 999139
> 
> does this referenced bug have work items associated with it?

Bug 999139 was opened for Android 2.3 and not updated for 4.3 -- until now.

> ::: layout/reftests/font-face/reftest.list
> @@ +6,5 @@
> >  HTTP(..) == download-2.html download-2-ref.html
> >  HTTP(..) != download-2.html about:blank
> >  random-if(winWidget) HTTP(..) == download-2-big.html download-2-big-otf.html # bug 470713
> >  HTTP(..) != download-2-big-otf.html about:blank
> > +asserts-if(Android&&!asyncPan,4-8) HTTP(..) != download-3-notref.html download-3.html # bug 1019192, bug 936226
> 
> I think one of these bugs need to be removed from the comment.

936226 (x86) removed.
(In reply to Joel Maher (:jmaher) from comment #11)
> ::: dom/media/tests/mochitest/identity/mochitest.ini
> @@ +2,4 @@
> >  # won't run on b2g desktop tests - bug 1119993
> >  # broken HTTPS on b2g emulator - bug 1135339
> >  subsuite = media
> > +skip-if = android_version == '18' || (buildapp == 'b2g' && toolkit != 'gonk') || (buildapp == 'b2g' && toolkit == 'gonk') || buildapp == 'mulet'
> 
> do we not have a bug for android version 18?

Good catch -- that should still be bug 981881 (bug needs updating to make that clearer).
You need to log in before you can comment on or make changes to this bug.