[Android] Support conditional test disabling based on android.os.Build values.

[Android] Support conditional test disabling based on android.os.Build values.

BUG=565324
TBR=thakis@chromium.org

Committed: https://crrev.com/ae13c65b3c776429bcc48968855112607db595a0
Cr-Commit-Position: refs/heads/master@{#366465}

[jbudorick, 2015-12-10 15:23:15]

jbudorick@chromium.org changed reviewers:
+ yfriedman@chromium.org

[jbudorick, 2015-12-10 15:23:15]

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
(right):

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
@DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
crbug.com/540723
There are, unsurprisingly, other tests where this can be used, but I just wanted
to include one as a sample.

I'm a bit concerned here about how this could facilitate accidental leaks, but
I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT check of some
kind?

[Yaron, 2015-12-11 02:22:16]

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
File
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:150:
return Build.VERSION.SDK_INT < v.sdk_is_less_than()
*sigh*

I guess I'll eventually lose this battle of only allowing restricting tests for
SDK>=X. IMHO we shouldn't have tests that stop working at some API level cause
we don't have features implemented that way :(

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:159:
Log.i(TAG, "  %s", s);
?

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:162:
v.supported_abis_includes());
would you need to strip this on spaces or can you only specify one?

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
(right):

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
@DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
crbug.com/540723
On 2015/12/10 15:23:15, jbudorick wrote:
> There are, unsurprisingly, other tests where this can be used, but I just
wanted
> to include one as a sample.
> 
:(
Should we include a message= attribute which can link to the bug (so you can see
in the test runner that it's skipping some tsets?)

> I'm a bit concerned here about how this could facilitate accidental leaks, but
> I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT check of some
> kind?

Ya, I could definitely see that happen. You could have a PRESUBMIT of
whitelisted devices based on what we run on waterfalls. Of course that can still
fail if it's on the internal waterfall. Another option is to wait to introduce
this until more of the tests are moved up (I know, not the greatest)

[jbudorick, 2015-12-17 02:08:13]

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
File
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:150:
return Build.VERSION.SDK_INT < v.sdk_is_less_than()
On 2015/12/11 02:22:16, Yaron wrote:
> *sigh*
> 
> I guess I'll eventually lose this battle of only allowing restricting tests
for
> SDK>=X. IMHO we shouldn't have tests that stop working at some API level cause
> we don't have features implemented that way :(

I'm fine with leaving this out until someone needs it.

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:159:
Log.i(TAG, "  %s", s);
On 2015/12/11 02:22:16, Yaron wrote:
> ?

leftover, removed.

https://codereview.chromium.org/1519523002/diff/1/base/test/android/javatests...
base/test/android/javatests/src/org/chromium/base/test/BaseInstrumentationTestRunner.java:162:
v.supported_abis_includes());
On 2015/12/11 02:22:16, Yaron wrote:
> would you need to strip this on spaces or can you only specify one?

at the moment, only one.

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
(right):

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
@DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
crbug.com/540723
On 2015/12/11 02:22:16, Yaron wrote:
> On 2015/12/10 15:23:15, jbudorick wrote:
> > There are, unsurprisingly, other tests where this can be used, but I just
> wanted
> > to include one as a sample.
> > 
> :(
> Should we include a message= attribute which can link to the bug (so you can
see
> in the test runner that it's skipping some tsets?)

I like this idea, but I'll address how the test runner handles this annotation
in general (and message= specifically) in a separate CL if at all.

> 
> > I'm a bit concerned here about how this could facilitate accidental leaks,
but
> > I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT check of
some
> > kind?
> 
> Ya, I could definitely see that happen. You could have a PRESUBMIT of
> whitelisted devices based on what we run on waterfalls. Of course that can
still
> fail if it's on the internal waterfall. Another option is to wait to introduce
> this until more of the tests are moved up (I know, not the greatest)

I'm less worried about the tests and more worried about the possibility of
wanting to disable an upstream test on prerelease hardware.

[Yaron, 2015-12-17 15:12:37]

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
(right):

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
@DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
crbug.com/540723
On 2015/12/17 02:08:13, jbudorick wrote:
> On 2015/12/11 02:22:16, Yaron wrote:
> > On 2015/12/10 15:23:15, jbudorick wrote:
> > > There are, unsurprisingly, other tests where this can be used, but I just
> > wanted
> > > to include one as a sample.
> > > 
> > :(
> > Should we include a message= attribute which can link to the bug (so you can
> see
> > in the test runner that it's skipping some tsets?)
> 
> I like this idea, but I'll address how the test runner handles this annotation
> in general (and message= specifically) in a separate CL if at all.
> 
> > 
> > > I'm a bit concerned here about how this could facilitate accidental leaks,
> but
> > > I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT check of
> some
> > > kind?
> > 
> > Ya, I could definitely see that happen. You could have a PRESUBMIT of
> > whitelisted devices based on what we run on waterfalls. Of course that can
> still
> > fail if it's on the internal waterfall. Another option is to wait to
introduce
> > this until more of the tests are moved up (I know, not the greatest)
> 
> I'm less worried about the tests and more worried about the possibility of
> wanting to disable an upstream test on prerelease hardware.

Right. So what options do we provide for sheriffs now? The SDK check <= (which I
complained about) seemed likes the easiest solution. I think in general it's
expected that we're testing with unreleased software and there's always a new
android coming - but hardware codenames are more sensitive. I guess I take it
back - feel free to add back sdk_is_less_than and we'll see to what extent it
gets (ab)used. It seems like the easiest way to support this restriction and
minimize leaks.

Relatedly - is there a significant difference between DisableIf and
Restrictions? DisableIf means it's broken and Restriction means it's never
intended to work?

[jbudorick, 2015-12-18 17:43:06]

Shuffled some things around s.t. I could unit test the new skip check.

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
File chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
(right):

https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
@DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
crbug.com/540723
On 2015/12/17 15:12:37, Yaron wrote:
> On 2015/12/17 02:08:13, jbudorick wrote:
> > On 2015/12/11 02:22:16, Yaron wrote:
> > > On 2015/12/10 15:23:15, jbudorick wrote:
> > > > There are, unsurprisingly, other tests where this can be used, but I
just
> > > wanted
> > > > to include one as a sample.
> > > > 
> > > :(
> > > Should we include a message= attribute which can link to the bug (so you
can
> > see
> > > in the test runner that it's skipping some tsets?)
> > 
> > I like this idea, but I'll address how the test runner handles this
annotation
> > in general (and message= specifically) in a separate CL if at all.
> > 
> > > 
> > > > I'm a bit concerned here about how this could facilitate accidental
leaks,
> > but
> > > > I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT check of
> > some
> > > > kind?
> > > 
> > > Ya, I could definitely see that happen. You could have a PRESUBMIT of
> > > whitelisted devices based on what we run on waterfalls. Of course that can
> > still
> > > fail if it's on the internal waterfall. Another option is to wait to
> introduce
> > > this until more of the tests are moved up (I know, not the greatest)
> > 
> > I'm less worried about the tests and more worried about the possibility of
> > wanting to disable an upstream test on prerelease hardware.
> 
> Right. So what options do we provide for sheriffs now? The SDK check <= (which
I
> complained about) seemed likes the easiest solution. I think in general it's
> expected that we're testing with unreleased software and there's always a new
> android coming - but hardware codenames are more sensitive. I guess I take it
> back - feel free to add back sdk_is_less_than and we'll see to what extent it
> gets (ab)used. It seems like the easiest way to support this restriction and
> minimize leaks.
> 

Hm, ok.

> Relatedly - is there a significant difference between DisableIf and
> Restrictions? DisableIf means it's broken and Restriction means it's never
> intended to work?
> 

That's what I had in mind, but perhaps that's a more nuanced split than we want.

[Yaron, 2015-12-21 18:13:20]

On 2015/12/18 17:43:06, jbudorick wrote:
> Shuffled some things around s.t. I could unit test the new skip check.
> 
>
https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
> File
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
> (right):
> 
>
https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
>
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
> @DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
> crbug.com/540723
> On 2015/12/17 15:12:37, Yaron wrote:
> > On 2015/12/17 02:08:13, jbudorick wrote:
> > > On 2015/12/11 02:22:16, Yaron wrote:
> > > > On 2015/12/10 15:23:15, jbudorick wrote:
> > > > > There are, unsurprisingly, other tests where this can be used, but I
> just
> > > > wanted
> > > > > to include one as a sample.
> > > > > 
> > > > :(
> > > > Should we include a message= attribute which can link to the bug (so you
> can
> > > see
> > > > in the test runner that it's skipping some tsets?)
> > > 
> > > I like this idea, but I'll address how the test runner handles this
> annotation
> > > in general (and message= specifically) in a separate CL if at all.
> > > 
> > > > 
> > > > > I'm a bit concerned here about how this could facilitate accidental
> leaks,
> > > but
> > > > > I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT check
of
> > > some
> > > > > kind?
> > > > 
> > > > Ya, I could definitely see that happen. You could have a PRESUBMIT of
> > > > whitelisted devices based on what we run on waterfalls. Of course that
can
> > > still
> > > > fail if it's on the internal waterfall. Another option is to wait to
> > introduce
> > > > this until more of the tests are moved up (I know, not the greatest)
> > > 
> > > I'm less worried about the tests and more worried about the possibility of
> > > wanting to disable an upstream test on prerelease hardware.
> > 
> > Right. So what options do we provide for sheriffs now? The SDK check <=
(which
> I
> > complained about) seemed likes the easiest solution. I think in general it's
> > expected that we're testing with unreleased software and there's always a
new
> > android coming - but hardware codenames are more sensitive. I guess I take
it
> > back - feel free to add back sdk_is_less_than and we'll see to what extent
it
> > gets (ab)used. It seems like the easiest way to support this restriction and
> > minimize leaks.
> > 
> 
> Hm, ok.
> 
> > Relatedly - is there a significant difference between DisableIf and
> > Restrictions? DisableIf means it's broken and Restriction means it's never
> > intended to work?
> > 
> 
> That's what I had in mind, but perhaps that's a more nuanced split than we
want.

Ok then please add a comment to this effect in the annotation's class-level
comment?
lgtm

[jbudorick, 2015-12-21 19:54:56]

Description was changed from

==========
[Android] Support conditional test disabling based on android.os.Build values.

BUG=565324
==========

to

==========
[Android] Support conditional test disabling based on android.os.Build values.

BUG=565324
TBR=thakis@chromium.org
==========

[jbudorick, 2015-12-21 19:57:54]

jbudorick@chromium.org changed reviewers:
+ thakis@chromium.org

[jbudorick, 2015-12-21 19:57:55]

TBR thakis for trivial changes to base/{base.gyp,BUILD.gn}

[jbudorick, 2015-12-21 19:58:15]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-12-21 19:58:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1519523002/#ps80001
(title: "yfriedman comment")

[commit-bot: I haz the power, 2015-12-21 19:58:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1519523002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1519523002/80001

[jbudorick, 2015-12-21 19:58:30]

On 2015/12/21 18:13:20, Yaron wrote:
> On 2015/12/18 17:43:06, jbudorick wrote:
> > Shuffled some things around s.t. I could unit test the new skip check.
> > 
> >
>
https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
> > File
> chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1519523002/diff/1/chrome/android/javatests/sr...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/NavigateTest.java:358:
> > @DisableIf.Build(hardware_is = "sprout") // Fails on android-one:
> > crbug.com/540723
> > On 2015/12/17 15:12:37, Yaron wrote:
> > > On 2015/12/17 02:08:13, jbudorick wrote:
> > > > On 2015/12/11 02:22:16, Yaron wrote:
> > > > > On 2015/12/10 15:23:15, jbudorick wrote:
> > > > > > There are, unsurprisingly, other tests where this can be used, but I
> > just
> > > > > wanted
> > > > > > to include one as a sample.
> > > > > > 
> > > > > :(
> > > > > Should we include a message= attribute which can link to the bug (so
you
> > can
> > > > see
> > > > > in the test runner that it's skipping some tsets?)
> > > > 
> > > > I like this idea, but I'll address how the test runner handles this
> > annotation
> > > > in general (and message= specifically) in a separate CL if at all.
> > > > 
> > > > > 
> > > > > > I'm a bit concerned here about how this could facilitate accidental
> > leaks,
> > > > but
> > > > > > I'm not sure how to resolve or mitigate that. Maybe a PRESUBMIT
check
> of
> > > > some
> > > > > > kind?
> > > > > 
> > > > > Ya, I could definitely see that happen. You could have a PRESUBMIT of
> > > > > whitelisted devices based on what we run on waterfalls. Of course that
> can
> > > > still
> > > > > fail if it's on the internal waterfall. Another option is to wait to
> > > introduce
> > > > > this until more of the tests are moved up (I know, not the greatest)
> > > > 
> > > > I'm less worried about the tests and more worried about the possibility
of
> > > > wanting to disable an upstream test on prerelease hardware.
> > > 
> > > Right. So what options do we provide for sheriffs now? The SDK check <=
> (which
> > I
> > > complained about) seemed likes the easiest solution. I think in general
it's
> > > expected that we're testing with unreleased software and there's always a
> new
> > > android coming - but hardware codenames are more sensitive. I guess I take
> it
> > > back - feel free to add back sdk_is_less_than and we'll see to what extent
> it
> > > gets (ab)used. It seems like the easiest way to support this restriction
and
> > > minimize leaks.
> > > 
> > 
> > Hm, ok.
> > 
> > > Relatedly - is there a significant difference between DisableIf and
> > > Restrictions? DisableIf means it's broken and Restriction means it's never
> > > intended to work?
> > > 
> > 
> > That's what I had in mind, but perhaps that's a more nuanced split than we
> want.
> 
> Ok then please add a comment to this effect in the annotation's class-level
> comment?
> lgtm

done

[commit-bot: I haz the power, 2015-12-21 20:14:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-21 20:14:49]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[jbudorick, 2015-12-21 20:41:23]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-12-21 20:41:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1519523002/#ps100001
(title: "fix one outdated use of SkipCheck")

[commit-bot: I haz the power, 2015-12-21 20:41:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1519523002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1519523002/100001

[commit-bot: I haz the power, 2015-12-21 21:54:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-21 21:55:13]

Description was changed from

==========
[Android] Support conditional test disabling based on android.os.Build values.

BUG=565324
TBR=thakis@chromium.org
==========

to

==========
[Android] Support conditional test disabling based on android.os.Build values.

BUG=565324
TBR=thakis@chromium.org

Committed: https://crrev.com/ae13c65b3c776429bcc48968855112607db595a0
Cr-Commit-Position: refs/heads/master@{#366465}
==========

[commit-bot: I haz the power, 2015-12-21 21:55:14]

Patchset 6 (id:??) landed as
https://crrev.com/ae13c65b3c776429bcc48968855112607db595a0
Cr-Commit-Position: refs/heads/master@{#366465}