[Android] Update all Robolectric tests to Robolectric 3.0

Update all Robolectric tests to Robolectric 3.0

BUG=522043
Committed: https://crrev.com/de621335b3d5b104587934b51cf83eb17911b14a
Cr-Commit-Position: refs/heads/master@{#411362}

[mikecase (-- gone --), 2016-07-31 00:52:48]

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

[mikecase (-- gone --), 2016-07-31 00:52:50]

All tests are passing!!!!! Some remaining cleanup, but its 99% there.

[mikecase (-- gone --), 2016-07-31 03:33:52]

This is ready for review.

Would like to commit this as 1 CL. I can split out some of the
robolectric/BUILD.gn changes but the rest of the changes have to be committed
together.

Also, this will completely break the GYP robolectric build targets. IDK if I
should delete the GYP targets before landing this so people dont try building
it.

[mikecase (-- gone --), 2016-07-31 04:21:10]

https://codereview.chromium.org/2195143002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java
(right):

https://codereview.chromium.org/2195143002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:20:
public class ShadowGcmNetworkManager {
So, it looks like the GcmNetworkManager APIs changed a bit. Methods no longer
static. Updated.

https://codereview.chromium.org/2195143002/diff/60001/base/BUILD.gn
File base/BUILD.gn (left):

https://codereview.chromium.org/2195143002/diff/60001/base/BUILD.gn#oldcode2368
base/BUILD.gn:2368: # TODO(jbudorick): Remove this once we roll to robolectric
3.0 and pull
Switched to use the ShadowMultiDex from
//third_party/robolectric:shadows_multidex

https://codereview.chromium.org/2195143002/diff/60001/base/test/android/junit...
File base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/base/test/android/junit...
base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java:20:
@Config(manifest = Config.NONE, reportSdk = 19)
reportSdk was removed. Using sdk=19 Robolectric complains the
android-all-<version>.jar isnt present for sdk=19.
Changed to sdk=21 (we have the sdk 18 and 21 android-all jars available)

https://codereview.chromium.org/2195143002/diff/60001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2195143002/diff/60001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:49: # TODO(mikecase): Set this property
through GN.
Would like to replace this with a --robolectric-dep arg to the junit test
runner. Would have the test_runner wrapper script fill this in automatically.

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:322: srcjar_deps = [ "//base:base_build_config_gen" ]
So, this is needed or some tests can't find BuildConfig.java which is excluded
from the chrome_java jar.
This was previously a dep of the base_junit_test_support (which I removed).

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/gcore/GoogleApiClientHelperTest.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/gcore/GoogleApiClientHelperTest.java:55:
ConnectionResult mockResult = mock(ConnectionResult.class);
ConnectionResult class is now final and can't be mocked with Mockito. But, it
seems fine to just create regular ConnectionResult objects

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:27:
shadows = { ChromeMediaRouterTestBase.FakeActivityManager.class })
So this was shadowed out because SysUtils used to use
ActivityManager.isLowRamDevice()
By a coincidence, the test still passed once this implementation was changed it
seems.

Instead, I removed this shadow and set isLowRamDevice via reflection for the
tests.
Kinda a hack but it's not too bad imo. Better than it is now.

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:88:
mGcmNetworkManager = (ShadowGcmNetworkManager) ShadowExtractor.extract(
They seemed to have made it waay harder to create custom shadows. You either
have to call this (or Shadows.shadowOf, but the Shadows class is generated at
compile time by the Robolectric annotation processor and makes the build waaay
more complicated). You also have to ensure the class you want to shadow is
instrumented by Robolectric. See the OfflinePageTestRunner.java

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:20:
public class ShadowGcmNetworkManager {
API for the GcmNetworkManager class changed so I needed to update. I'm actually
not sure why this was using the old play_services classes before (I dont know
why this test didnt break when the play_services jar was updated. It doesn't
look like Robolectric has its own copy)

https://codereview.chromium.org/2195143002/diff/60001/components/invalidation...
File
components/invalidation/impl/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/components/invalidation...
components/invalidation/impl/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java:20:
@Config(manifest = Config.NONE, sdk = 18)
Tests that I have sdk=18 don't seem to work on sdk=21. (stackoverflow
recommended just setting sdk=18 when I looked up error)
Also, not sure why this test wasn't using the RobolectricTestRunner when it uses
some Android classes.

https://codereview.chromium.org/2195143002/diff/60001/content/public/android/...
File
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/content/public/android/...
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:160:
@Ignore("crbug/632792")
These tests just hang. The tests say "should not hang" so I'm not sure what is
up.

https://codereview.chromium.org/2195143002/diff/60001/testing/android/junit/j...
File
testing/android/junit/java/src/org/chromium/testing/local/RobolectricClasspathDependencyResolver.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/RobolectricClasspathDependencyResolver.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Now that I have multiple android-all-##.jars and multiple shadows-core-##.jar, I
problably shouldnt put them all in the classpath. I added something to
junit/test_runner.py to set the robolectric-deps-dir system property.
Robolectric is able to find the jars by looking there.

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
File third_party/robolectric/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/BUILD.gn:602: gen_dir =
"//out/Default/gen/third_party/robolectric/18/$gen_dir"
Ooooooh, I forgot I hard coded this. Any idea how to make this nicer. I need
multiple gen_dirs for the same template files. That is, I need to process the
template files multiple times for different api levels. I need to then build
shadows-core-<API LEVEL>.jar with the different template files.

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
File
third_party/robolectric/template_processor/src/org/chromium/testing/robolectric/template/ProcessRobolectricTemplate.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/template_processor/src/org/chromium/testing/robolectric/template/ProcessRobolectricTemplate.java:78:
String relativeOutputFile = "" + api_level + "/" +
templateFile.toString().replace(baseTemplateDir.toString(), "").replace(".vm",
"");
Definitely ugliest hack in this CL. I need to generate multiple versions of
shadow-core java files (for differnet API levels) from the same template files.
GN doesnt seem to support this case. 

See robolectric/BUILD.gn

[jbudorick, 2016-08-01 13:52:04]

This is a massive CL, and other reviewers are liable to ask, so about splitting:
 - can this be split by target?
 - can the GN stuff be done beforehand?

I think deleting the gyp targets sounds reasonable.

https://codereview.chromium.org/2195143002/diff/60001/base/test/android/junit...
File base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/base/test/android/junit...
base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java:20:
@Config(manifest = Config.NONE, reportSdk = 19)
On 2016/07/31 04:21:09, mikecase wrote:
> reportSdk was removed. Using sdk=19 Robolectric complains the
> android-all-<version>.jar isnt present for sdk=19.
> Changed to sdk=21 (we have the sdk 18 and 21 android-all jars available)

sgtm. Looks like you've preserved the intent of these tests.

https://codereview.chromium.org/2195143002/diff/60001/build/android/pylib/jun...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2195143002/diff/60001/build/android/pylib/jun...
build/android/pylib/junit/test_runner.py:49: # TODO(mikecase): Set this property
through GN.
On 2016/07/31 04:21:09, mikecase wrote:
> Would like to replace this with a --robolectric-dep arg to the junit test
> runner. Would have the test_runner wrapper script fill this in automatically.

That sgtm. Fine with leaving it in a separate CL, though.

https://codereview.chromium.org/2195143002/diff/60001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2195143002/diff/60001/build/config/android/in...
build/config/android/internal_rules.gni:54:
"//third_party/robolectric:shadows-core-3.0-21",
nit: alpha

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:83:
AccountManager manager = (AccountManager) RuntimeEnvironment
nit: not crazy about this line split.

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java:134:
// TODO(jbudorick): Remove this once we roll to Robolectric 3.0, which should
contain
This CL might get the record for most TODO(jbudorick)s removed :)

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java
(left):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:27:
shadows = { ChromeMediaRouterTestBase.FakeActivityManager.class })
On 2016/07/31 04:21:09, mikecase wrote:
> So this was shadowed out because SysUtils used to use
> ActivityManager.isLowRamDevice()
> By a coincidence, the test still passed once this implementation was changed
it
> seems.
> 
> Instead, I removed this shadow and set isLowRamDevice via reflection for the
> tests.
> Kinda a hack but it's not too bad imo. Better than it is now.

