Android: Instrumentation tests in gradle

Android: Instrumentation tests in gradle

Add instrumentation tests as androidTest for gradle projects. May
require more tweaking to get all the dependencies/files generated.

BUG=680217
Review-Url: https://codereview.chromium.org/2633533002
Cr-Commit-Position: refs/heads/master@{#444800}
Committed: https://chromium.googlesource.com/chromium/src/+/5ee39c9a1b872d95800d7527dbc2ffe7b16cab93

[Peter Wen, 2017-01-12 20:54:03]

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

[Peter Wen, 2017-01-12 20:54:03]

Just wanted to get your opinion before I go too much further. I want to add:

- A processor to process _ProjectEntry rather than have so many processing
methods like JavaDirs in it and passing around so many options.
- Have the junit tests specify which java library they are primarily testing.
Most are actually pretty clear, just remove junit_test and add java, but
//clank/junit:junit is the exception here, it tests //clank/java. Just wanted an
equivalent of apk_under_test to be explicit. :P

[agrieve, 2017-01-12 21:28:36]

Looks great overall! The only approach-wise comment I have, is that I think it
might be nicer to try and leave "ProjectEntry" as a data-focused as possible
(struct-like), and create a new class that has the processing logic in it.

E.g. in MVC, ProjectEntry == the Model, and maybe have a "GradleGenerator" be
the controller (we don't have a view, so what a great analogy :P)

You could pass things like the resources flag value to the constructor of the
GradleGenerator to save on parameters.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/androi...
File build/android/gradle/android.jinja (right):

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/androi...
build/android/gradle/android.jinja:4: {% macro expand_sourceset(variables,
prefix) %}
fancy!

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/androi...
build/android/gradle/android.jinja:6: {{ prefix }}.manifest.srcFile "{{
variables.android_manifest }}"
nit: might as well put this in the block below. e.g.:

{{ prefix }} {
  manifest.srcFile ""
  java.srcDirs = ...

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
File build/android/gradle/generate_gradle.py (right):

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:142: def SetTest(self, test_entry):
It'd be more python-y to just make test_entry and android_test_entry a public
property. (you can always add setters/getters later if need be)

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:155: srcjars =
_RebasePath(self.Gradle().get('bundled_srcjars', []))
I think it'd be nicer to not rebase in these methods. rebasing should be done by
the thing using it.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:170: java_dirs = _CreateJavaSourceDir(
This seems a bit odd as well. The method seems like a getter, but has the
side-effect of modifying the filesystem.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:488: return combined_entries
From inspection, it seems like there's risk of there being entries in the dicts
that didn't match up with a target. Should this instead remove entries from the
dicts as they are found, and then have the remaining added as top-level entries?

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:559: entries =
_CombineTestEntries(all_entries)
nit: move this above the "writing gradle files" log

[Peter Wen, 2017-01-18 20:50:31]

Description was changed from

==========
Android: Instrumentation and junit tests in gradle

Add instrumentation tests as androidTest and junit tests as test for
gradle projects. Requires more explicitly linking junit tests with their
test targets.

BUG=680217
==========

to

==========
Android: Instrumentation tests in gradle

Add instrumentation tests as androidTest for gradle projects. May
require more tweaking to get all the dependencies/files generated.

BUG=680217
==========

[Peter Wen, 2017-01-18 20:54:41]

Turns out even android studio canary doesn't do the same thing as intellij
2016.1 as it's something in intellij's gradle plugin. So android's gradle plugin
would have to make some changes. :/

For now leaving junit targets as separate modules explicitly.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/androi...
File build/android/gradle/android.jinja (right):

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/androi...
build/android/gradle/android.jinja:4: {% macro expand_sourceset(variables,
prefix) %}
On 2017/01/12 21:28:35, agrieve wrote:
> fancy!

Acknowledged.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/androi...
build/android/gradle/android.jinja:6: {{ prefix }}.manifest.srcFile "{{
variables.android_manifest }}"
On 2017/01/12 21:28:35, agrieve wrote:
> nit: might as well put this in the block below. e.g.:
> 
> {{ prefix }} {
>   manifest.srcFile ""
>   java.srcDirs = ...

Done.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
File build/android/gradle/generate_gradle.py (right):

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:142: def SetTest(self, test_entry):
On 2017/01/12 21:28:35, agrieve wrote:
> It'd be more python-y to just make test_entry and android_test_entry a public
> property. (you can always add setters/getters later if need be)

Done.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:155: srcjars =
_RebasePath(self.Gradle().get('bundled_srcjars', []))
On 2017/01/12 21:28:35, agrieve wrote:
> I think it'd be nicer to not rebase in these methods. rebasing should be done
by
> the thing using it.

Hmm... now that they're in GradleGenerator do you still feel the same way?

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:170: java_dirs = _CreateJavaSourceDir(
On 2017/01/12 21:28:35, agrieve wrote:
> This seems a bit odd as well. The method seems like a getter, but has the
> side-effect of modifying the filesystem.

Now it's called GenJavaDirs. :)

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:488: return combined_entries
On 2017/01/12 21:28:35, agrieve wrote:
> From inspection, it seems like there's risk of there being entries in the
dicts
> that didn't match up with a target. Should this instead remove entries from
the
> dicts as they are found, and then have the remaining added as top-level
entries?

Done.

https://codereview.chromium.org/2633533002/diff/1/build/android/gradle/genera...
build/android/gradle/generate_gradle.py:559: entries =
_CombineTestEntries(all_entries)
On 2017/01/12 21:28:35, agrieve wrote:
> nit: move this above the "writing gradle files" log

Done.

[agrieve, 2017-01-18 21:58:10]

lgtm with some yak-shaving of naming.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
File build/android/gradle/generate_gradle.py (right):

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:183: java_files =
build_utils.ReadSourcesList(java_sources_file)
nit: cache this value.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:187: class _GradleGenerator(object):
nit: this doesn't actuall generate gradle files. How about
_ProjectVariablesGenerator or somesuch

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:214: def OutputDir(self, entry):
nit: conflicts with other output_dir. How about "EntryOutputDir"

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:217: def GenerateSrcjars(self, entry):
this doesn't generate anything. How about jjust SrcJars()

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:243: def GeneratedInputs(self, entry):
nit: might make more sense to have this right beside GenerateSrcjars

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:456: def _CombineTestEntries(entries):
It would be great to add some comments to this method saying why we do this:
- minimize gradle files
- adds under-test as a dependency
- does not work for junit

[Peter Wen, 2017-01-19 16:43:07]

Thanks for the review!

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
File build/android/gradle/generate_gradle.py (right):

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:183: java_files =
build_utils.ReadSourcesList(java_sources_file)
On 2017/01/18 21:58:10, agrieve wrote:
> nit: cache this value.

Done.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:187: class _GradleGenerator(object):
On 2017/01/18 21:58:10, agrieve wrote:
> nit: this doesn't actuall generate gradle files. How about
> _ProjectVariablesGenerator or somesuch

Done.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:214: def OutputDir(self, entry):
On 2017/01/18 21:58:10, agrieve wrote:
> nit: conflicts with other output_dir. How about "EntryOutputDir"

Done.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:217: def GenerateSrcjars(self, entry):
On 2017/01/18 21:58:10, agrieve wrote:
> this doesn't generate anything. How about jjust SrcJars()

Done.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:243: def GeneratedInputs(self, entry):
On 2017/01/18 21:58:10, agrieve wrote:
> nit: might make more sense to have this right beside GenerateSrcjars

Done.

https://codereview.chromium.org/2633533002/diff/20001/build/android/gradle/ge...
build/android/gradle/generate_gradle.py:456: def _CombineTestEntries(entries):
On 2017/01/18 21:58:10, agrieve wrote:
> It would be great to add some comments to this method saying why we do this:
> - minimize gradle files
> - adds under-test as a dependency
> - does not work for junit

Done.

[Peter Wen, 2017-01-19 16:43:11]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2017-01-19 16:43:12]

The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2633533002/#ps40001
(title: "Fix per review.")

[commit-bot: I haz the power, 2017-01-19 16:43:49]

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-01-19 18:40:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484844191738220,
"parent_rev": "e1f1fe2d0357708601b18cf6155d2dc9ac5d3201", "commit_rev":
"5ee39c9a1b872d95800d7527dbc2ffe7b16cab93"}

[commit-bot: I haz the power, 2017-01-19 18:40:58]

Description was changed from

==========
Android: Instrumentation tests in gradle

Add instrumentation tests as androidTest for gradle projects. May
require more tweaking to get all the dependencies/files generated.

BUG=680217
==========

to

==========
Android: Instrumentation tests in gradle

Add instrumentation tests as androidTest for gradle projects. May
require more tweaking to get all the dependencies/files generated.

BUG=680217

Review-Url: https://codereview.chromium.org/2633533002
Cr-Commit-Position: refs/heads/master@{#444800}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ee39c9a1b872d95800d7527dbc2...
==========

[commit-bot: I haz the power, 2017-01-19 18:41:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5ee39c9a1b872d95800d7527dbc2...