Expose resources in Robolectric/JUnit tests.

Expose resources in Robolectric/JUnit tests.

BUG=693573
Review-Url: https://codereview.chromium.org/2767613002
Cr-Commit-Position: refs/heads/master@{#464422}
Committed: https://chromium.googlesource.com/chromium/src/+/0d81d90a4b60f8dbcf75f92405cb441db3d08cd3

[mikecase (-- gone --), 2017-03-21 17:51:20]

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

[mikecase (-- gone --), 2017-03-21 17:51:21]

This CL is 90% plumbing and 10% actual stuff.

Basically, just need a way to pipe paths such as AndroidManifest.xml path,
resource directory, and robolectric runtime deps dir, from GN all the way to
Robolectric Java.

This CL lets you specify...

package_name
resource_dir
manifest_path

...in junit_binary targets. Unfortunately, to have Robolectric actually be able
to find resources correctly, it seems like you need to specify BOTH resource_dir
and package_name. See chrome/android/BUILD.gn and TileTest.java for example.

As for manifest, I added the ability to specify it in the GN code. I tried
pointing Robolectric to the chrome AndroidManifest.xml and I got a bunch of
errors. So will be investigating that and maybe committing another CL.

[mikecase (-- gone --), 2017-03-21 17:52:35]

https://codereview.chromium.org/2767613002/diff/40001/testing/android/OWNERS
File testing/android/OWNERS (right):

https://codereview.chromium.org/2767613002/diff/40001/testing/android/OWNERS#...
testing/android/OWNERS:4: mikecase@chromium.org
also, sneaking my name in here if that is ok

[dgn, 2017-03-21 18:16:42]

So if I understand correctly, running simply "test_runner.py junit -s
chrome_junit_tests" won't work and we will have to use
out/Debug/bin/run_chrome_junit_tests or something like that?

https://codereview.chromium.org/2767613002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java
(right):

https://codereview.chromium.org/2767613002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:412:
public static class ShadowLayoutInflater {
does it work when this one is removed too? AFAICT it's there for the same reason
as the shadow above.

[mikecase (-- gone --), 2017-03-21 18:39:36]

Yeah, with this change you would have to run the test with
out/Debug/bin/run_chrome_junit_tests

https://codereview.chromium.org/2767613002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java
(right):

https://codereview.chromium.org/2767613002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:412:
public static class ShadowLayoutInflater {
On 2017/03/21 at 18:16:41, dgn wrote:
> does it work when this one is removed too? AFAICT it's there for the same
reason as the shadow above.

I get an error when I remove this....


Caused by: java.lang.RuntimeException: huh?
ResName{org.chromium.chrome:string/accessibility_ntp_offline_badge}
	at
org.robolectric.shadows.ShadowAssetManager.convertAndFill(ShadowAssetManager.java:157)
	at
org.robolectric.shadows.ShadowAssetManager.buildTypedValue(ShadowAssetManager.java:771)
	at
org.robolectric.shadows.ShadowAssetManager.attrsToTypedArray(ShadowAssetManager.java:797)
	at
org.robolectric.shadows.ShadowResources$ShadowTheme.obtainStyledAttributes(ShadowResources.java:221)
	at android.content.res.Resources$Theme.obtainStyledAttributes(Resources.java)
	at android.content.Context.obtainStyledAttributes(Context.java:472)
	at android.view.View.__constructor__(View.java:3677)
	at android.view.View.<init>(View.java)
	at android.widget.ImageView.<init>(ImageView.java)
	at android.support.v7.widget.AppCompatImageView.<init>(AppCompatImageView.java)
	at
org.chromium.chrome.browser.widget.TintedImageView.<init>(TintedImageView.java:28)
	at android.view.LayoutInflater.createView(LayoutInflater.java:607)
	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:743)
	at android.view.LayoutInflater.rInflate(LayoutInflater.java:806)
	at android.view.LayoutInflater.inflate(LayoutInflater.java:504)
	at android.view.LayoutInflater.inflate(LayoutInflater.java:414)
	at
org.chromium.chrome.browser.suggestions.TileGroup.buildTileView(TileGroup.java:325)
	at
org.chromium.chrome.browser.suggestions.TileGroupTest.testIconLoadingWhenTileNotRegistered(TileGroupTest.java:342)


https://github.com/robolectric/robolectric/blob/055f4112eae8a2b865278f3e73e52...

Apparently, Robolectric is surprised to get a string resource here?

[dgn, 2017-03-22 10:23:47]

https://codereview.chromium.org/2767613002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java
(right):

https://codereview.chromium.org/2767613002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/suggestions/TileGroupTest.java:412:
public static class ShadowLayoutInflater {
On 2017/03/21 18:39:36, mikecase wrote:
> On 2017/03/21 at 18:16:41, dgn wrote:
> > does it work when this one is removed too? AFAICT it's there for the same
> reason as the shadow above.
> 
> I get an error when I remove this....
> 
> 
> Caused by: java.lang.RuntimeException: huh?
> ResName{org.chromium.chrome:string/accessibility_ntp_offline_badge}
> 	at
>
org.robolectric.shadows.ShadowAssetManager.convertAndFill(ShadowAssetManager.java:157)
> 	at
>
org.robolectric.shadows.ShadowAssetManager.buildTypedValue(ShadowAssetManager.java:771)
> 	at
>
org.robolectric.shadows.ShadowAssetManager.attrsToTypedArray(ShadowAssetManager.java:797)
> 	at
>
org.robolectric.shadows.ShadowResources$ShadowTheme.obtainStyledAttributes(ShadowResources.java:221)
> 	at android.content.res.Resources$Theme.obtainStyledAttributes(Resources.java)
> 	at android.content.Context.obtainStyledAttributes(Context.java:472)
> 	at android.view.View.__constructor__(View.java:3677)
> 	at android.view.View.<init>(View.java)
> 	at android.widget.ImageView.<init>(ImageView.java)
> 	at
android.support.v7.widget.AppCompatImageView.<init>(AppCompatImageView.java)
> 	at
>
org.chromium.chrome.browser.widget.TintedImageView.<init>(TintedImageView.java:28)
> 	at android.view.LayoutInflater.createView(LayoutInflater.java:607)
> 	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:743)
> 	at android.view.LayoutInflater.rInflate(LayoutInflater.java:806)
> 	at android.view.LayoutInflater.inflate(LayoutInflater.java:504)
> 	at android.view.LayoutInflater.inflate(LayoutInflater.java:414)
> 	at
>
org.chromium.chrome.browser.suggestions.TileGroup.buildTileView(TileGroup.java:325)
> 	at
>
org.chromium.chrome.browser.suggestions.TileGroupTest.testIconLoadingWhenTileNotRegistered(TileGroupTest.java:342)
> 
> 
>
https://github.com/robolectric/robolectric/blob/055f4112eae8a2b865278f3e73e52...
> 
> Apparently, Robolectric is surprised to get a string resource here?

That's a string used a content description for accessibility purposes, and
defined here:
https://cs.chromium.org/chromium/src/chrome/android/java/strings/android_chro...

According to the source and the stack above, the resource was registered (it
could resolve the resource name from the id), but it failed getting the value.
String themselves seem fine as in the case where a resource value is found there
is some string specific processing.

So... any idea why getting the value from the resource table would fail?

[mikecase (-- gone --), 2017-03-29 00:08:31]

ok, turns out string resources do not live with the rest of the resources. So
that is why Robolectric could not find that resource. I manually moved the
string resources into the main res/ folder and was able to delete the other
shadow and have all of the tests pass.

Not really sure best way to fix this since Robolectric does not support multiple
resource folders. There is a hacky workaround, or I can make some changes to
Robolectric source to support what I need.

[mikecase (-- gone --), 2017-03-29 18:30:31]

Updated to be able to specify multiple resource paths. If you specify the
resource paths as... 

resource_dirs = [
  "//chrome/android/java/res/",
  "$root_gen_dir/chrome/android/chrome_strings_grd_grit_output/",
]

...then all of the tests pass in the TileTest without either of the custom
shadows.

PTAL

[jbudorick, 2017-03-29 18:32:28]

On 2017/03/29 18:30:31, mikecase wrote:
> Updated to be able to specify multiple resource paths. If you specify the
> resource paths as... 
> 
> resource_dirs = [
>   "//chrome/android/java/res/",
>   "$root_gen_dir/chrome/android/chrome_strings_grd_grit_output/",
> ]
> 
> ...then all of the tests pass in the TileTest without either of the custom
> shadows.
> 
> PTAL

I'll get to this tomorrow! Sorry about the delay :|

[jbudorick, 2017-03-30 19:11:44]

https://codereview.chromium.org/2767613002/diff/60001/build/android/gyp/creat...
File build/android/gyp/create_test_runner_script.py (right):

https://codereview.chromium.org/2767613002/diff/60001/build/android/gyp/creat...
build/android/gyp/create_test_runner_script.py:152: if
args.robolectric_runtime_deps_dir:
Separate CL idea: there has to be a way to do this that doesn't involve the same
four lines of code for every option.

https://codereview.chromium.org/2767613002/diff/60001/build/android/pylib/loc...
File build/android/pylib/local/machine/local_machine_junit_test_run.py (left):

https://codereview.chromium.org/2767613002/diff/60001/build/android/pylib/loc...
build/android/pylib/local/machine/local_machine_junit_test_run.py:50: #
TODO(mikecase): Add a --robolectric-dep-dir arg to test runner.
Nice.

https://codereview.chromium.org/2767613002/diff/60001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2767613002/diff/60001/build/android/test_runn...
build/android/test_runner.py:480: '--resource-dir', action='append',
nit: drop action onto the line below. The flags should be the only thing on the
first line.

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

https://codereview.chromium.org/2767613002/diff/60001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:375: resource_dirs = [
Is there a way we can get this from the resources targets on which this target
depends? (build_config?)

https://codereview.chromium.org/2767613002/diff/60001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:376: "//chrome/android/java/res/",
Should this also include res_chromium?

[jbudorick, 2017-03-30 19:12:26]

https://codereview.chromium.org/2767613002/diff/40001/testing/android/OWNERS
File testing/android/OWNERS (right):

https://codereview.chromium.org/2767613002/diff/40001/testing/android/OWNERS#...
testing/android/OWNERS:4: mikecase@chromium.org
On 2017/03/21 17:52:35, mikecase wrote:
> also, sneaking my name in here if that is ok

Also! This is not exactly "sneaking" if you call it out :P

(but yes, I am fine with it)

[mikecase (-- gone --), 2017-03-31 17:35:07]

Welp, seems like the bots don't use the runner scripts (e.g.
out/bin/run_chrome_junit_tests) to run JUnit tests. So I won't be able to commit
this CL till I change them to do that.

https://codereview.chromium.org/2767613002/diff/60001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/2767613002/diff/60001/build/android/test_runn...
build/android/test_runner.py:480: '--resource-dir', action='append',
On 2017/03/30 at 19:11:44, jbudorick wrote:
> nit: drop action onto the line below. The flags should be the only thing on
the first line.

Done

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

https://codereview.chromium.org/2767613002/diff/60001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:375: resource_dirs = [
On 2017/03/30 at 19:11:44, jbudorick wrote:
> Is there a way we can get this from the resources targets on which this target
depends? (build_config?)

Really unsure. I took a look through the build config files and it didnt look
like anything currently in the build config files would help out here.

But yeah, the current solution of listing out all directories that have relevant
resources by hand isnt great...

https://codereview.chromium.org/2767613002/diff/60001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:376: "//chrome/android/java/res/",
On 2017/03/30 at 19:11:44, jbudorick wrote:
> Should this also include res_chromium?

Probably? Done

[mikecase (-- gone --), 2017-04-03 17:14:19]

Now that the bots use the bin/run_junit_test wrapper scripts to run tests, all
the junit tests are passing on the trybots.

[jbudorick, 2017-04-03 19:43:25]

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

https://codereview.chromium.org/2767613002/diff/60001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:375: resource_dirs = [
On 2017/03/31 17:35:07, mikecase wrote:
> On 2017/03/30 at 19:11:44, jbudorick wrote:
> > Is there a way we can get this from the resources targets on which this
target
> depends? (build_config?)
> 
> Really unsure. I took a look through the build config files and it didnt look
> like anything currently in the build config files would help out here.
> 
> But yeah, the current solution of listing out all directories that have
relevant
> resources by hand isnt great...

As mentioned offline: the build_config tracks resource directories, so we really
should be able to do it automatically.

[mikecase (-- gone --), 2017-04-10 23:50:35]

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

[mikecase (-- gone --), 2017-04-10 23:50:36]

+ agrieve@ for build config questions/advice

I create a new entry 'test_info' in the build config and add all res_dirs of all
resource_deps to this. To get this to work for strings, I had to make it so
java_strings_grd targets pass resource_dir argument along to write_build_config.
Will have to update lint paths since lint prefers resource_dir over
resource_zip.

Additionally, it seems that Robolectric allows you to pass multiple
resource_dirs, but it maps them all to the app's package name? (still a bit
confused about this tbh)

