Add a testing restriction for whether Google Play Services is up-to-date.

Add a testing restriction for whether Google Play Services is up-to-date.

Rather than having these tests being forced into the flaky bucket,
condition their execution on a new restriction. It has the nice
side-effect that one bad bot or configuration doesn't cause the test to
get disabled and hopefully these will be running *somewhere*.

Tested locally that the tests passed, then removed all updates to google
play services and they were ignored.
BUG=514449
Committed: https://crrev.com/fffffd3dbb28c8eb8e25faa14726c657717c636a
Cr-Commit-Position: refs/heads/master@{#371799}

[Yaron, 2016-01-12 22:35:48]

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

[Yaron, 2016-01-12 22:35:48]

I originally thought to add this to DisableIf but settled on a Restriction.
There's no value I would specify in DisableIf, as the "is play services
sufficiently up to date" question is configured in the xml for the app, not by a
given feature.

[jbudorick, 2016-01-13 14:43:50]

https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java
(right):

https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java:21:
/** Specifies the test is only valid on phone form factors. Only valid in
chrome/ layer */
1) Why are these two only valid in the chrome/ layer? (play services is less
surprising)
2) If they're chrome/-specific, why are we handling them in base/?

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/BUI...
File chrome/test/android/BUILD.gn (right):

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/BUI...
chrome/test/android/BUILD.gn:33: google_play_services_library,
... does this work? I don't think I've seen something like this before.

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:238:
.isGooglePlayServicesAvailable(getTargetContext()))) {
It's not clear to me from the docs
(https://developers.google.com/android/reference/com/google/android/gms/common...)
that this does what we want it to do.

Also, it sounds suspiciously like there's a network dependency somewhere in
there.

[Yaron, 2016-01-13 15:54:30]

https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java
(right):

https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java:21:
/** Specifies the test is only valid on phone form factors. Only valid in
chrome/ layer */
On 2016/01/13 14:43:50, jbudorick wrote:
> 1) Why are these two only valid in the chrome/ layer? (play services is less
> surprising)
> 2) If they're chrome/-specific, why are we handling them in base/?

They're only processed by ChromeRestrictionSkipCheck. TBH, I was surprised that
they were in base/. I guess I move this out to a separate file

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/BUI...
File chrome/test/android/BUILD.gn (right):

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/BUI...
chrome/test/android/BUILD.gn:33: google_play_services_library,
On 2016/01/13 14:43:50, jbudorick wrote:
> ... does this work? I don't think I've seen something like this before.

it does. It's how we selectively swap the library in gn between
external/internal

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java
(right):

https://codereview.chromium.org/1582513003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:238:
.isGooglePlayServicesAvailable(getTargetContext()))) {
On 2016/01/13 14:43:50, jbudorick wrote:
> It's not clear to me from the docs
>
(https://developers.google.com/android/reference/com/google/android/gms/common...)
> that this does what we want it to do.
> 
> Also, it sounds suspiciously like there's a network dependency somewhere in
> there.

Hmm, I didn't rule out the possibility that underneath it does network but in
general it's not. What this does is tries to bind to the play services app and
check the version number. The version number that's required is specified in
./third_party/android_tools/sdk/extras/google/google_play_services/libproject/google-play-services_lib/res/values/version.xml
We use it in other places to check whether we can connect to play services:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

[jbudorick, 2016-01-13 16:54:50]

Any thoughts on the try job failure? The patch skipped those three tests
correctly for me locally. I'm trying to figure out why that didn't happen on the
bot.

https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java
(right):

https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java:21:
/** Specifies the test is only valid on phone form factors. Only valid in
chrome/ layer */
On 2016/01/13 15:54:30, Yaron wrote:
> On 2016/01/13 14:43:50, jbudorick wrote:
> > 1) Why are these two only valid in the chrome/ layer? (play services is less
> > surprising)
> > 2) If they're chrome/-specific, why are we handling them in base/?
> 
> They're only processed by ChromeRestrictionSkipCheck. TBH, I was surprised
that
> they were in base/. I guess I move this out to a separate file

