Update Robolectric to 3.2.2

Update Robolectric to 3.2.2

BUG=693659
Review-Url: https://codereview.chromium.org/2710343003
Cr-Commit-Position: refs/heads/master@{#457560}
Committed: https://chromium.googlesource.com/chromium/src/+/6a5f508e65cf947c6292fe8578868d0fe739f0aa

[mikecase (-- gone --), 2017-02-24 21:22:13]

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

[mikecase (-- gone --), 2017-02-24 21:22:14]

Need to land this CL to Chromium clone of Robolectric repo.
https://codereview.appspot.com/319410043/
and then DEPS that version in.

I think as-is, all Robolectric tests are passing with 3 exceptions.
- For 2 classes I had to set SDK = 21 to fix a problem. Maybe some Android APIs
changed which broke the tests? Not sure.
- For 1 class I had to disable 3 tests that I can't get working. They changed
how Activitys/ContentProviders and some other things are constructed in
Robolectric...

see first paragraph:
https://github.com/robolectric/robolectric/wiki/3.0-to-3.1-Upgrade-Guide

This seems to have broken these tests.


Here are the overall changes:
- Upgrade Guava to 20.0
- Upgrade accessibility-test-framework to 2.1
- Add xstream library
- Add android-all-7.1 jar
- Remove Velocity Template code completely. They no longer use templated Java
code in build. This simplifies things quite a bit
- Remove per SDK shadow libraries. I think they do some magic at runtime now,
and no longer load different shadows for each SDK level.

[mikecase (-- gone --), 2017-02-24 22:00:34]

https://codereview.chromium.org/2710343003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/2710343003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:187:
@Ignore
https://github.com/robolectric/robolectric/commit/80d495d66e5207dd0c35bc06799...

is the reason these are failing. getContext() was removed from the
ContextProvider shadow. Adding it back fixes the test, but there's probably a
reason they removed it?

[jbudorick, 2017-02-27 02:16:44]

https://codereview.chromium.org/2710343003/diff/40001/DEPS
File DEPS (right):

https://codereview.chromium.org/2710343003/diff/40001/DEPS#newcode772
DEPS:772: {
OSTPR?

https://codereview.chromium.org/2710343003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/2710343003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:187:
@Ignore
On 2017/02/24 22:00:33, mikecase wrote:
>
https://github.com/robolectric/robolectric/commit/80d495d66e5207dd0c35bc06799...
> 
> is the reason these are failing. getContext() was removed from the
> ContextProvider shadow. Adding it back fixes the test, but there's probably a
> reason they removed it?

I'd guess that there's a better way to get the Context.

https://codereview.chromium.org/2710343003/diff/40001/third_party/guava/lib/g...
File third_party/guava/lib/guava-18.0.jar.sha1 (left):

https://codereview.chromium.org/2710343003/diff/40001/third_party/guava/lib/g...
third_party/guava/lib/guava-18.0.jar.sha1:1:
cce0823396aa693798f8882e64213b1772032b09
Will the jars that people have previously downloaded get removed? If not, will
we at least mention something along the lines of what gclient does when a repo
is no longer used ("xyz is no longer necessary, it's recommended that you delete
it" or something along those lines)?

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

https://codereview.chromium.org/2710343003/diff/40001/third_party/robolectric...
third_party/robolectric/BUILD.gn:15: ":shadows_core_java",
Out of curiosity, why are you de-versioning these? Less headache when upgrading?

[Bernhard Bauer, 2017-02-27 10:02:32]

Description was changed from

==========
Update Robolectric to 3.2

BUG=693659
==========

to

==========
Update Robolectric to 3.2

BUG=693659
==========

[mikecase (-- gone --), 2017-02-27 16:21:06]

https://codereview.chromium.org/2710343003/diff/40001/DEPS
File DEPS (right):

https://codereview.chromium.org/2710343003/diff/40001/DEPS#newcode772
DEPS:772: {
On 2017/02/27 at 02:16:44, jbudorick is ooo until monday wrote:
> OSTPR?

yeah, I sent out an email. The library has the same License (BSD 3-clause) as
many other things in Chromium so Im guessing it will be fine.

https://codereview.chromium.org/2710343003/diff/40001/third_party/guava/lib/g...
File third_party/guava/lib/guava-18.0.jar.sha1 (left):

https://codereview.chromium.org/2710343003/diff/40001/third_party/guava/lib/g...
third_party/guava/lib/guava-18.0.jar.sha1:1:
cce0823396aa693798f8882e64213b1772032b09
On 2017/02/27 at 02:16:44, jbudorick is ooo until monday wrote:
> Will the jars that people have previously downloaded get removed? If not, will
we at least mention something along the lines of what gclient does when a repo
is no longer used ("xyz is no longer necessary, it's recommended that you delete
it" or something along those lines)?

No, I dont think jars that people have will get removed. Un

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

https://codereview.chromium.org/2710343003/diff/40001/third_party/robolectric...
third_party/robolectric/BUILD.gn:15: ":shadows_core_java",
On 2017/02/27 at 02:16:44, jbudorick is ooo until monday wrote:
> Out of curiosity, why are you de-versioning these? Less headache when
upgrading?

Two reasons. First is that Robolectric removed per-sdk Shadow libraries. Before
you had to build a separate versions of the shadows.jar for each SDK level. Now
they do some fancy runtime shenanigans.

Secondly, we still need the jar to have a specific name. Previously it had this
name so that GN would create the correctly named jar. Now there is an
output_name GN variable where you can explicitly specify the output jar name.

[mikecase (-- gone --), 2017-02-27 18:03:26]

https://codereview.chromium.org/2710343003/diff/40001/DEPS
File DEPS (right):

https://codereview.chromium.org/2710343003/diff/40001/DEPS#newcode772
DEPS:772: {
On 2017/02/27 at 16:21:06, mikecase wrote:
> On 2017/02/27 at 02:16:44, jbudorick is ooo until monday wrote:
> > OSTPR?
> 
> yeah, I sent out an email. The library has the same License (BSD 3-clause) as
many other things in Chromium so Im guessing it will be fine.

Got lgtm for adding xstream. Factored out xstream stuff here.

https://codereview.chromium.org/2715323002/

Will rebase once it lands

https://codereview.chromium.org/2710343003/diff/40001/third_party/guava/lib/g...
File third_party/guava/lib/guava-18.0.jar.sha1 (left):

https://codereview.chromium.org/2710343003/diff/40001/third_party/guava/lib/g...
third_party/guava/lib/guava-18.0.jar.sha1:1:
cce0823396aa693798f8882e64213b1772032b09
On 2017/02/27 at 16:21:06, mikecase wrote:
> On 2017/02/27 at 02:16:44, jbudorick is ooo until monday wrote:
> > Will the jars that people have previously downloaded get removed? If not,
will we at least mention something along the lines of what gclient does when a
repo is no longer used ("xyz is no longer necessary, it's recommended that you
delete it" or something along those lines)?
> 
> No, I dont think jars that people have will get removed. Un

What I could do, is add the JARs I need explicitly named to the .gitignore
(instead of ignoring guava/lib/*.jar, I would ignore guava/lib/guava-20.0.jar).
So at least people would see the files they no longer need and can delete them.

[mikecase (-- gone --), 2017-02-27 20:04:22]

Description was changed from

==========
Update Robolectric to 3.2

BUG=693659
==========

to

==========
Update Robolectric to 3.2.2

BUG=693659
==========

[mikecase (-- gone --), 2017-03-01 00:37:54]

Rebased now that Xstream is committed.

Just need to commit this and https://codereview.appspot.com/319410043/ and
Robolectric will be updated!

After commiting other CL, I will need to update DEPS to pick up that Robolectric
revision.

PTAL!

[jbudorick, 2017-03-01 02:19:12]

https://codereview.chromium.org/2710343003/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/2710343003/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:187:
@Ignore
On 2017/02/27 02:16:44, jbudorick wrote:
> On 2017/02/24 22:00:33, mikecase wrote:
> >
>
https://github.com/robolectric/robolectric/commit/80d495d66e5207dd0c35bc06799...
> > 
> > is the reason these are failing. getContext() was removed from the
> > ContextProvider shadow. Adding it back fixes the test, but there's probably
a
> > reason they removed it?
> 
> I'd guess that there's a better way to get the Context.

(This is the comment I alluded to on the other review.)

[mikecase (-- gone --), 2017-03-01 22:37:17]

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

[mikecase (-- gone --), 2017-03-01 22:37:18]

aberent@ can you take a look at the changes to
SupervisedUserContentProviderUnitTest.java

Basically, Robolectric removed getContext() from the ShadowContentProvider
here...
https://github.com/robolectric/robolectric/commit/80d495d66e5207dd0c35bc06799...
...which causes problems in 3 of these tests.

Not at all familiar with this code, but my changes do fix the tests.

[mikecase (-- gone --), 2017-03-02 06:32:31]

Fixed LocalRobolectricTestRunner. Now it actually uses SDK level 25 when running
tests. They actually made it a bit more difficult to override the SDK tests are
run with. I think this is as a consequence of Robolectric now letting you run a
test multiple times with different SDK levels now. We might eventually want to
pass the AndroidManifests to Robolectric which might make this more
straightforward.

Additionally, ran tryjob and it failed since bots don't have Java 8 :(  Rumor is
they could have Java 8 as early as Thurs

[mikecase (-- gone --), 2017-03-07 20:29:40]

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

[mikecase (-- gone --), 2017-03-07 20:29:41]

This is ready for review! All junit tests seem to pass upstream and downstream
with this upgrade to Robolectric 3.2.2. Please take a look


+ nyquist@ for changes to chrome/android

- adding sdk=21 for tests that didn't work getting upgraded to sdk=25 for now.
- was able to remove a special test runner I had to make before since no longer
needed
- created special ShadowContentProvider in SupervisedUserContentProviderUnitTest
since they removed the getContext() API that this test relies on.

+ jwd@ for change to compnents/variations

+ jbudorick@ for testing/android and third_party/robolectric

- Greatly simplified build file since (1) no more API specific shadow jars (2)
no more templated java files
- Added GNManifestFactory so that overriding the packageName in the @Config
annotation actually works.
- Rewrote LocalRobolectricTestRunner due to some API changes
- Upgraded guava and accessibility test framewokr versions.

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
File
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:50:
ContextUtils.initApplicationContextForTests(RuntimeEnvironment.application);
Adding this fixed some errors when running the test. Unsure if this is correct
though. Would appreciate if someone more familiar with the code confirmed

[jwd, 2017-03-08 22:44:33]

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
File
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:50:
ContextUtils.initApplicationContextForTests(RuntimeEnvironment.application);
On 2017/03/07 20:29:41, mikecase wrote:
> Adding this fixed some errors when running the test. Unsure if this is correct
> though. Would appreciate if someone more familiar with the code confirmed

What does this line do? The robolectric doc on RuntimeEnvironment.application
doesn't describe it
http://robolectric.org/javadoc/3.2/org/robolectric/RuntimeEnvironment.html#ap....

What were the errors you were getting without it?

[mikecase (-- gone --), 2017-03-09 01:50:28]

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
File
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:50:
ContextUtils.initApplicationContextForTests(RuntimeEnvironment.application);
On 2017/03/08 at 22:44:33, jwd wrote:
> On 2017/03/07 20:29:41, mikecase wrote:
> > Adding this fixed some errors when running the test. Unsure if this is
correct
> > though. Would appreciate if someone more familiar with the code confirmed
> 
> What does this line do? The robolectric doc on RuntimeEnvironment.application
doesn't describe it
http://robolectric.org/javadoc/3.2/org/robolectric/RuntimeEnvironment.html#ap....
> 
> What were the errors you were getting without it?

I think RuntimeEnvironment.application is just how you get the application
Context within a Robolectric test.

The error I get without this line is....

[ RUN      ]
org.chromium.components.variations.firstrun.VariationsSeedFetcherTest.testFetchSeed_badResponse
java.lang.ExceptionInInitializerError
	at org.chromium.base.ContextUtils.getAppSharedPreferences(ContextUtils.java:91)
	at
org.chromium.components.variations.firstrun.VariationsSeedFetcherTest.setUp(VariationsSeedFetcherTest.java:57)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:52)
	at
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at
org.robolectric.RobolectricTestRunner$2.evaluate(RobolectricTestRunner.java:316)
	at
org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:236)
	at
org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:41)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at
org.robolectric.RobolectricTestRunner$1.evaluate(RobolectricTestRunner.java:176)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at
org.chromium.testing.local.GtestComputer$GtestSuiteRunner.run(GtestComputer.java:46)
	at org.junit.runners.Suite.runChild(Suite.java:128)
	at org.junit.runners.Suite.runChild(Suite.java:27)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
	at org.chromium.testing.local.JunitTestMain.main(JunitTestMain.java:105)
Caused by: java.lang.NullPointerException
	at
android.preference.PreferenceManager.getDefaultSharedPreferencesName(PreferenceManager.java:498)
	at
android.preference.PreferenceManager.getDefaultSharedPreferences(PreferenceManager.java:487)
	at
org.chromium.base.ContextUtils.fetchAppSharedPreferences(ContextUtils.java:80)
	at org.chromium.base.ContextUtils.access$000(ContextUtils.java:19)
	at org.chromium.base.ContextUtils$Holder.<clinit>(ContextUtils.java:28)
	at org.chromium.base.ContextUtils.getAppSharedPreferences(ContextUtils.java:91)
	at
org.chromium.components.variations.firstrun.VariationsSeedFetcherTest.setUp(VariationsSeedFetcherTest.java:57)
	... 27 more

[jwd, 2017-03-09 15:16:32]

LGTM for variations component.

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
File
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/components/variations/...
components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:50:
ContextUtils.initApplicationContextForTests(RuntimeEnvironment.application);
On 2017/03/09 01:50:28, mikecase wrote:
> On 2017/03/08 at 22:44:33, jwd wrote:
> > On 2017/03/07 20:29:41, mikecase wrote:
> > > Adding this fixed some errors when running the test. Unsure if this is
> correct
> > > though. Would appreciate if someone more familiar with the code confirmed
> > 
> > What does this line do? The robolectric doc on
RuntimeEnvironment.application
> doesn't describe it
>
http://robolectric.org/javadoc/3.2/org/robolectric/RuntimeEnvironment.html#ap....
> > 
> > What were the errors you were getting without it?
> 
> I think RuntimeEnvironment.application is just how you get the application
> Context within a Robolectric test.
> 
> The error I get without this line is....
> 
> [ RUN      ]
>
org.chromium.components.variations.firstrun.VariationsSeedFetcherTest.testFetchSeed_badResponse
> java.lang.ExceptionInInitializerError
> 	at
org.chromium.base.ContextUtils.getAppSharedPreferences(ContextUtils.java:91)
> 	at
>
org.chromium.components.variations.firstrun.VariationsSeedFetcherTest.setUp(VariationsSeedFetcherTest.java:57)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> 	at
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> 	at
>
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> 	at java.lang.reflect.Method.invoke(Method.java:498)
> 	at
>
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
> 	at
>
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
> 	at
>
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:52)
> 	at
> org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
> 	at
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
> 	at
>
org.robolectric.RobolectricTestRunner$2.evaluate(RobolectricTestRunner.java:316)
> 	at
> org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:236)
> 	at
> org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:41)
> 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
> 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
> 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
> 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
> 	at
>
org.robolectric.RobolectricTestRunner$1.evaluate(RobolectricTestRunner.java:176)
> 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
> 	at
>
org.chromium.testing.local.GtestComputer$GtestSuiteRunner.run(GtestComputer.java:46)
> 	at org.junit.runners.Suite.runChild(Suite.java:128)
> 	at org.junit.runners.Suite.runChild(Suite.java:27)
> 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
> 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
> 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
> 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
> 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
> 	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
> 	at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
> 	at org.chromium.testing.local.JunitTestMain.main(JunitTestMain.java:105)
> Caused by: java.lang.NullPointerException
> 	at
>
android.preference.PreferenceManager.getDefaultSharedPreferencesName(PreferenceManager.java:498)
> 	at
>
android.preference.PreferenceManager.getDefaultSharedPreferences(PreferenceManager.java:487)
> 	at
> org.chromium.base.ContextUtils.fetchAppSharedPreferences(ContextUtils.java:80)
> 	at org.chromium.base.ContextUtils.access$000(ContextUtils.java:19)
> 	at org.chromium.base.ContextUtils$Holder.<clinit>(ContextUtils.java:28)
> 	at
org.chromium.base.ContextUtils.getAppSharedPreferences(ContextUtils.java:91)
> 	at
>
org.chromium.components.variations.firstrun.VariationsSeedFetcherTest.setUp(VariationsSeedFetcherTest.java:57)
> 	... 27 more

Ok, seems fine to add it. The test shouldn't be affected. I don't understand
what's changed between 3.0 and 3.2 that broke this though.

[mikecase (-- gone --), 2017-03-13 18:07:57]

friendly ping. Would be great if I could land this later this week  :D

[jbudorick, 2017-03-13 22:36:54]

I'm still concerned about what happens to the outdated JARs. Do we warn about
them? Do we let them sit there silently on users' machines, invisible because of
the *.jar lines in //third_party/.gitignore?

https://codereview.chromium.org/2710343003/diff/340001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:50:
@Config(manifest = Config.NONE, shadows = {CustomShadowAsyncTask.class}, sdk =
21)
super-nit: You have sdk before shadows in some cases and after it in others.
Consistency (maybe alphabetizing?) would be nice.

https://codereview.chromium.org/2710343003/diff/340001/testing/android/junit/...
File
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java:12:
// TODO(mikecase): Add support for specifying the AndroidManifest for
Do you have an idea as to when and how you'll do this?

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

https://codereview.chromium.org/2710343003/diff/340001/third_party/robolectri...
third_party/robolectric/BUILD.gn:27: deps = [
If these are run-time deps (and not build-time deps), they should be data_deps
(though I don't know whether java_prebuilt will handle that correctly).

https://codereview.chromium.org/2710343003/diff/340001/third_party/robolectri...
third_party/robolectric/BUILD.gn:61: ":robolectric_runtime_deps",
same

[mikecase (-- gone --), 2017-03-14 18:08:39]

Added explicit names of the jars to the third_party/.gitignore so that now
unused jars don't just hang out forever.

https://codereview.chromium.org/2710343003/diff/340001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java:50:
@Config(manifest = Config.NONE, shadows = {CustomShadowAsyncTask.class}, sdk =
21)
On 2017/03/13 at 22:36:54, jbudorick wrote:
> super-nit: You have sdk before shadows in some cases and after it in others.
Consistency (maybe alphabetizing?) would be nice.

Done

https://codereview.chromium.org/2710343003/diff/340001/testing/android/junit/...
File
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java:12:
// TODO(mikecase): Add support for specifying the AndroidManifest for
On 2017/03/13 at 22:36:54, jbudorick wrote:
> Do you have an idea as to when and how you'll do this?

No and no. It will be pretty difficult and no-one has requested this to me. The
purpose of this class is to currently allow packageName variable to be
overwritten. For some weird reason, in the default (MavenManifestFactory), if
you don't specify the AndroidManifest you cannot override the packageName
variable (just because of the way the logic is structured)

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

https://codereview.chromium.org/2710343003/diff/340001/third_party/robolectri...
third_party/robolectric/BUILD.gn:27: deps = [
On 2017/03/13 at 22:36:54, jbudorick wrote:
> If these are run-time deps (and not build-time deps), they should be data_deps
(though I don't know whether java_prebuilt will handle that correctly).

Done, got rid of this group and just added these under robolectric_java
data_deps

[nyquist, 2017-03-14 21:20:37]

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java
(left):

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java:28:
builder.addInstrumentedPackage("com.google.android.gms.common");
Where did these go? Are they not required anymore?

I know we discussed this on chat that GMS is supposed to be always shadowed from
now on, but for future reference it might be helpful to document as a comment in
the review why they are not needed anymore (i.e. not in the code, but here on
rietveld).

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:66:
public static class MyShadowContentProvider extends ShadowContentProvider {
It seems like we are calling getContext() a few places in
//chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
:

- getContext().getApplicationContext()
- getContext().getSystemService(...)
- getContext().getPackageName()
- getContext().registerReceiver(...);
 
Do you know which one of those do not work with the context that's usually
retrieved?

Unsure how much time you want to spend on that, but maybe you could create a
class that wraps the RuntimeEnvironment.application?
 You can use this class and pass in RuntimeEnvironment.application to the
constructor:
https://developer.android.com/reference/android/content/ContextWrapper.html

At that point you can override the methods you are interested in, and see
if/when the test doesn't work based on what you return?

[mikecase (-- gone --), 2017-03-14 22:07:08]

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java
(left):

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java:28:
builder.addInstrumentedPackage("com.google.android.gms.common");
On 2017/03/14 at 21:20:37, nyquist wrote:
> Where did these go? Are they not required anymore?
> 
> I know we discussed this on chat that GMS is supposed to be always shadowed
from now on, but for future reference it might be helpful to document as a
comment in the review why they are not needed anymore (i.e. not in the code, but
here on rietveld).

Ever since this commit...

https://github.com/robolectric/robolectric/commit/d6cef1bcc644c7152d0716b8d33...

all shadow classes specified in @Config(shadow=MyShadow.class) are automatically
instrumented.

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:66:
public static class MyShadowContentProvider extends ShadowContentProvider {
So for some info, in this commit they removed getContext() from the default
ShadowContentProvider...

https://github.com/robolectric/robolectric/commit/80d495d66e5207dd0c35bc06799...



Without the custom ShadowContentProvider, I think that getContext() just returns
null.
Here is the error using the default ShadowContentProvider...


java.lang.NullPointerException
	at
org.chromium.chrome.browser.superviseduser.SupervisedUserContentProvider.getSupervisedUserContentProvider(SupervisedUserContentProvider.java:72)
	at
org.chromium.chrome.browser.superviseduser.SupervisedUserContentProvider.shouldProceed(SupervisedUserContentProvider.java:177)
	at
org.chromium.chrome.browser.superviseduser.SupervisedUserContentProviderUnitTest.testShouldProceed_withStartupSignedIn(SupervisedUserContentProviderUnitTest.java:213)

[nyquist, 2017-03-14 22:17:30]

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java
(right):

https://codereview.chromium.org/2710343003/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java:66:
public static class MyShadowContentProvider extends ShadowContentProvider {
On 2017/03/14 22:07:07, mikecase wrote:
> So for some info, in this commit they removed getContext() from the default
> ShadowContentProvider...
> 
>
https://github.com/robolectric/robolectric/commit/80d495d66e5207dd0c35bc06799...
> 
> 
> 
> Without the custom ShadowContentProvider, I think that getContext() just
returns
> null.
> Here is the error using the default ShadowContentProvider...
> 
> 
> java.lang.NullPointerException
> 	at
>
org.chromium.chrome.browser.superviseduser.SupervisedUserContentProvider.getSupervisedUserContentProvider(SupervisedUserContentProvider.java:72)
> 	at
>
org.chromium.chrome.browser.superviseduser.SupervisedUserContentProvider.shouldProceed(SupervisedUserContentProvider.java:177)
> 	at
>
org.chromium.chrome.browser.superviseduser.SupervisedUserContentProviderUnitTest.testShouldProceed_withStartupSignedIn(SupervisedUserContentProviderUnitTest.java:213)

Right; but it seems like they are attaching the context through ProviderInfo
here:
https://github.com/robolectric/robolectric/commit/c3bbc6fc4176581fb1ac2ee28de...
(That commit was linked to from the other commit)

So could there be that there is something we need to do to ensure that content
providers are correctly set up using robolectric?

For example maybe we need to register our content provider?

ProviderInfo info = new ProviderInfo();
info.authority = "INSERT_OUR_MANIFEST_PACKAGE_HERE.SupervisedUserProvider";
Robolectric.buildContentProvider(SupervisedUserContentProvider.class).create(info);

[mikecase (-- gone --), 2017-03-15 01:17:46]

Most recent two patch sets have two different fixes for
SupervisedUserContentProviderUnittest.


First is just attaching the app context the to content provider initialized in
the setUp() method.


        ProviderInfo info = new ProviderInfo();
        info.authority = "foo.bar.baz";
        mSupervisedUserContentProvider.attachInfo(
                RuntimeEnvironment.application.getBaseContext(), info);



Second way (which I think is the better one) is have Robolectric initialize the
content provider via Robolectric.buildContentProvider.

[nyquist, 2017-03-15 14:40:19]

Yeah, according to
http://robolectric.blogspot.com/2016/10/robolectric-313-is-released.html :
"ContentProviders should be created using Robolectric.buildContentProvider(),
which simulates how the framework creates them (and calls onCreate()
automatically)."

So your second approach lgtm! Thanks for sticking with me here :-)

https://codereview.chromium.org/2710343003/diff/420001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java
(left):

https://codereview.chromium.org/2710343003/diff/420001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java:30:
builder.addInstrumentedClass("org.chromium.chrome.browser.offlinepages.DeviceConditions");
Could you comment in this review why this class is not needed to be instrumented
anymore?
Was it because in the test, the @Config attribute contained:
@Config(...,  shadows = {..., ShadowDeviceConditions.class})
and these are now automatically instrumented?

[mikecase (-- gone --), 2017-03-15 15:35:16]

https://codereview.chromium.org/2710343003/diff/420001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java
(left):

https://codereview.chromium.org/2710343003/diff/420001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTestRunner.java:30:
builder.addInstrumentedClass("org.chromium.chrome.browser.offlinepages.DeviceConditions");
On 2017/03/15 at 14:40:19, nyquist wrote:
> Could you comment in this review why this class is not needed to be
instrumented anymore?
> Was it because in the test, the @Config attribute contained:
> @Config(...,  shadows = {..., ShadowDeviceConditions.class})
> and these are now automatically instrumented?

Yeah you basically said it. So if I understand correctly, Robolectric will only
try loading Shadow classes for certain packages/classes that it instruments.

Looks like ever since
https://github.com/robolectric/robolectric/commit/d6cef1bcc644c7152d0716b8d33...
, all classes that are passed through @Config(...,  shadows = {...,
MyShadow.class}) will automatically get instrumented by Robolectric. Previously,
you add to override the createClassLoaderConfig() method like above.

Much nicer and cleaner now :D

[jbudorick, 2017-03-15 15:58:58]

lgtm

We should revisit the mechanism for updating those JARs, though. I'm wondering
if we can/should attempt to automatically remove the old ones.

https://codereview.chromium.org/2710343003/diff/340001/testing/android/junit/...
File
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java
(right):

https://codereview.chromium.org/2710343003/diff/340001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java:12:
// TODO(mikecase): Add support for specifying the AndroidManifest for
On 2017/03/14 18:08:39, mikecase wrote:
> On 2017/03/13 at 22:36:54, jbudorick wrote:
> > Do you have an idea as to when and how you'll do this?
> 
> No and no. It will be pretty difficult and no-one has requested this to me.
The
> purpose of this class is to currently allow packageName variable to be
> overwritten. For some weird reason, in the default (MavenManifestFactory), if
> you don't specify the AndroidManifest you cannot override the packageName
> variable (just because of the way the logic is structured)

Acknowledged.

[mikecase (-- gone --), 2017-03-15 22:41:48]

Seems like some newly added tests failed on Robolectric 3.2.2

Fixed errors in BackgroundOfflinerTaskTest.java by explicitly setting sdk=21.

There were also errors in AndroidPaymentAppFinderTest caused by the fact
ShadowIntent used to override .equals(). It no longer does, and the Mockito arg
matcher no longer correctly matched Intent arguments.


Please take a look at AndroidPaymentAppFinderTest.java. If it looks good, Ill go
ahead and commit.

[nyquist, 2017-03-16 15:29:51]

lgtm % nits!

https://codereview.chromium.org/2710343003/diff/440001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java
(right):

https://codereview.chromium.org/2710343003/diff/440001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:46:
private Intent mIntent;
Nit: final

https://codereview.chromium.org/2710343003/diff/440001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:49:
this.mIntent = intent;
Nit: Remove |this|

[mikecase (-- gone --), 2017-03-16 15:41:36]

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

[mikecase (-- gone --), 2017-03-16 15:41:38]

+ thakis@chromium.org, need lgtm from OWNER of third_party/.gitignore :D

[Nico, 2017-03-16 15:46:17]

From what I can tell, the .jar files you're adding to .gitignore are in fact
checked in to git and not DEPS'd in -- why do you want to list them in
.gitignore?

Also, should the jar files be pulled via
src/build/android/update_deps/update_third_party_deps.py instead of checking
them in? (But given the list of people lgtm'ing this it's very likely fine as-is
-- feel free to ignore this paragraph or use it to educate me on what
update_third_party_deps.py is for :-) )

[jbudorick, 2017-03-16 15:47:01]

On 2017/03/16 15:41:38, mikecase wrote:
> + mailto:thakis@chromium.org, need lgtm from OWNER of third_party/.gitignore
:D

related question: //.gitignore OWNERS is *; should it be the same here?

[mikecase (-- gone --), 2017-03-16 15:58:02]

On 2017/03/16 at 15:46:17, thakis wrote:
> From what I can tell, the .jar files you're adding to .gitignore are in fact
checked in to git and not DEPS'd in -- why do you want to list them in
.gitignore?
> 
> Also, should the jar files be pulled via
src/build/android/update_deps/update_third_party_deps.py instead of checking
them in? (But given the list of people lgtm'ing this it's very likely fine as-is
-- feel free to ignore this paragraph or use it to educate me on what
update_third_party_deps.py is for :-) )


So, the jars are *not* checked in, and should be downloaded via
update_third_party_deps.py. The update_third_party_deps script looks for all
*.sha1 files in a directory, and downloads the corresponding jar during "gclient
runhooks".

Since these jars are not checked in, we need to add them to .gitignore.
Previously, I added *.jar to .gitignore. However, this means that some
unused/outdated JARs get hidden and will never be cleaned up.

[Nico, 2017-03-16 16:06:32]

lgtm

Sorry, I missed that you're adding .jar.sha1 files, not .jar files. All good
then.

Making .gitignore *-owned sounds fine to me.

[mikecase (-- gone --), 2017-03-16 16:30:40]

https://codereview.chromium.org/2710343003/diff/440001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java
(right):

https://codereview.chromium.org/2710343003/diff/440001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:46:
private Intent mIntent;
On 2017/03/16 at 15:29:50, nyquist wrote:
> Nit: final

Done

https://codereview.chromium.org/2710343003/diff/440001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java:49:
this.mIntent = intent;
On 2017/03/16 at 15:29:51, nyquist wrote:
> Nit: Remove |this|

Done.

Also added @Override to matches() and equals() method that I forgot >_<

[mikecase (-- gone --), 2017-03-16 16:30:42]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-03-16 16:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org,
jbudorick@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2710343003/#ps460001
(title: "Nits")

[commit-bot: I haz the power, 2017-03-16 16:31:28]

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, 2017-03-16 17:52:15]

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

[commit-bot: I haz the power, 2017-03-16 17:52:17]

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...)

[mikecase (-- gone --), 2017-03-16 20:24:56]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-03-16 20:24:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, nyquist@chromium.org, thakis@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2710343003/#ps480001
(title: "Small fix to StripLayoutHelperTest")

[commit-bot: I haz the power, 2017-03-16 20:25:42]

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, 2017-03-16 21:30:11]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1489695896991770,
"parent_rev": "93b10af49dc4f680debc7fa2274bd07a55c6492f", "commit_rev":
"6a5f508e65cf947c6292fe8578868d0fe739f0aa"}

[commit-bot: I haz the power, 2017-03-16 21:30:59]

Description was changed from

==========
Update Robolectric to 3.2.2

BUG=693659
==========

to

==========
Update Robolectric to 3.2.2

BUG=693659

Review-Url: https://codereview.chromium.org/2710343003
Cr-Commit-Position: refs/heads/master@{#457560}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a5f508e65cf947c6292fe857886...
==========

[commit-bot: I haz the power, 2017-03-16 21:31:02]

Committed patchset #25 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/6a5f508e65cf947c6292fe857886...