https://github.com/robolectric/robolectric/blob/42fd1be010f01d88129591805fd2f...

Im might have to look deeper, but it seems that passing the app's package name
and a list of all the resource_dirs works best. So, for resources to work in
Robolectric tests, you have to specify package_name GN variable, but that is
all.

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

https://codereview.chromium.org/2767613002/diff/100001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:386: ]
How does this look? You now dont have to specify the resource_dirs (which is
hacky) and instead just have to list off relevant android_resource targets.

https://codereview.chromium.org/2767613002/diff/120001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2767613002/diff/120001/build/config/android/i...
build/config/android/internal_rules.gni:587:
"${target_gen_dir}/${invoker.test_suite}.build_config"
@agrieve,

I need to specify the build config of the parent junit_binary target. I can
construct it like so, but it seems a little bit of a hack.

https://codereview.chromium.org/2767613002/diff/120001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2767613002/diff/120001/build/config/android/r...
build/config/android/rules.gni:900: resource_dirs = [ grit_output_dir ]
@agrieve,

I looked into this a bit and am almost certain this is fine to do. But to your
knowledge, should it be ok for the java_strings_grd and
java_strings_grd_prebuilt to list off resource_dir in their build config.

It seems this pretty much just effects linting.

[agrieve, 2017-04-11 00:24:23]

