Move the test template to //testing/test.gni (part 2)

Move the test template to //testing/test.gni (part 2)

This CL move the test target from build/config/BUILDCONFIG.gn to testing/test.gni

It also update the test template to automatically build an apk on Android.

R=cjhopman@chromium.org,brettw@chromium.org

Committed: https://crrev.com/fb5251d1ff4f36ea296da4be72ddf26a9e3e5643
Cr-Commit-Position: refs/heads/master@{#312396}

[qsr, 2015-01-09 13:03:32]

Chris, Brett, what do you think about this?

The main place to look at is //testing/rules.gni

The idea is that unit tests on android needs an apk, the way to do this now is
to conditionally add the "//testing/android:native_test_native_code" as a
dependency on the test target, and then declare a unittest_apk target.

This is a lot of boilerplate that can be automated. It would be better to just
update build/config/BUILDCONFIG.gn, but I don't feel like having dependency from
this file to //build/config/android and //testing is correct. Am I correct?

 If so, what do you think about having the test template in //testing as in this
CL?

 This is mostly a PoC at this point, because if we go forward, I will need to
first define a new test_new template, commit this, then update webkit and then
remove test from BUILDCONFIG and rename test_new, because the webkit repo
contains gn file with a test template.

[brettw, 2015-01-12 09:13:39]

I will leave Android opinions to Chris.

I think moving the test template out is good. But the template file has to be in
build/config somewhere (the rule is that build/* can't depend on anything
outside of build since it's used in various contexts liks v8).

I would call it build/config/test.gni

[cjhopman, 2015-01-12 18:36:13]

I think this is fine, I'm not a big fan of all the boilerplate unittest_apk
targets that we currently have to create.

Would it make sense to import this somewhere shared by everything so that we
don't need to have the import everywhere? This would then bring in
//build/config/android/rules.gni everywhere, too (I haven't thought much about
whether that would be a good or bad thing). I don't know what perf impact adding
imports to everything would have.

https://codereview.chromium.org/824263004/diff/1/testing/rules.gni
File testing/rules.gni (right):

https://codereview.chromium.org/824263004/diff/1/testing/rules.gni#newcode27
testing/rules.gni:27: # See above call.
Might want to bring the relevant comment from BUILDCONFIG.gn

[tfarina, 2015-01-12 18:44:58]

tfarina@chromium.org changed reviewers:
+ tfarina@chromium.org

[tfarina, 2015-01-12 18:45:00]

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (left):

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn#oldcode854
ui/base/BUILD.gn:854: #"testing/android/native_test.gyp:native_test_native_code"
 TODO(GYP)
Why this is not necessary anymore? I was going to upload a CL fixing this TODO.

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn#oldcode907
ui/base/BUILD.gn:907: unittest_apk("ui_base_unittests_apk") {
Why is is going away now?

[qsr, 2015-01-12 18:47:16]

On 2015/01/12 18:36:13, cjhopman wrote:
> I think this is fine, I'm not a big fan of all the boilerplate unittest_apk
> targets that we currently have to create.

 Except that it seems I cannot make this working. Brett comment is that we need
test.gni is //build, but //build cannot depend on //testing, which we need for
android.

 How can we make this working? Should we have a testing.gni in build, and a
chrome_testing in //testing, the second one creating the apk? Is everyone fine
with this?

> Would it make sense to import this somewhere shared by everything so that we
> don't need to have the import everywhere? This would then bring in
> //build/config/android/rules.gni everywhere, too (I haven't thought much about
> whether that would be a good or bad thing). I don't know what perf impact
adding
> imports to everything would have.
> 
> https://codereview.chromium.org/824263004/diff/1/testing/rules.gni
> File testing/rules.gni (right):
> 
> https://codereview.chromium.org/824263004/diff/1/testing/rules.gni#newcode27
> testing/rules.gni:27: # See above call.
> Might want to bring the relevant comment from BUILDCONFIG.gn

[qsr, 2015-01-12 18:48:54]

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (left):

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn#oldcode854
ui/base/BUILD.gn:854: #"testing/android/native_test.gyp:native_test_native_code"
 TODO(GYP)
On 2015/01/12 18:44:59, tfarina wrote:
> Why this is not necessary anymore? I was going to upload a CL fixing this
TODO.

Because this CL change the test template so that we never need anything like
this anymore. As this is WiP, please feel free to fix that as you need, and I
will just remove it when this lands.

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn#oldcode907
ui/base/BUILD.gn:907: unittest_apk("ui_base_unittests_apk") {
On 2015/01/12 18:44:59, tfarina wrote:
> Why is is going away now?

Because this CL change the test template to automatically create the apk.

[cjhopman, 2015-01-12 19:10:22]

https://codereview.chromium.org/824263004/diff/1/testing/rules.gni
File testing/rules.gni (right):

https://codereview.chromium.org/824263004/diff/1/testing/rules.gni#newcode69
testing/rules.gni:69: "//testing/android:native_test_native_code",
This is actually the added dependency on //testing, right?

So, even moving the apk part out of //build won't solve the problem.

In general, the problem is that a lot of how we build things for Android depends
on bits of //base or //testing or //tools. In addition to the deps here, and
even though the unittest_apk part below comes from //build/config/android, it,
too, then depends on //testing and //tools and such.

You could limit these violations to //build/config/android by defining this list
of test shared_library deps there, or even moving this whole if block to a
template defined there.

We should fix those violations in //build/config/android, but I don't expect
that to happen soon.

[tfarina, 2015-01-12 19:10:37]

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (left):

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn#oldcode905
ui/base/BUILD.gn:905: # TODO(GYP) Mac (ui_base_tests_bundle)
btw, do not remove this Mac TODO. We still need to do it.

[qsr, 2015-01-12 19:15:52]

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn
File ui/base/BUILD.gn (left):

https://codereview.chromium.org/824263004/diff/1/ui/base/BUILD.gn#oldcode905
ui/base/BUILD.gn:905: # TODO(GYP) Mac (ui_base_tests_bundle)
On 2015/01/12 19:10:37, tfarina wrote:
> btw, do not remove this Mac TODO. We still need to do it.

Will do. The comment was really weird, as it seemed to apply to an is_android
section, which made little sense.

[brettw, 2015-01-13 03:27:55]

Sorry I was confused before when I answered. The fact that this injects a
dependency on //testing means that this *can't* go into //build/* rather than
that it must.

As far as I know, the files in build won't need to depend on the new .gni file
you're adding, so then everything should be fine. I'm assuming that other
projects that need to define a test will also have the testing directory since
they'll need gtest and friends.

I think the name //testing/test.gni is better than ".../rules.gni" personally.

[qsr, 2015-01-15 02:05:23]

Brett, Chris, can you take a look. This seems to be working (needs a change in
pdfium that is in progress before being able to land, but that should not impact
this change)

[cjhopman, 2015-01-15 21:03:40]

lgtm

[brettw, 2015-01-20 21:09:31]

lgtm

[qsr, 2015-01-21 09:39:02]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2015-01-21 09:41:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/824263004/40001

[commit-bot: I haz the power, 2015-01-21 14:49:39]

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

[commit-bot: I haz the power, 2015-01-21 14:49:41]

Failed to apply patch for ui/wm/BUILD.gn:
While running git apply --index -3 -p1;
  error: patch failed: ui/wm/BUILD.gn:2
  Falling back to three-way merge...
  Applied patch to 'ui/wm/BUILD.gn' with conflicts.
  U ui/wm/BUILD.gn

Patch:       ui/wm/BUILD.gn
Index: ui/wm/BUILD.gn
diff --git a/ui/wm/BUILD.gn b/ui/wm/BUILD.gn
index
ea50577c4cc091d231911250e2de5ede37ad5ed9..eee06a960aeeb1db9c7f7a1d7bf209daf4b5b960
100644
--- a/ui/wm/BUILD.gn
+++ b/ui/wm/BUILD.gn
@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import("//testing/test.gni")
+
 component("wm") {
   sources = [
     "core/accelerator_delegate.h",

[commit-bot: I haz the power, 2015-01-21 14:49:47]

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

[qsr, 2015-01-21 14:51:21]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2015-01-21 14:52:25]

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

[commit-bot: I haz the power, 2015-01-21 15:24:47]

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

[commit-bot: I haz the power, 2015-01-21 15:24:49]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[qsr, 2015-01-21 15:38:02]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2015-01-21 15:38:46]

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

[commit-bot: I haz the power, 2015-01-21 15:58:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-21 15:58:57]

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