906081 - Allow robocop and xpcshell tests to be skipped/failed depending on Android OS version

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Geoff Brown [:gbrown]]

This is a follow-up to https://bugzilla.mozilla.org/show_bug.cgi?id=902081#c4. In that bug, we have identified one test that fails for a well understood reason on Android 2.2 but passes on all newer versions of Android. Currently all we can do is skip that test on all Android platforms:

skip-if = os == "android"

For this case it would be convenient to have a method of failing or skipping a test based on OS version also.

Comment 1

[(not currently active) Ted Mielczarek]

This is tricky because at the moment we don't have any way to get this info. mozinfo.json is generated at build time, where the concept is sort of meaningless.

We could make the harness determine this at runtime (whether by querying the target device, or explicitly passed as a harness argument) and set a key in mozinfo as appropriate. We'd probably have to default it to None because I'm not sure the expression evaluator will handle non-existent keys in expressions sanely.

Comment 2

[Geoff Brown [:gbrown]]

See the patches on bug 897924 for another approach / work-around.

Comment 3

[Geoff Brown [:gbrown]]

Attachment: (1) add androidVersion to mozinfo in remote xpcshell tests

Comment 4

[Geoff Brown [:gbrown]]

Attachment: (2) add androidVersion to mozinfo in remote mochitests

The same capability would be useful in mochitest-robocop (robocop.ini).

Try run underway at https://tbpl.mozilla.org/?tree=Try&rev=7d79c05da6a8 to verify both xpcshell and robocop.

Comment 5

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

Comment on attachment 809461 [details] [diff] [review]
(1) add androidVersion to mozinfo in remote xpcshell tests

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

::: testing/xpcshell/remotexpcshelltests.py
@@ +544,5 @@
>          sys.exit(1)
>  
> +    # Add Android version (SDK level) to mozinfo so that manifest entries
> +    # can be conditional on androidVersion.
> +    androidVersion = dm.shellCheckOutput(['getprop', 'ro.build.version.sdk'])

what happens if this fails?  I would like some type of warning message printed out if it fails or the value looks invalid.

Comment 6

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

Comment on attachment 809463 [details] [diff] [review]
(2) add androidVersion to mozinfo in remote mochitests

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

::: testing/mochitest/runtestsremote.py
@@ +569,5 @@
>  
> +    # Add Android version (SDK level) to mozinfo so that manifest entries
> +    # can be conditional on androidVersion.
> +    androidVersion = dm.shellCheckOutput(['getprop', 'ro.build.version.sdk'])
> +    log.info("Android version: "+str(androidVersion))

maybe make this say "Android sdk version %s, will use this to filter manifests" % androidVersion

Comment 7

[Geoff Brown [:gbrown]]

Attachment: WIP (3) teach robocop to skip disabled tests

Comment 8

[Geoff Brown [:gbrown]]

Attachment: (3) teach robocop to skip disabled tests

Comment 9

[Geoff Brown [:gbrown]]

Comment on attachment 809850 [details] [diff] [review]
(3) teach robocop to skip disabled tests

https://tbpl.mozilla.org/?tree=Try&rev=09618ca2cdaf shows this in action.

Comment 10

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

Comment on attachment 809850 [details] [diff] [review]
(3) teach robocop to skip disabled tests

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

that was easy teachings.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 809461 [details] [diff] [review]
(1) add androidVersion to mozinfo in remote xpcshell tests

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

::: testing/xpcshell/remotexpcshelltests.py
@@ +545,5 @@
>  
> +    # Add Android version (SDK level) to mozinfo so that manifest entries
> +    # can be conditional on androidVersion.
> +    androidVersion = dm.shellCheckOutput(['getprop', 'ro.build.version.sdk'])
> +    mozinfo.info['androidVersion'] = androidVersion

nit: I think most of the other mozinfo keys we put in use underscores, not camelCase, so maybe android_version?

We don't currently have support for greater-than or less-than comparisons, is that going to be something we need in the future, or will string equality suffice for now?

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 809463 [details] [diff] [review]
(2) add androidVersion to mozinfo in remote mochitests

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

If we're going to want this in every harness maybe we can move this into mozinfo itself?

Comment 13

[Geoff Brown [:gbrown]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #11)
> Comment on attachment 809461 [details] [diff] [review]
> > +    # Add Android version (SDK level) to mozinfo so that manifest entries
> > +    # can be conditional on androidVersion.
> > +    androidVersion = dm.shellCheckOutput(['getprop', 'ro.build.version.sdk'])
> > +    mozinfo.info['androidVersion'] = androidVersion
> 
> nit: I think most of the other mozinfo keys we put in use underscores, not
> camelCase, so maybe android_version?

Yes, absolutely. I thought there were no existing multi-word keys, but now I see for example, 'service_pack'.

> We don't currently have support for greater-than or less-than comparisons,
> is that going to be something we need in the future, or will string equality
> suffice for now?

String equality will suffice for now, but greater-than/less-than would be useful.

Comment 14

[Geoff Brown [:gbrown]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)
> Comment on attachment 809463 [details] [diff] [review]
> (2) add androidVersion to mozinfo in remote mochitests
> 
> Review of attachment 809463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we're going to want this in every harness maybe we can move this into
> mozinfo itself?

But then mozinfo would need to know about devicemanager -- that doesn't seem like a good thing.

Comment 15

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #5)
> Comment on attachment 809461 [details] [diff] [review]
> > +    androidVersion = dm.shellCheckOutput(['getprop', 'ro.build.version.sdk'])
> 
> what happens if this fails?  I would like some type of warning message
> printed out if it fails or the value looks invalid.

If getprop is not found or shellCheckOutput otherwise has a problem executing the command, a DMError is thrown and reported and the test ends immediately.

If getprop succeeds but ro.build.version.sdk does not exist, then we report:

   Mochi-Remote INFO | Android version ''; will be used to filter manifests

and continue. I think that might happen on some developer devices (it does not happen on tegras, pandas, the x86 emulator or any of my devices); I think developers would prefer that this be non-fatal, so that tests can still be run even if this property is missing or non-standard.

Comment 16

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98c568a0e6e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/61f225dd6932
https://hg.mozilla.org/integration/mozilla-inbound/rev/04c327ab2215

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98c568a0e6e7
https://hg.mozilla.org/mozilla-central/rev/61f225dd6932
https://hg.mozilla.org/mozilla-central/rev/04c327ab2215