sgtm

[Yaron, 2016-01-13 19:11:22]

On 2016/01/13 16:54:50, jbudorick wrote:
> Any thoughts on the try job failure? The patch skipped those three tests
> correctly for me locally. I'm trying to figure out why that didn't happen on
the
> bot.

Hmm. Not sure yet but must be a bug in the bot scripts or some incorrect caching

> 
>
https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
> File
> base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java
> (right):
> 
>
https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
>
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java:21:
> /** Specifies the test is only valid on phone form factors. Only valid in
> chrome/ layer */
> On 2016/01/13 15:54:30, Yaron wrote:
> > On 2016/01/13 14:43:50, jbudorick wrote:
> > > 1) Why are these two only valid in the chrome/ layer? (play services is
less
> > > surprising)
> > > 2) If they're chrome/-specific, why are we handling them in base/?
> > 
> > They're only processed by ChromeRestrictionSkipCheck. TBH, I was surprised
> that
> > they were in base/. I guess I move this out to a separate file
> 
> sgtm

[Yaron, 2016-01-14 22:52:08]

Patchset #8 (id:140001) has been deleted

[Yaron, 2016-01-18 20:29:04]

On 2016/01/13 19:11:22, Yaron wrote:
> On 2016/01/13 16:54:50, jbudorick wrote:
> > Any thoughts on the try job failure? The patch skipped those three tests
> > correctly for me locally. I'm trying to figure out why that didn't happen on
> the
> > bot.
> 
> Hmm. Not sure yet but must be a bug in the bot scripts or some incorrect
caching
> 
> > 
> >
>
https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
> > File
> > base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1582513003/diff/20001/base/test/android/javat...
> >
>
base/test/android/javatests/src/org/chromium/base/test/util/Restriction.java:21:
> > /** Specifies the test is only valid on phone form factors. Only valid in
> > chrome/ layer */
> > On 2016/01/13 15:54:30, Yaron wrote:
> > > On 2016/01/13 14:43:50, jbudorick wrote:
> > > > 1) Why are these two only valid in the chrome/ layer? (play services is
> less
> > > > surprising)
> > > > 2) If they're chrome/-specific, why are we handling them in base/?
> > > 
> > > They're only processed by ChromeRestrictionSkipCheck. TBH, I was surprised
> > that
> > > they were in base/. I guess I move this out to a separate file
> > 
> > sgtm

ok, I created ChromeRestrictions.java which introduced a bit of churn, lmk what
you think of that (I also have a downstream CL for changes there)
As for linux_rel_ng it has something to do with proguard and which I don't get.
I verified locally that GoogleApiAvailability was getting renamed inside
chrome's classes.dex (by looking at the proguard.mapping file). I also see that
mapping file passed into chrome tests's obfuscation step, but for some reason
it's not applied. We do pass -dontobfuscate as part of that, but I would hope
that it would still apply mappings from dependents but it doesn't seem to be the
case. I think we typically make this work by adding @VisibleForTesting but since
the code is actually from play services, I whitelisted inside proguard.flags

[Yaron, 2016-01-25 15:04:32]

friendly bump - I'll likely need to rebase but lmk if you're ok with this
approach

[jbudorick, 2016-01-25 15:21:37]

Sorry about the delay here.

https://codereview.chromium.org/1582513003/diff/240001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java
(right):

https://codereview.chromium.org/1582513003/diff/240001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java:79:
* @ChromeRestriction(RESTRICTION_TYPE_PHONE)
nit: ChromeRestriction.RESTRICTION_TYPE_PHONE

https://codereview.chromium.org/1582513003/diff/240001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeRestriction.java
(right):

