877266 - Add reftests sandbox flag to distinguish between Android 2.x and Android 4.x and update manifests

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Dan Minor [:dminor]]

Reftests are currently disabled on the pandas due to getting different results on the tegras and the pandas and not being able to distinguish between platforms in the reftests manifests.

Adding a sandbox flag will allow the manifests to be updated so that reftests can be enabled on pandas.

Comment 1

[Dan Minor [:dminor]]

This is how I plan to handle getting the Android version (sans printf) in reftest.jsm, BuildConditionSandbox:

544,550d541
< 
<     if (sandbox.Android) {
<         var sysInfo = CC["@mozilla.org/system-info;1"].getService(CI.nsIPropertyBag2);
<         sandbox.AndroidVersion = sysInfo.getPropertyAsInt32("version");
<         gDumpLog("version: " + sandbox.AndroidVersion.toString() + "\n");
<     }
<

Comment 2

[Dan Minor [:dminor]]

Any thoughts on whether the approach should be to store version number for use in the manifest, or use the version number to set a flag for test device type?

e.g. sandbox.AndroidVersion == 9 or
     sandbox.Android22 or
     sandbox.tegra

Comment 3

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

I like the sandbox.AndroidVersion == x, but I also like to keep our stuff as precise as possible.

How about:
sandbox.Android covers all android, then sandbox.AndroidVersion to tackle the corner cases.  We could have 2.2, 4.0.4.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> How about:
> sandbox.Android covers all android, then sandbox.AndroidVersion to tackle
> the corner cases.  We could have 2.2, 4.0.4.

By which you mean changing sandbox.Android from being true/false to being <integer>/false?  That seems reasonable.  (Rather like the current sandbox.OSX.)  I think that's probably good, though I'd hope we don't even need sandbox.AndroidVersion; it seems unlikely to lead to differences.)

(I worry we're writing the conditions so they happen to match our testing machines, rather than conditioning the test failures on the actual conditions that trigger the failures.  It would be nice if we could write the correct conditions at least when we know what they are, which would help when bringing up new platforms.  I worry that having AndroidVersion is likely to lead to more of that rather than being the correct condition.)

Comment 5

[Dan Minor [:dminor]]

Thanks for the comments.

I'm going to run through the reftests on my panda a few times and see exactly where it differs from the current tegra results. With any luck I'll find some conditions that we can use other than just Android version.

Comment 6

[Dan Minor [:dminor]]

As an update, there are a few failures that appear to be actual bugs, and also occur on Android X86. I will be filing bugs for these separately, soon.

There are a few unexpected passes.

It appears the remaining failures are due to small pixel differences in rendering, e.g. nearly black (4, 0, 8) instead of fully black (0, 0, 0), and can be handled by fuzzy-if. I'll also file a bug to track this.

Comment 7

[Dan Minor [:dminor]]

The vast majority of the unexpected results are common between the pandaboard and my Samsung S3. Since the devices have different CPUs and GPUs, I'd like to mark these as fuzzy-if based upon Android version.

For the few remaining bugs that appear to be panda only, I would like to add an Android.Pandaboard flag (I can pull the device type out of system info as well.) The majority of these are clear failures due to bugs (e.g. text incorrectly rendered) which would hopefully be fixed in the future.

Comment 8

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

this sounds logical.  The panda only bugs might be something specific to hardware or maybe something we need to install/configure on the system.  I know in the past we had to install fonts for maemo, maybe there is something similar we need to do (or even a preference to set).

Comment 9

[Dan Minor [:dminor]]

Attachment: Patch to add AndroidVersion and modify manifests.

Green try run for the patch:
https://tbpl.mozilla.org/?tree=Try&rev=b3803aec505e

In a few cases, I adjusted an existing fuzzy-if for Android rather than create a new one specifically for Android 4, if I felt that the difference in ranges wasn't too extreme.

Comment 10

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

Comment on attachment 760835 [details] [diff] [review]
Patch to add AndroidVersion and modify manifests.

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

a few small details below.  Overall this is much simpler and cleaner than a massive patch.  Personally I don't like the way the syntax looks and reads:
android&&androidversion>=15

if we are checking androidversion, can we assume that android exists?  Maybe something like:
android.version>=15

that would not match if android is not defined or if it is defined and the version < 15.  Not sure how to do that or if I am getting too picky.  :dbaron, what are your thoughts on it?

I could be convinced of an r+ assuming we all agree on the syntax.

::: layout/reftests/svg/smil/transform/reftest.list
@@ +8,5 @@
>  fuzzy(32,216) == rotate-angle-2.svg rotate-angle-ref.svg # bug 839865
>  fuzzy(32,216) == rotate-angle-3.svg rotate-angle-ref.svg # bug 839865
>  fuzzy(32,192) == rotate-angle-4.svg rotate-angle-ref.svg # bug 839865
>  fuzzy(32,120) == rotate-angle-5.svg rotate-angle-ref.svg # bug 839865
> +fuzzy(32,6237) == scale-1.svg scale-1-ref.svg            # bug 839865

why is this changed?  I assume we needed to change it for android?

::: layout/tools/reftest/reftest.js
@@ +581,5 @@
> +    if (sandbox.Android) {
> +        var sysInfo = CC["@mozilla.org/system-info;1"].getService(CI.nsIPropertyBag2);
> +
> +        sandbox.AndroidVersion = sysInfo.getPropertyAsInt32("version");
> +    }

can you document in a comment here what some of the versions are?

Comment 11

[Dan Minor [:dminor]]

Thanks for the review.

As mentioned in IRC, I don't have strong feelings on the syntax. Either your option of adding .version to Android, or dbarron's suggestion of having Android be defined as false or a number corresponding to the version work for me, although to be honest, I would prefer to keep the type of Android consistent, which was my rationale for adding AndroidVersion.

Thanks for the catch on fuzzy(32,6237), I had meant to back that change out when I added the skip for smil/transform.

I'll add comments as requested.

Comment 12

[Dan Minor [:dminor]]

dbaron, do you have any comments on the syntax for android version? If not, I'll go with jmaher's suggestion. Thanks.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 760835 [details] [diff] [review]
Patch to add AndroidVersion and modify manifests.

I'm fine with the syntax of AndroidVersion (modulo jmaher's other comments).

Comment 14

[Dan Minor [:dminor]]

Attachment: Patch to add AndroidVersion and modify manifests.

This addresses earlier feedback and brings the patch up to date to handle new failures that have shown up in the past week. I've kept the AndroidVersion syntax.

Comment 15

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

Comment on attachment 765843 [details] [diff] [review]
Patch to add AndroidVersion and modify manifests.

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

lets make sure we have at least 90% pass rate before filing a bug to turn these on, from the sounds of it we are already there.

Comment 16

[Dan Minor [:dminor]]

Attachment: Patch to add AndroidVersion and modify manifests.

This patch fixes a few unexpected passes from the last version. Try run here:
https://tbpl.mozilla.org/?tree=Try&rev=64f8680c2e2a

Comment 17

[Dan Minor [:dminor]]

Mostly green try run for the other reftest suites: https://tbpl.mozilla.org/?tree=Try&rev=65a7d13b24d3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef53f8a79327

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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