On 2017/04/10 23:50:36, mikecase wrote:
> + agrieve@ for build config questions/advice
> 
> I create a new entry 'test_info' in the build config and add all res_dirs of
all
> resource_deps to this. To get this to work for strings, I had to make it so
> java_strings_grd targets pass resource_dir argument along to
write_build_config.
> Will have to update lint paths since lint prefers resource_dir over
> resource_zip.
> 
> Additionally, it seems that Robolectric allows you to pass multiple
> resource_dirs, but it maps them all to the app's package name? (still a bit
> confused about this tbh)
> 
>
https://github.com/robolectric/robolectric/blob/42fd1be010f01d88129591805fd2f...
> 
> Im might have to look deeper, but it seems that passing the app's package name
> and a list of all the resource_dirs works best. So, for resources to work in
> Robolectric tests, you have to specify package_name GN variable, but that is
> all.
> 
> https://codereview.chromium.org/2767613002/diff/100001/chrome/android/BUILD.gn
> File chrome/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2767613002/diff/100001/chrome/android/BUILD.g...
> chrome/android/BUILD.gn:386: ]
> How does this look? You now dont have to specify the resource_dirs (which is
> hacky) and instead just have to list off relevant android_resource targets.
> 
>
https://codereview.chromium.org/2767613002/diff/120001/build/config/android/i...
> File build/config/android/internal_rules.gni (right):
> 
>
https://codereview.chromium.org/2767613002/diff/120001/build/config/android/i...
> build/config/android/internal_rules.gni:587:
> "${target_gen_dir}/${invoker.test_suite}.build_config"
> @agrieve,
> 
> I need to specify the build config of the parent junit_binary target. I can
> construct it like so, but it seems a little bit of a hack.
> 
>
https://codereview.chromium.org/2767613002/diff/120001/build/config/android/r...
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/2767613002/diff/120001/build/config/android/r...
> build/config/android/rules.gni:900: resource_dirs = [ grit_output_dir ]
> @agrieve,
> 
> I looked into this a bit and am almost certain this is fine to do. But to your
> knowledge, should it be ok for the java_strings_grd and
> java_strings_grd_prebuilt to list off resource_dir in their build config.
> 
> It seems this pretty much just effects linting.