Agreed. Passing by coincidence is not great.

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:88:
mGcmNetworkManager = (ShadowGcmNetworkManager) ShadowExtractor.extract(
On 2016/07/31 04:21:09, mikecase wrote:
> They seemed to have made it waay harder to create custom shadows. You either
> have to call this (or Shadows.shadowOf, but the Shadows class is generated at
> compile time by the Robolectric annotation processor and makes the build waaay
> more complicated). You also have to ensure the class you want to shadow is
> instrumented by Robolectric. See the OfflinePageTestRunner.java

Can you write up a brief doc in
https://chromium.googlesource.com/chromium/src/+/master/docs/ explaining this?

https://codereview.chromium.org/2195143002/diff/60001/content/public/android/...
File
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/content/public/android/...
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:160:
@Ignore("crbug/632792")
On 2016/07/31 04:21:10, mikecase wrote:
> These tests just hang. The tests say "should not hang" so I'm not sure what is
> up.

Sounds like something for the author/owner of this test to investigate.

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
File third_party/robolectric/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/BUILD.gn:522: # TODO(mikecase): Template this build file
so it is easier to add more sdk levels to Robolectric.
Could this be done prior to and separate from this CL?

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/BUILD.gn:602: gen_dir =
"//out/Default/gen/third_party/robolectric/18/$gen_dir"
On 2016/07/31 04:21:10, mikecase wrote:
> Ooooooh, I forgot I hard coded this. Any idea how to make this nicer. I need
> multiple gen_dirs for the same template files. That is, I need to process the
> template files multiple times for different api levels. I need to then build
> shadows-core-<API LEVEL>.jar with the different template files.

I mean, your idea of templating on SDK level sounds reasonable.

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
File
third_party/robolectric/template_processor/src/org/chromium/testing/robolectric/template/ProcessRobolectricTemplate.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/template_processor/src/org/chromium/testing/robolectric/template/ProcessRobolectricTemplate.java:78:
String relativeOutputFile = "" + api_level + "/" +
templateFile.toString().replace(baseTemplateDir.toString(), "").replace(".vm",
"");
On 2016/07/31 04:21:10, mikecase wrote:
> Definitely ugliest hack in this CL. I need to generate multiple versions of
> shadow-core java files (for differnet API levels) from the same template
files.
> GN doesnt seem to support this case. 
> 
> See robolectric/BUILD.gn

😬

[mikecase (-- gone --), 2016-08-01 21:34:32]

Description was changed from

==========
Update all Robolectric tests to Robolectric 3.0

BUG=
==========

to

==========
Update all Robolectric tests to Robolectric 3.0

BUG=522043
==========

[mikecase (-- gone --), 2016-08-02 06:12:26]

linux_android_rel_ng passed! (prev patch set). Plan to add a bunch of reviewers
tomorrow morning. \o/

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:83:
AccountManager manager = (AccountManager) RuntimeEnvironment
On 2016/08/01 at 13:52:04, jbudorick wrote:
> nit: not crazy about this line split.

Changed it to a line split you will be absolutely crazy about!

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
File third_party/robolectric/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/BUILD.gn:602: gen_dir =
"//out/Default/gen/third_party/robolectric/18/$gen_dir"
On 2016/08/01 at 13:52:04, jbudorick wrote:
> On 2016/07/31 04:21:10, mikecase wrote:
> > Ooooooh, I forgot I hard coded this. Any idea how to make this nicer. I need
> > multiple gen_dirs for the same template files. That is, I need to process
the
> > template files multiple times for different api levels. I need to then build
> > shadows-core-<API LEVEL>.jar with the different template files.
> 
> I mean, your idea of templating on SDK level sounds reasonable.

Done. Committed in a different CL. This CL is now rebased on top of that one.

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
File
third_party/robolectric/template_processor/src/org/chromium/testing/robolectric/template/ProcessRobolectricTemplate.java
(right):

https://codereview.chromium.org/2195143002/diff/60001/third_party/robolectric...
third_party/robolectric/template_processor/src/org/chromium/testing/robolectric/template/ProcessRobolectricTemplate.java:78:
String relativeOutputFile = "" + api_level + "/" +
templateFile.toString().replace(baseTemplateDir.toString(), "").replace(".vm",
"");
On 2016/08/01 at 13:52:04, jbudorick wrote:
> On 2016/07/31 04:21:10, mikecase wrote:
> > Definitely ugliest hack in this CL. I need to generate multiple versions of
> > shadow-core java files (for differnet API levels) from the same template
files.
> > GN doesnt seem to support this case. 
> > 
> > See robolectric/BUILD.gn
> 
> 😬

Well, this got kinda committed in a different CL. It ended up not being too bad.
This CL is now rebased on top of that CL 

https://codereview.chromium.org/2197403003

https://codereview.chromium.org/2195143002/diff/100001/third_party/robolectri...
File third_party/robolectric/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/100001/third_party/robolectri...
third_party/robolectric/BUILD.gn:574: additional_jar_files = [
This didnt end up working how I wanted. I assumed is_linux would be true if you
were building on a linux machine. Not the case :/
Probably only the linux .so are necessary (since idk of anyone that builds for
android on a non-linux machine). But the other binaries are there, so why not
include them

[mikecase (-- gone --), 2016-08-02 14:21:08]

mikecase@chromium.org changed reviewers:
+ jochen@chromium.org, pauljensen@chromium.org, tedchoc@chromium.org,
thestig@chromium.org

[mikecase (-- gone --), 2016-08-02 14:21:09]

+ jochen@chromium.org to review components/
+ pauljensen@chromium.org to review net/
+ thestig@chromium.org to review base/

+ tedchoc@chromium.org to review
ui/android/
content/public/android/
chrome/android/

+ jbudorick@chromium.org  to review
testing/android/
build/
third_party/robolectric/

In patch set 4, I posted a few comments to explain things people might have
questions about. Mostly, this CL is straightforward and just updates the
Robolectric APIs from version 2.4 -> 3.0.

[mikecase (-- gone --), 2016-08-02 14:21:39]

The CQ bit was checked by mikecase@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 14:21:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 14:47:22]

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

[commit-bot: I haz the power, 2016-08-02 14:47:23]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[jochen (gone - plz use gerrit), 2016-08-02 15:33:27]

I don't know about Robolectric.

Can you please get a reviewer that knows about this library and its correct
usage to review the entire CL? Happy to stamp afterwards

[mikecase (-- gone --), 2016-08-02 15:41:15]

mikecase@chromium.org changed reviewers:
+ aberent@chromium.org

[mikecase (-- gone --), 2016-08-02 15:41:16]

+ aberent@ to review the changes in components/. It looks like you wrote one of
the tests there.

[aberent, 2016-08-02 16:13:54]

Components lgtm with one nit.

https://codereview.chromium.org/2195143002/diff/160001/components/invalidatio...
File
components/invalidation/impl/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/components/invalidatio...
components/invalidation/impl/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java:19:
@RunWith(LocalRobolectricTestRunner.class)
Why this change? This appears to be a pure Junit test, making no use of Android,
why not leave it as such?

[mikecase (-- gone --), 2016-08-02 16:47:29]

https://codereview.chromium.org/2195143002/diff/160001/components/invalidatio...
File
components/invalidation/impl/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/components/invalidatio...
components/invalidation/impl/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java:19:
@RunWith(LocalRobolectricTestRunner.class)
On 2016/08/02 at 16:13:54, aberent wrote:
> Why this change? This appears to be a pure Junit test, making no use of
Android, why not leave it as such?

This test uses android.os.Bundle. Basically, in these Junit classes, any time
you use android classes they are using the Robolectric version of the classes
and you should make your test a Robolectric test.

Especially if you set something like @Config(sdk=<api level>) for the test,
Robolectric will load the appropriate version of the android sdk at runtime but
only if you use the RobolectricTestRunner.

[Lei Zhang, 2016-08-02 16:55:35]

On 2016/08/02 14:21:09, mikecase wrote:
> + mailto:thestig@chromium.org to review base/

Please ask base/android/OWNERS. Happy to stamp for any remaining bits once they
are happy.

[mikecase (-- gone --), 2016-08-02 16:58:41]

mikecase@chromium.org changed reviewers:
+ nyquist@chromium.org

[mikecase (-- gone --), 2016-08-02 16:58:42]

+nyquist for review of /base/android

[jbudorick, 2016-08-03 15:32:29]

https://codereview.chromium.org/2195143002/diff/160001/build/android/pylib/ju...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2195143002/diff/160001/build/android/pylib/ju...
build/android/pylib/junit/test_runner.py:49: # TODO(mikecase): Set this property
through GN.
Can you add a note about when you plan to address this or what it's blocked on?

https://codereview.chromium.org/2195143002/diff/160001/testing/android/junit/...
File
testing/android/junit/java/src/org/chromium/testing/local/LocalRobolectricTestRunner.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/LocalRobolectricTestRunner.java:33:
if (config != null && config.sdk().length > 1) {
nit: wdyt about

  if (config != null) {
      if (config.sdk().length > 1) {
          throw ...
      } else if (config.sdk().length == 1) {
          return config.sdk()[0];
      }
  }
  return DEFAULT_ANDROID_API_LEVEL;

https://codereview.chromium.org/2195143002/diff/160001/third_party/robolectri...
File third_party/robolectric/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/160001/third_party/robolectri...
third_party/robolectric/BUILD.gn:574: additional_jar_files = [
Why are we now adding all of these?

[Ted C, 2016-08-03 19:18:26]

https://codereview.chromium.org/2195143002/diff/160001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:115:
Field field = SysUtils.class.getDeclaredField("sLowEndDevice");
We should really, really, really try to never use reflection for code we own. 
For these tests, can they use command lines?

BaseSwitches.ENABLE_LOW_END_DEVICE_MODE
BaseSwitches.DISABLE_LOW_END_DEVICE_MODE

Both exist for tests doing exactly this.

https://codereview.chromium.org/2195143002/diff/160001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:160:
@Ignore("crbug/632792")
tiny nit, I'd vote for crbug.com/<#> to make it a link-y thing

https://codereview.chromium.org/2195143002/diff/160001/ui/android/junit/src/o...
File ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/ui/android/junit/src/o...
ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java:25:
@Config(manifest = Config.NONE, sdk = 18)
Might have been explained but why do we need to specify sdk?  What does it
actually represent?

Is it the minimum sdk that we expect this to run against?  In which case,
wouldn't we want it to be 16 as the first jellybean release?

If it is just the sdk this is built against, don't we always want the latest?

[mikecase (-- gone --), 2016-08-05 03:17:07]

https://codereview.chromium.org/2195143002/diff/160001/build/android/pylib/ju...
File build/android/pylib/junit/test_runner.py (right):

https://codereview.chromium.org/2195143002/diff/160001/build/android/pylib/ju...
build/android/pylib/junit/test_runner.py:49: # TODO(mikecase): Set this property
through GN.
On 2016/08/03 at 15:32:29, jbudorick wrote:
> Can you add a note about when you plan to address this or what it's blocked
on?

Added more detail desc. Not blocked on anything, just wanting to keep CL size
down. Will add --robolectric-dep-dir as a test_runner arg, and set it in GN for
the generated test runner scripts.

https://codereview.chromium.org/2195143002/diff/160001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:115:
Field field = SysUtils.class.getDeclaredField("sLowEndDevice");
On 2016/08/03 at 19:18:26, Ted C wrote:
> We should really, really, really try to never use reflection for code we own. 
For these tests, can they use command lines?
> 
> BaseSwitches.ENABLE_LOW_END_DEVICE_MODE
> BaseSwitches.DISABLE_LOW_END_DEVICE_MODE
> 
> Both exist for tests doing exactly this.

Didn't know these switches existed. Fixed. Way simpler/better now.

https://codereview.chromium.org/2195143002/diff/160001/content/public/android...
File
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/content/public/android...
content/public/android/junit/src/org/chromium/content/browser/input/ThreadedInputConnectionTest.java:160:
@Ignore("crbug/632792")
On 2016/08/03 at 19:18:26, Ted C wrote:
> tiny nit, I'd vote for crbug.com/<#> to make it a link-y thing

Done

https://codereview.chromium.org/2195143002/diff/160001/testing/android/junit/...
File
testing/android/junit/java/src/org/chromium/testing/local/LocalRobolectricTestRunner.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/LocalRobolectricTestRunner.java:33:
if (config != null && config.sdk().length > 1) {
On 2016/08/03 at 15:32:29, jbudorick wrote:
> nit: wdyt about
> 
>   if (config != null) {
>       if (config.sdk().length > 1) {
>           throw ...
>       } else if (config.sdk().length == 1) {
>           return config.sdk()[0];
>       }
>   }
>   return DEFAULT_ANDROID_API_LEVEL;

Done

https://codereview.chromium.org/2195143002/diff/160001/third_party/robolectri...
File third_party/robolectric/BUILD.gn (right):

https://codereview.chromium.org/2195143002/diff/160001/third_party/robolectri...
third_party/robolectric/BUILD.gn:574: additional_jar_files = [
On 2016/08/03 at 15:32:29, jbudorick wrote:
> Why are we now adding all of these?

I made a quick comment about this. When I changed this before, I assumed
is_linux would be true if you were on a linux workstation. Pretty sure is_linux
means you are building for linux. Really, you just need the library for w/e your
workstation is. But yeah, when I tried building and running tests with how it
was before, it was giving me an error that linux-x86_64/libsqlite4java.so
couldn't be found.

https://codereview.chromium.org/2195143002/diff/160001/ui/android/junit/src/o...
File ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/ui/android/junit/src/o...
ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java:25:
@Config(manifest = Config.NONE, sdk = 18)
On 2016/08/03 at 19:18:26, Ted C wrote:
> Might have been explained but why do we need to specify sdk?  What does it
actually represent?
> 
> Is it the minimum sdk that we expect this to run against?  In which case,
wouldn't we want it to be 16 as the first jellybean release?
> 
> If it is just the sdk this is built against, don't we always want the latest?

Robolectric has special versions of the android sdk with classes that are able
to run in your workstations jvm.
You can specify which of these sdks is loaded by Robolectric when running a
test. Previously, only Robolectric's android
sdk 18 was available in Chromium (the highest sdk level supported by Robolectric
2.4). Now I'm adding support for sdk 21
(the highest sdk level supported by Robolectric 3.0) and making it the default
(see LocalRobolectricTestRunner.java 
DEFAULT_ANDROID_SDK).

There are a couple tests that don't seem to work with sdk=21.
In this specific instance, Im hitting this error. 
https://github.com/robolectric/robolectric/issues/2024

Setting sdk=18 (which was what was being used previously) fixes the issue and
seems to be the recommended fix for this issue.

[jbudorick, 2016-08-05 15:38:45]

https://codereview.chromium.org/2195143002/diff/200001/third_party/robolectri...
File third_party/robolectric/BUILD.gn (left):

https://codereview.chromium.org/2195143002/diff/200001/third_party/robolectri...
third_party/robolectric/BUILD.gn:597: if (is_linux) {
I think what you wanted was
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

[mikecase (-- gone --), 2016-08-05 16:15:43]

https://codereview.chromium.org/2195143002/diff/200001/third_party/robolectri...
File third_party/robolectric/BUILD.gn (left):

https://codereview.chromium.org/2195143002/diff/200001/third_party/robolectri...
third_party/robolectric/BUILD.gn:597: if (is_linux) {
On 2016/08/05 at 15:38:45, jbudorick wrote:
> I think what you wanted was
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

Woohoo! This works. Updated.

[Ted C, 2016-08-05 20:17:15]

https://codereview.chromium.org/2195143002/diff/160001/ui/android/junit/src/o...
File ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java
(right):

https://codereview.chromium.org/2195143002/diff/160001/ui/android/junit/src/o...
ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java:25:
@Config(manifest = Config.NONE, sdk = 18)
On 2016/08/05 03:17:07, mikecase wrote:
> On 2016/08/03 at 19:18:26, Ted C wrote:
> > Might have been explained but why do we need to specify sdk?  What does it
> actually represent?
> > 
> > Is it the minimum sdk that we expect this to run against?  In which case,
> wouldn't we want it to be 16 as the first jellybean release?
> > 
> > If it is just the sdk this is built against, don't we always want the
latest?
> 
> Robolectric has special versions of the android sdk with classes that are able
> to run in your workstations jvm.
> You can specify which of these sdks is loaded by Robolectric when running a
> test. Previously, only Robolectric's android
> sdk 18 was available in Chromium (the highest sdk level supported by
Robolectric
> 2.4). Now I'm adding support for sdk 21
> (the highest sdk level supported by Robolectric 3.0) and making it the default
> (see LocalRobolectricTestRunner.java 
> DEFAULT_ANDROID_SDK).
> 
> There are a couple tests that don't seem to work with sdk=21.
> In this specific instance, Im hitting this error. 
> https://github.com/robolectric/robolectric/issues/2024
> 
> Setting sdk=18 (which was what was being used previously) fixes the issue and
> seems to be the recommended fix for this issue.

So if you don't specify sdk, do you get the most recent version of the
roboelectric sdk by default (i.e. 21)?

For example,
https://codereview.chromium.org/2195143002/diff/160001/base/test/android/juni...
that test seems to specify 21 despite that being the most up to date one.

Ideally, all our tests would just use the latest version of roboelectric and
"just work".  I'd prefer to just have to specify the sdk if the new one breaks
an existing tests (and I'd prefer they have a crbug associated with each one
that maybe just points to the roboelectric bug you found...ideally, these tests
would update to the newer sdk once the bug in the underlying framework was
addressed).

Buuut..if it doesn't pick up a default sdk, then I guess this is what we'll have
to do.

[nyquist, 2016-08-05 21:56:45]

I would agree with Ted that it would be awesome with a default SDK version

[mikecase (-- gone --), 2016-08-06 01:16:13]

https://codereview.chromium.org/2195143002/diff/240001/base/test/android/juni...
File base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java
(right):

https://codereview.chromium.org/2195143002/diff/240001/base/test/android/juni...
base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java:20:
@Config(manifest = Config.NONE, sdk = 21)
Im just setting sdk=21 explicitly in this test because it tests things like
"sdk_is_less_than" annotations.
sdk=21 is the default, but just want to make it clear that it is being used
(since it is very relevant for these tests)

https://codereview.chromium.org/2195143002/diff/240001/testing/android/junit/...
File
testing/android/junit/java/src/org/chromium/testing/local/LocalRobolectricTestRunner.java
(right):

https://codereview.chromium.org/2195143002/diff/240001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/LocalRobolectricTestRunner.java:22:
private static final int DEFAULT_ANDROID_API_LEVEL = 21;
All tests by default will use sdk=21 now.

The sdk version is set by the pickSdkVersion() method below, which basically
just looks for the sdk=<> in the config. If no sdk is set in the config, it just
uses sdk=21.

https://codereview.chromium.org/2195143002/diff/240001/ui/android/junit/src/o...
File ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java
(right):

https://codereview.chromium.org/2195143002/diff/240001/ui/android/junit/src/o...
ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java:25: //
sdk 18 is used due to a bug in Robolectric, crbug.com/635199
Added crbug for needing sdk=18 on this test.

[Ted C, 2016-08-06 03:14:38]

lgtm...thanks for all the explanations!

On Aug 5, 2016 6:16 PM, <mikecase@chromium.org> wrote:

>
> https://codereview.chromium.org/2195143002/diff/240001/
> base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java
> File
> base/test/android/junit/src/org/chromium/base/test/util/DisableIfTest.java
> (right):
>
> https://codereview.chromium.org/2195143002/diff/240001/
> base/test/android/junit/src/org/chromium/base/test/util/
> DisableIfTest.java#newcode20
> base/test/android/junit/src/org/chromium/base/test/util/
> DisableIfTest.java:20:
> @Config(manifest = Config.NONE, sdk = 21)
> Im just setting sdk=21 explicitly in this test because it tests things
> like "sdk_is_less_than" annotations.
> sdk=21 is the default, but just want to make it clear that it is being
> used (since it is very relevant for these tests)
>
> https://codereview.chromium.org/2195143002/diff/240001/
> testing/android/junit/java/src/org/chromium/testing/local/
> LocalRobolectricTestRunner.java
> File
> testing/android/junit/java/src/org/chromium/testing/local/
> LocalRobolectricTestRunner.java
> (right):
>
> https://codereview.chromium.org/2195143002/diff/240001/
> testing/android/junit/java/src/org/chromium/testing/local/
> LocalRobolectricTestRunner.java#newcode22
> testing/android/junit/java/src/org/chromium/testing/local/
> LocalRobolectricTestRunner.java:22:
> private static final int DEFAULT_ANDROID_API_LEVEL = 21;
> All tests by default will use sdk=21 now.
>
> The sdk version is set by the pickSdkVersion() method below, which
> basically just looks for the sdk=<> in the config. If no sdk is set in
> the config, it just uses sdk=21.
>
> https://codereview.chromium.org/2195143002/diff/240001/ui/
> android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java
> File
> ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java
> (right):
>
> https://codereview.chromium.org/2195143002/diff/240001/ui/
> android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java#
> newcode25
> ui/android/junit/src/org/chromium/ui/picker/DateDialogNormalizerTest.java:
> 25:
> // sdk 18 is used due to a bug in Robolectric, crbug.com/635199
> Added crbug for needing sdk=18 on this test.
>
> https://codereview.chromium.org/2195143002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2016-08-06 06:43:21]

rs lgtm for base and chrome

[Ted C, 2016-08-06 13:53:33]

lgtm

[jbudorick, 2016-08-08 17:59:10]

build/, testing/android/, and third_party/robolectric/ lgtm

[mikecase (-- gone --), 2016-08-08 18:45:45]

jochen@chromium.org could you take a quick look / stamp for components. I added
aberent@ to look at the components files and he gave .lgtm

pauljensen@chromium.org, could you quick take a look at net/ . Not any big
changes to those files.

ty :D

[mikecase (-- gone --), 2016-08-08 19:11:02]

mikecase@chromium.org changed reviewers:
+ caitkp@chromium.org
- jochen@chromium.org

[mikecase (-- gone --), 2016-08-08 19:11:04]

jochen is OOO.

+ caitkp@chromium.org, please take a look at components/
aberent@ looked at the components/ changes already but I still need an OWNERs
stamp.

Thanks :D

[Cait (Slow), 2016-08-08 19:17:35]

rs lgtm for components/

[mikecase (-- gone --), 2016-08-08 20:19:30]

mikecase@chromium.org changed reviewers:
+ cbentzel@chromium.org
- pauljensen@chromium.org

[mikecase (-- gone --), 2016-08-08 20:19:31]

pauljensen is also OOO this week.

+ cbentzel@ to review net/ changes instead. Thanks :D

[cbentzel, 2016-08-09 17:05:02]

On 2016/08/08 20:19:31, mikecase wrote:
> pauljensen is also OOO this week.
> 
> + cbentzel@ to review net/ changes instead. Thanks :D

net/ LGTM

[cbentzel, 2016-08-09 17:05:07]

On 2016/08/08 20:19:31, mikecase wrote:
> pauljensen is also OOO this week.
> 
> + cbentzel@ to review net/ changes instead. Thanks :D

net/ LGTM

[nyquist, 2016-08-10 20:05:36]

lgtm

[mikecase (-- gone --), 2016-08-10 20:32:09]

The CQ bit was checked by mikecase@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 20:33:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 20:36:42]

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

[commit-bot: I haz the power, 2016-08-10 20:36:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mikecase (-- gone --), 2016-08-10 20:45:09]

The CQ bit was checked by mikecase@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 20:45:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 23:15:14]

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

[commit-bot: I haz the power, 2016-08-10 23:15:16]

Dry run: This issue passed the CQ dry run.

[mikecase (-- gone --), 2016-08-11 16:54:04]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2016-08-11 16:54:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
aberent@chromium.org, cbentzel@chromium.org, thestig@chromium.org,
jbudorick@chromium.org, tedchoc@chromium.org, nyquist@chromium.org,
caitkp@chromium.org
Link to the patchset: https://codereview.chromium.org/2195143002/#ps260001
(title: "Rebase!")

[commit-bot: I haz the power, 2016-08-11 16:54:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 17:00:21]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-11 17:03:19]

Description was changed from

==========
Update all Robolectric tests to Robolectric 3.0

BUG=522043
==========

to

==========
Update all Robolectric tests to Robolectric 3.0

BUG=522043

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

[commit-bot: I haz the power, 2016-08-11 17:03:21]

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

[mikecase (-- gone --), 2016-08-11 17:21:09]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/2239003002/ by mikecase@chromium.org.

The reason for reverting is: Compile errors here...
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder/builds....

[mikecase (-- gone --), 2016-08-11 17:29:53]

On 2016/08/11 at 17:21:09, mikecase wrote:
> A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/2239003002/ by mikecase@chromium.org.
> 
> The reason for reverting is: Compile errors here...
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Builder/builds....

Someone committed a new Robolectric test between when I did the CQ dry-run and
committing this :/

[findit-for-me, 2016-08-11 17:41:58]

FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
at revision 411362 as the culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...