https://codereview.chromium.org/1582513003/diff/240001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeRestriction.java:20:
public @interface ChromeRestriction {
Why is this its own annotation instead of just being a bunch of static final
Strings that get processed within the existing @Restriction annotation by
ChromeRestrictionSkipCheck?

i.e. this:

  @Restriction({Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE,
                ChromeRestriction.RESTRICTION_TYPE_PHONE})
  public void testNoStackAnimation() throws Exception {
      ...

seems less clunky to me than this:

  @ChromeRestriction({ChromeRestriction.RESTRICTION_TYPE_PHONE})
  @Restriction({Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE})
  public void testNoStackAnimation() throws Exception {
      ...

[Yaron, 2016-01-25 21:39:55]

https://codereview.chromium.org/1582513003/diff/240001/chrome/test/android/ja...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeRestriction.java
(right):

https://codereview.chromium.org/1582513003/diff/240001/chrome/test/android/ja...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/ChromeRestriction.java:20:
public @interface ChromeRestriction {
On 2016/01/25 15:21:37, jbudorick wrote:
> Why is this its own annotation instead of just being a bunch of static final
> Strings that get processed within the existing @Restriction annotation by
> ChromeRestrictionSkipCheck?
> 
> i.e. this:
> 
>   @Restriction({Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE,
>                 ChromeRestriction.RESTRICTION_TYPE_PHONE})
>   public void testNoStackAnimation() throws Exception {
>       ...
> 
> seems less clunky to me than this:
> 
>   @ChromeRestriction({ChromeRestriction.RESTRICTION_TYPE_PHONE})
>   @Restriction({Restriction.RESTRICTION_TYPE_NON_LOW_END_DEVICE})
>   public void testNoStackAnimation() throws Exception {
>       ...

Hmm. I guess I got confused and thought the annotation took the enum and not
just any string. Much better, thanks.

https://codereview.chromium.org/1582513003/diff/280001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/translate/TranslateInfoBarTest.java
(right):

https://codereview.chromium.org/1582513003/diff/280001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/translate/TranslateInfoBarTest.java:102:
@DisabledTest
This test appears to be legitimately broken now. Will deal with it separately.

[jbudorick, 2016-01-25 21:57:50]

lgtm w/ question

not sure what's up with the analyze failures in your try jobs...

https://codereview.chromium.org/1582513003/diff/280001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1582513003/diff/280001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:3182:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib'
What's up with the changes in this file other than this one?

[Yaron, 2016-01-25 22:04:02]

thanks for the review! I'll fix that file when I land tomorrow

https://codereview.chromium.org/1582513003/diff/280001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/1582513003/diff/280001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:3182:
'../third_party/android_tools/android_tools.gyp:google_play_services_javalib'
On 2016/01/25 21:57:49, jbudorick wrote:
> What's up with the changes in this file other than this one?

bad switch b/w branches on my part. Perhaps this is also what's making analyze
sad.

[Yaron, 2016-01-27 15:08:40]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2016-01-27 15:08:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1582513003/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-27 15:08:52]

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

[commit-bot: I haz the power, 2016-01-27 16:25:32]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2016-01-27 16:26:45]

Description was changed from

==========
Add a testing restriction for whether Google Play Services is up-to-date.

Rather than having these tests being forced into the flaky bucket,
condition their execution on a new restriction. It has the nice
side-effect that one bad bot or configuration doesn't cause the test to
get disabled and hopefully these will be running *somewhere*.

Tested locally that the tests passed, then removed all updates to google
play services and they were ignored.
BUG=514449
==========

to

==========
Add a testing restriction for whether Google Play Services is up-to-date.

Rather than having these tests being forced into the flaky bucket,
condition their execution on a new restriction. It has the nice
side-effect that one bad bot or configuration doesn't cause the test to
get disabled and hopefully these will be running *somewhere*.

Tested locally that the tests passed, then removed all updates to google
play services and they were ignored.
BUG=514449

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

[commit-bot: I haz the power, 2016-01-27 16:26:46]

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