Package names are used for 2 things in android resources:
1. To generate R classes
2. Each resources.arsc file is marked as being owned by a single package name.

It sounds like maybe robolectric is using the package name only for #2, in which
case it makes sense to just pass the manifest's package id.

[agrieve, 2017-04-11 00:25:06]

I think the solution for the grit resources is to just extract the resource
.zips either as on start-up of the test (from .java), or before running the test
(from python wrapper).

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

https://codereview.chromium.org/2767613002/diff/100001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:386: ]
On 2017/04/10 23:50:36, mikecase wrote:
> How does this look? You now dont have to specify the resource_dirs (which is
> hacky) and instead just have to list off relevant android_resource targets.

Looks like you've gotten rid of this all together now, which is even better :)

https://codereview.chromium.org/2767613002/diff/120001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2767613002/diff/120001/build/config/android/i...
build/config/android/internal_rules.gni:587:
"${target_gen_dir}/${invoker.test_suite}.build_config"
On 2017/04/10 23:50:36, mikecase wrote:
> @agrieve,
> 
> I need to specify the build config of the parent junit_binary target. I can
> construct it like so, but it seems a little bit of a hack.

I can't think of a better way.

https://codereview.chromium.org/2767613002/diff/140001/build/android/gyp/writ...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/2767613002/diff/140001/build/android/gyp/writ...
build/android/gyp/write_build_config.py:546:
config['test_info']['resources_dirs'] = [
This hurts my head to look at :P. Better to just use a for loop.

https://codereview.chromium.org/2767613002/diff/140001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2767613002/diff/140001/build/config/android/r...
build/config/android/rules.gni:898:
write_build_config("${target_name}__build_config") {
The main reason we use .zip files rather than directories is for when outputs
are removed or renamed.

E.g. if at one point this produced:
$grit_output_dir/res/values/foo.xml

then someone changes it so that it produces:
$grit_output_dir/res/values/bar.xml

Then grit_output_dir will look like:
$grit_output_dir/res/values/foo.xml
$grit_output_dir/res/values/bar.xml

Until a clobber build is performed.

So... I think this is probably better left as a .zip. Most resource commands
don't even leave their pre-zipped outputs around (and this one arguably
shouldn't either)

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

https://codereview.chromium.org/2767613002/diff/140001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java:44:
final LinkedList<FsFile> resourceDirsList = new LinkedList<FsFile>();
nit: Why LinkedList? Looks like an ArrayList would be fine here.

[mikecase (-- gone --), 2017-04-11 18:02:52]

Change CL to use resource zipfiles. Much nicer now :D

Also, extracting all of the zips is way faster than I thought it would be
(basically instant). PTAL

[mikecase (-- gone --), 2017-04-11 18:04:31]

https://codereview.chromium.org/2767613002/diff/140001/build/android/gyp/writ...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/2767613002/diff/140001/build/android/gyp/writ...
build/android/gyp/write_build_config.py:546:
config['test_info']['resources_dirs'] = [
On 2017/04/11 at 00:25:03, agrieve wrote:
> This hurts my head to look at :P. Better to just use a for loop.

Done

https://codereview.chromium.org/2767613002/diff/140001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2767613002/diff/140001/build/config/android/r...
build/config/android/rules.gni:898:
write_build_config("${target_name}__build_config") {
On 2017/04/11 at 00:25:03, agrieve wrote:
> The main reason we use .zip files rather than directories is for when outputs
are removed or renamed.
> 
> E.g. if at one point this produced:
> $grit_output_dir/res/values/foo.xml
> 
> then someone changes it so that it produces:
> $grit_output_dir/res/values/bar.xml
> 
> Then grit_output_dir will look like:
> $grit_output_dir/res/values/foo.xml
> $grit_output_dir/res/values/bar.xml
> 
> Until a clobber build is performed.
> 
> So... I think this is probably better left as a .zip. Most resource commands
don't even leave their pre-zipped outputs around (and this one arguably
shouldn't either)

ahhh, I see, fixed to use zipfiles.

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

https://codereview.chromium.org/2767613002/diff/140001/testing/android/junit/...
testing/android/junit/java/src/org/chromium/testing/local/GNManifestFactory.java:44:
final LinkedList<FsFile> resourceDirsList = new LinkedList<FsFile>();
On 2017/04/11 at 00:25:04, agrieve wrote:
> nit: Why LinkedList? Looks like an ArrayList would be fine here.

Will fix in next patch. Missed this.

[agrieve, 2017-04-11 19:52:31]

lgtm!

[jbudorick, 2017-04-11 19:55:05]

lgtm w/ q

https://codereview.chromium.org/2767613002/diff/200001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2767613002/diff/200001/build/config/android/i...
build/config/android/internal_rules.gni:585: deps += [
":${invoker.test_suite}__build_config" ]
Do these 8 lines need to be in the invoker.package_name block?

[mikecase (-- gone --), 2017-04-11 21:30:53]

https://codereview.chromium.org/2767613002/diff/200001/build/config/android/i...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/2767613002/diff/200001/build/config/android/i...
build/config/android/internal_rules.gni:585: deps += [
":${invoker.test_suite}__build_config" ]
On 2017/04/11 at 19:55:05, jbudorick wrote:
> Do these 8 lines need to be in the invoker.package_name block?

So, technically no. But the way things are currently setup, robolectric will
never find the resources if the package name is not specified because it will
look for them in the wrong place (it will look at the default package which is
like org.robolectric.default or something which doesnt have an R.java file)).

I may look to improve this in the future, but for the time being, everything
works reasonably in my opinion. You just have to be sure to specify the package
name.

[mikecase (-- gone --), 2017-04-11 22:01:15]

The CQ bit was checked by mikecase@chromium.org

[commit-bot: I haz the power, 2017-04-11 22:02:38]

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-04-11 22:06:21]

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

[commit-bot: I haz the power, 2017-04-11 22:06:22]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mikecase (-- gone --), 2017-04-11 22:38:12]

The CQ bit was checked by mikecase@chromium.org

[mikecase (-- gone --), 2017-04-11 22:38:12]

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

[commit-bot: I haz the power, 2017-04-11 22:39:24]

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-04-11 23:04:07]

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

