[GN] Add a template which wraps generate_test_lists.py.

[GN] Add a template which wraps generate_test_lists.py.

generate_test_lists.py is currently our solution for building and
running tests in an automated fashion. This CL adds a GN target,
cast_test_lists, which, when invoked, generates lists of tests to be
built and run.

Bug: b/23533616
BUG=516899
Committed: https://crrev.com/87db9d1c44344e7cff12e75214d71de8966f6014
Cr-Commit-Position: refs/heads/master@{#352412}

[slan, 2015-10-01 00:49:30]

slan@chromium.org changed reviewers:
+ alokp@chromium.org, derekjchow@chromium.org, halliwell@chromium.org,
mbjorge@chromium.org

[slan, 2015-10-01 00:49:31]

Though I don't love this script, I want to make sure that the builds are testing
on GN. These templates allow us to make calls to the python script in the
correct order, and obscure most of the mess from the developer.

The filters are really messy, but I prefer them in the same file as the list of
tests, rather than broken out elsewhere. They are copied word-for-word from GYP.

PTAL

[alokp, 2015-10-01 13:13:46]

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:38: cast_test_group("cast_tests") {
If both of these templates are only being used once, why do you need a template?
Do you expect to use them in internal code as well?

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
File chromecast/build/tests/cast_test.gni (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:30: #   # This is an organizational target.
This cannot be built directly.
I did not understand the comment about not being built directly.

[slan, 2015-10-01 14:08:56]

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:38: cast_test_group("cast_tests") {
On 2015/10/01 13:13:46, Alok Priyadarshi wrote:
> If both of these templates are only being used once, why do you need a
template?
> Do you expect to use them in internal code as well?

Exactly. There will be 2 cast_test_group()s: one for external tests and one for
internal tests. And while there will only be one cast_group_test_list, that
object requires knowledge of the implicit targets created for each
cast_test_group, so it makes sense to define it in this .gni. Here is the
internal CL for reference:

https://eureka-internal-review.git.corp.google.com/#/c/39533/

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
File chromecast/build/tests/cast_test.gni (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:30: #   # This is an organizational target.
This cannot be built directly.
On 2015/10/01 13:13:46, Alok Priyadarshi wrote:
> I did not understand the comment about not being built directly.

There is no ninja target created with this name. In this case, the targets
"cast_tests_build_list" and "cast_tests_filters" will be created (transparent to
the developer). These are depended upon by cast_group_test_list targets.

However, calling ninja -C out/Release cast_tests will give you something like:
No such target exists.

[mbjorge, 2015-10-01 17:11:40]

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:37: # cannot be built directly.
Can you add a note here that also says which target I should be trying to build
if I want to make/run the tests (e.g. "See cast_test_lists below")? Or maybe
switch the order and put the buildable/runnable target first?

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:44: "//content/content_unittests",
Is this target supposed to be //content/test:content_unittests?

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:62: if (target_cpu == "arm" && target_os == "linux") {
I like the filters being here. I don't think it would make sense to put them
elsewhere.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:106: } else {
maybe copy over the "#else x86 or android" comment, to make it more clear, since
the matching if() is kind of far away? Or, even better make it into an 'else if'
with an explicit condition

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:142: test_groups += [
"//chromecast/internal:internal_unittests" ]
the name 'internal_unittests' looks more like a test() target and not a
test_group like 'cast_tests'. maybe cast_internal_tests or internal_cast_tests
for consistency? Or change cast_tests to be cast_unittests?

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
File chromecast/build/tests/cast_test.gni (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:68: # An group of test executables.
Including a test target in this group makes it
nit: An -> A

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:69: # possible for chrome It also defines
an action which generates a build
> possible for chrome It

I think you a word here?

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:71: # Do not build these internal targets.
Build cast_test_group_list instead.
(This is basically the comment I want next to cast_test_group("cast_tests"))

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:75: #       A list of cast_test targets
included in the assembly. Do not include any
list of cast_test targets, or just test targets?

[slan, 2015-10-01 20:08:03]

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:37: # cannot be built directly.
On 2015/10/01 17:11:40, mbjorge wrote:
> Can you add a note here that also says which target I should be trying to
build
> if I want to make/run the tests (e.g. "See cast_test_lists below")? Or maybe
> switch the order and put the buildable/runnable target first?

Done.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:44: "//content/content_unittests",
On 2015/10/01 17:11:40, mbjorge wrote:
> Is this target supposed to be //content/test:content_unittests?

Yes, added a mechanism to ensure that the target names are valid.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:62: if (target_cpu == "arm" && target_os == "linux") {
On 2015/10/01 17:11:39, mbjorge wrote:
> I like the filters being here. I don't think it would make sense to put them
> elsewhere.

Agreed.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:106: } else {
On 2015/10/01 17:11:39, mbjorge wrote:
> maybe copy over the "#else x86 or android" comment, to make it more clear,
since
> the matching if() is kind of far away? Or, even better make it into an 'else
if'
> with an explicit condition

Done.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/BUILD.gn#...
chromecast/BUILD.gn:142: test_groups += [
"//chromecast/internal:internal_unittests" ]
On 2015/10/01 17:11:40, mbjorge wrote:
> the name 'internal_unittests' looks more like a test() target and not a
> test_group like 'cast_tests'. maybe cast_internal_tests or internal_cast_tests
> for consistency? Or change cast_tests to be cast_unittests?

Done.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
File chromecast/build/tests/cast_test.gni (right):

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:68: # An group of test executables.
Including a test target in this group makes it
On 2015/10/01 17:11:40, mbjorge wrote:
> nit: An -> A

Done.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:69: # possible for chrome It also defines
an action which generates a build
On 2015/10/01 17:11:40, mbjorge wrote:
> > possible for chrome It
> 
> I think you a word here?

This guy.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:71: # Do not build these internal targets.
Build cast_test_group_list instead.
On 2015/10/01 17:11:40, mbjorge wrote:
> (This is basically the comment I want next to cast_test_group("cast_tests"))

Done.

https://chromiumcodereview.appspot.com/1382713003/diff/1/chromecast/build/tes...
chromecast/build/tests/cast_test.gni:75: #       A list of cast_test targets
included in the assembly. Do not include any
On 2015/10/01 17:11:40, mbjorge wrote:
> list of cast_test targets, or just test targets?

Good catch, removed.

[derekjchow1, 2015-10-01 20:16:35]

https://chromiumcodereview.appspot.com/1382713003/diff/20001/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1382713003/diff/20001/chromecast/BUILD...
chromecast/BUILD.gn:106: } else if (target_os == "android" || target_cpu !=
"arm") {
Swap target_cpu and target_os to be consistent with line 62.

[slan, 2015-10-01 20:19:39]

https://chromiumcodereview.appspot.com/1382713003/diff/20001/chromecast/BUILD.gn
File chromecast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1382713003/diff/20001/chromecast/BUILD...
chromecast/BUILD.gn:106: } else if (target_os == "android" || target_cpu !=
"arm") {
On 2015/10/01 20:16:34, derekjchow1 wrote:
> Swap target_cpu and target_os to be consistent with line 62.

Done.

[mbjorge, 2015-10-01 20:59:54]

lgtm

[slan, 2015-10-02 22:34:29]

Ping for feedback! This feature is the only blocker for automated testing on GN.

Thanks,

[derekjchow1, 2015-10-02 23:01:59]

lgtm

[halliwell, 2015-10-02 23:13:41]

On 2015/10/02 23:01:59, derekjchow1 wrote:
> lgtm

rs-lgtm

[alokp, 2015-10-02 23:58:19]

lgtm

[slan, 2015-10-05 18:56:20]

The most recent patch:
 - rebases
 - fixes a bug in a unittest
 - Adds an option to build all dependency tests when the test list target is
invoked. Our build infrastructure unfortunately relies on this assumption. I
plan on deprecating that feature to more closely align it with Chrome: Reading a
script to build each testing target individually. But for now it has parity with
GYP.

Verified with internal automated test scripts locally.

[slan, 2015-10-05 18:56:39]

The CQ bit was checked by slan@chromium.org

[slan, 2015-10-05 18:56:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
derekjchow@chromium.org, halliwell@chromium.org, mbjorge@chromium.org,
alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/1382713003/#ps60001
(title: "Add option to build all tests when cast_test_group_list is invoked.")

[commit-bot: I haz the power, 2015-10-05 18:57:33]

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

[commit-bot: I haz the power, 2015-10-05 20:37:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-05 20:39:20]

Patchset 4 (id:??) landed as
https://crrev.com/87db9d1c44344e7cff12e75214d71de8966f6014
Cr-Commit-Position: refs/heads/master@{#352412}