Closed Bug 936226 Opened 6 years ago Closed 4 years ago

"Green up" remaining test failures for Android x86 emulator

Categories

(Testing :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

(Keywords: ateam-mobile-big)

Attachments

(7 files)

I need to land several trivial patches to update test manifests, disabling the remaining test failures for Android x86 emu.
Whiteboard: [leave open]
There are several reftest UNEXPECTED-PASS tests. All of these are currently marked fail-if(Android), or similar, like fail-if(Android&&AndroidVersion>=15).

Do we want to change to:

   fail-if(Android&&AndroidVersion!=17)

or introduce an X86/ARM distinction, as proposed in bug 875801:

   fail-if(Android&&ARM)   /  fail-if(Android&&!AndroidX86)    /    fail-if(AndroidARM)   /   ???



REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8856/tests/layout/reftests/bugs/650228-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8856/tests/layout/reftests/bugs/654950-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8856/tests/layout/reftests/bugs/718521.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8858/tests/layout/reftests/transform-3d/rotatex-perspective-3a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8858/tests/layout/reftests/transform-3d/scale3d-all.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8858/tests/layout/reftests/transform-3d/scale3d-all-separate.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-4b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-4d.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-4f.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-5c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-5d.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-9a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-9b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-9c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-9d.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-9e.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/243519-9f.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/402807-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/409659-1c.html | image comparison (!=), max difference: 255, number of differing pixels: 300
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/433700.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/458487-4a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/458487-4b.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/458487-4c.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/579323-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/586683-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/602200-4.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/617242-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/633344-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | http://10.0.2.2:8854/tests/layout/reftests/bugs/635302-1.html | image comparison (==)
Comment on attachment 828923 [details] [diff] [review]
(1) disable robocop testBookmark, testReaderMode, testAddonManager on x86

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

looks good
(In reply to Geoff Brown [:gbrown] from comment #2)
> There are several reftest UNEXPECTED-PASS tests. All of these are currently
> marked fail-if(Android), or similar, like
> fail-if(Android&&AndroidVersion>=15).
> 
> Do we want to change to:
> 
>    fail-if(Android&&AndroidVersion!=17)
> 
> or introduce an X86/ARM distinction, as proposed in bug 875801:
> 
>    fail-if(Android&&ARM)   /  fail-if(Android&&!AndroidX86)    /   
> fail-if(AndroidARM)   /   ???
> 

That's a can of worms. My vote is for using version for now and then if we start running tests on ARM against V17 of Android and getting unexpected passes, introduce a different way of distinguishing between the platforms at that point.
(In reply to Dan Minor [:dminor] from comment #4)
> That's a can of worms. My vote is for using version for now and then if we
> start running tests on ARM against V17 of Android and getting unexpected
> passes, introduce a different way of distinguishing between the platforms at
> that point.

If we can figure this out now (running the tests locally against a newer Android/ARM) we'll save ourselves a lot of headaches in the future. Making the annotations match the underlying cause instead of just our observations would be great.
> If we can figure this out now (running the tests locally against a newer
> Android/ARM) we'll save ourselves a lot of headaches in the future. Making
> the annotations match the underlying cause instead of just our observations
> would be great.

I agree in principle, but finding an underlying cause effectively means debugging each failure and trying to determine if it is Android version, ARM or x86, emulator or real hardware, powervr vs. tegra vs. desktop graphics, etc. and then adding flags for each of these.

We already have conditionals like this in the reftest manifests:
fuzzy-if(!contentSameGfxBackendAsCanvas,128,91) random-if(d2d) skip-if(azureSkiaGL) == 555388-1.html 555388-1-ref.html

Getting more specific isn't going to improve readability.
More worms for the can:

I tested some of the reftests from Comment 2 on devices, yielding these results:

                   Panda/ARM/4.0     RAZR/x86/4.1.2       emulator/x86/4.2      Nexus 4/ARM/4.3
650228-1.html           Fail              Pass                 Pass                  Fail
654950-1.html           Fail              Fail                 Pass                  Fail
718521.html             Fail              Fail                 Pass                  Fail
243519-4b.html          Fail              Fail                 Pass                  Fail
Comment on attachment 828923 [details] [diff] [review]
(1) disable robocop testBookmark, testReaderMode, testAddonManager on x86

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

> # disabled on x86 only; bug 936220 
Trailing whitespace, otherwise looks good.
Attachment #828923 - Flags: review?(dminor) → review+
testBookmark was recently disabled on all platforms, so dropped the x86-specific skip-if:

(1) https://hg.mozilla.org/integration/mozilla-inbound/rev/e80de1d36bb9
Attachment #828923 - Attachment description: disable robocop testBookmark, testReaderMode, testAddonManager on x86 → (1) disable robocop testBookmark, testReaderMode, testAddonManager on x86
This uses (Android&&AndroidVersion==17) as the basic condition to test for the new environment. The OS version may or may not be the actual factor responsible for the different pass/fail results -- I'm just trying to be pragmatic here.

In addition to the unexpected passes (Comment 2) there were a few failures, and one crash. The failures all look to be significant failures (not just off by a few pixels), so I have used fails-if rather than fuzzy-if. I used skip-if to get around the crash.

I have verified a clean run on the loaner and pushed to try to verify no harm done to existing platforms -- https://tbpl.mozilla.org/?tree=Try&rev=a17b6ab871a5.
Attachment #830869 - Flags: review?(dminor)
Skip 2 remaining mochitests that crash.
Attachment #831636 - Flags: review?(dminor)
Comment on attachment 830869 [details] [diff] [review]
(2) update reftest manifests for Android x86 emu platform

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

Looks good. I'll leave it to your discretion since I'm not sure of the nature of the failures, but most likely you should file bugs for the things that fail only on the emulator.

::: image/test/reftest/generic/reftest.list
@@ +1,1 @@
> +fails-if(Android&&AndroidVersion==17) HTTP == accept-image-catchall.html accept-image-catchall-ref.html

You should file a bug for this.

::: layout/reftests/backgrounds/reftest.list
@@ +127,5 @@
>  
>  random-if(B2G) == fixed-bg-with-transform-outside-viewport-1.html fixed-bg-with-transform-outside-viewport-ref.html
>  
>  HTTP == root-background-1.html root-background-ref.html
> +fails-if(Android&&AndroidVersion==17) HTTP != root-background-1.html about:blank

You should file a bug for this.

::: 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
> +skip-if(Android&&AndroidVersion==17) HTTP(..) != download-3-notref.html download-3.html

You should file a bug for this.

::: layout/reftests/text/reftest.list
@@ +105,5 @@
>  == white-space-2.html white-space-2-ref.html
>  == wordbreak-1.html wordbreak-1-ref.html
>  == wordbreak-2.html wordbreak-2-ref.html
>  == wordbreak-3.html wordbreak-3-ref.html
> +fails-if(Android&&AndroidVersion==17) == wordbreak-4a.html wordbreak-4a-ref.html

You should file a bug for this.
Attachment #830869 - Flags: review?(dminor) → review+
Comment on attachment 831636 [details] [diff] [review]
(3) update mochitest manifests for Android x86 emu platform

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

Looks good.
Attachment #831636 - Flags: review?(dminor) → review+
Most of the changes in the review patch are no longer necessary, due to bug 938316 (way to go, roc!). Here are the remainder (I will circle back and file bugs for these soon):

(2) https://hg.mozilla.org/integration/mozilla-inbound/rev/35d5387c2427
Depends on: 939823
Depends on: 941788
I commented on the existing bug.
Attachment #8336277 - Flags: review?(dminor)
Comment on attachment 8336277 [details] [diff] [review]
(4) update crashtest manifest for Android x86 emu platform

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

Looks good.
Attachment #8336277 - Flags: review?(dminor) → review+
Duplicate of this bug: 892688
testSystemPages is disabled on x86 -- bug 907383 no longer blocks.
No longer depends on: 907383
We can only be visible once we green things up.
Let's block on bug 917361.
Blocks: 917361
No longer blocks: 891959
Depends on: 944440
Depends on: 946908
We need to skip one troublesome test on Android x86 to avoid intermittent failures.
Attachment #8348494 - Flags: review?(jmaher)
Attachment #8348494 - Attachment description: skip xpcshell test_temporary_storage.js on android x86 → (5) skip xpcshell test_temporary_storage.js on android x86
Attachment #8348494 - Flags: review?(jmaher) → review+
Depends on: 949740
No longer depends on: 944440
I found a couple of tests had been added to android.json but not to androidx86.json. Also noticed that test_ipc had a different comment and position in androidx86.json, so updated that as well to make future diffs simpler.
Attachment #8358199 - Flags: review?(dminor)
Comment on attachment 8358199 [details] [diff] [review]
(6) re-sync androidx86.json with android.json

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

Looks good.
Attachment #8358199 - Flags: review?(dminor) → review+
Depends on: 957185
Depends on: 944440
Depends on: 962121
Attachment #8366869 - Flags: review?(dminor) → review+
(In reply to :Ms2ger from comment #34)
> skip-if() can't contain spaces:

Thanks, and sorry for breaking Cedar like that. The new patch is under review in bug 927602.
Depends on: 963317
Status update:

S1 and S2 fail intermittently due to bug 927602. Improvements to logging and diagnostics have failed to point to a cause. More improvements coming in bug 960265 and bug 963838. More experiments under way on the loaner, on a low priority basis. Logcat analysis of S2 might be fruitful.

S3 is green except for intermittent NSS crashes on shutdown, bug 963317, which also affects other Android and Windows jobs. Bug 941788 (crashes in web-gl tests) remains a concern, but frequency is now very low.

S4 is running trouble-free on trunk.
No longer depends on: 963317
Maintenance - disable 2 more tests to keep the jobs green on ash/cedar:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b737ef86e781
Depends on: 924622
Depends on: 983406
Maintenance - skip a few more tests giving us trouble on ash/cedar:

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb6d91d8472
Keywords: leave-open
Whiteboard: [leave open]
Status update:

S4 is running trouble-free on trunk.

S1, S2, and S3 fail intermittently due to bug 927602. Ideas for resolving:
 - try a new build of the emulator
 - run the emulator in gdb
 - contrast and compare S4 vs {S1,S2,S3} -- why is S4 okay?
I'm not finding time for this and x86 testing is not currently a high priority. Comment 43 still reflects current status.
Assignee: gbrown → nobody
No longer depends on: 957185
We're still only running xpcshell tests on Android x86.

I removed Android x86-specific annotations from all other test manifests in bug 1251013.
Assignee: nobody → gbrown
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.