[commit-bot: I haz the power, 2017-04-11 23:04:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mikecase (-- gone --), 2017-04-11 23:40:47]

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

[mikecase (-- gone --), 2017-04-11 23:40:48]

+nyquist for OWNER review of chrome/android/*

[nyquist, 2017-04-13 03:44:33]

lgtm

[mikecase (-- gone --), 2017-04-13 14:44:19]

The CQ bit was checked by mikecase@chromium.org

[commit-bot: I haz the power, 2017-04-13 14:44:39]

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-04-13 15:53:13]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492094659165380,
"parent_rev": "37301f841b14abb1ab6401d5c86e355e8005988e", "commit_rev":
"0d81d90a4b60f8dbcf75f92405cb441db3d08cd3"}

[commit-bot: I haz the power, 2017-04-13 15:54:11]

Description was changed from

==========
Expose resources in Robolectric/JUnit tests.

BUG=693573
==========

to

==========
Expose resources in Robolectric/JUnit tests.

BUG=693573

Review-Url: https://codereview.chromium.org/2767613002
Cr-Commit-Position: refs/heads/master@{#464422}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d81d90a4b60f8dbcf75f92405cb...
==========

[commit-bot: I haz the power, 2017-04-13 15:54:16]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/0d81d90a4b60f8dbcf75f92405cb...

[mikecase (-- gone --), 2017-04-13 18:02:00]

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

The reason for reverting is: crbug/711372.

[mikecase (-- gone --), 2017-04-13 18:03:58]

On 2017/04/13 at 18:02:00, mikecase wrote:
> A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2819593002/ by mikecase@chromium.org.
> 
> The reason for reverting is: crbug/711372.

Seems like its possible for resouce_zip to be in the build_config that doesnt
actually exist? Unsure, but I guess I have to check the zip files exist before
trying to unzip them.