[Cronet] Add cronet_test_package build target

components/cronet/android/BUILD.gn

Index: components/cronet/android/BUILD.gn
diff --git a/components/cronet/android/BUILD.gn b/components/cronet/android/BUILD.gn
index 725024e297a43e8c5c7280cbae71ad33bd4c765f..e5f74fef8eccb79e12e1687cdb9ae0e7fb37b00d 100644
--- a/components/cronet/android/BUILD.gn
+++ b/components/cronet/android/BUILD.gn
@@ -790,7 +790,9 @@ test("cronet_unittests") {
 }
 
 _package_dir = "$root_out_dir/cronet"
+_test_package_dir = "$root_out_dir/cronet/test"
 _extract_cronet_jars_dir = "$target_gen_dir/cronet_jar_extract"
+_extract_cronet_test_jars_dir = "$target_gen_dir/cronet_test_jar_extract"
 
 action("extract_cronet_jars") {
   # extract_from_jars.py deletes the target directory before extracting.
@@ -849,6 +851,75 @@ action("repackage_extracted_jars") {
   ]
 }
 
+action("extract_cronet_test_jars") {
+  # extract_from_jars.py deletes the target directory before extracting.
+  script = "//components/cronet/tools/extract_from_jars.py"
+  depfile = "$target_gen_dir/$target_name.d"
+  testonly = true
+
+  sources = [
+    "$root_out_dir/lib.java/base/base_java.jar",
+    "$root_out_dir/lib.java/base/base_java_test_support.jar",
+    "$root_out_dir/lib.java/components/cronet/android/cronet_javatests.jar",
+    "$root_out_dir/lib.java/components/cronet/android/cronet_test_apk_java.jar",
+    "$root_out_dir/lib.java/net/android/net_java.jar",
+    "$root_out_dir/lib.java/net/android/net_java_test_support.jar",
+    "$root_out_dir/lib.java/third_party/netty-tcnative/netty-tcnative_java.jar",
+    "$root_out_dir/lib.java/url/url_java.jar",
+    "//third_party/netty4/src/jar/all-in-one/netty-all-4.1.0.CR1-20160111.120759-50.jar",

[mef, 2016/12/14 23:04:51]

This seems very fragile. 
I wonder whether there is a way to get the list of jars from
"//third_party/netty4:netty_all_java" target.
Adding jbudorick to comment.

[pauljensen, 2016/12/15 04:08:34]

Ya, I agree it's fragile.  I showed it to Andrei last week and complained it was
fragile but I'm not sure either of us had a good idea to improve it.  In Blaze
you can specify targets by name as sources of genrules...but not in GN.  I
imagine we could do something like write a "template" that accesses the jar
paths but that might be a lot of work for this.  Another alternative would be to
have a gypi file that exposes the jar path, like net.gypi.  For now I don't
think this will be a significant amount of work to maintain, and am okay punting
doing something smarter until later.  If there's a simple and easy solution I
can give that a shot.

[mef, 2016/12/15 18:06:34]

Yeah, per our offline conversation, adding something like
//third_party/netty4/netty.gni would make it less fragile.

[jbudorick, 2016/12/15 18:19:34]

We've used .build_config files for stuff like this elsewhere. I think you'd want
something like:

"@FileArg(//third_party/netty4:netty_all_java__build_config:deps_info:jar_path)"

I think you could do something like this for all of the above jars.

[pauljensen, 2016/12/15 19:12:52]

I tried that and just got:
ninja: error:
'../../components/cronet/android/@FileArg(/third_party/netty4:netty_all_java__build_config:deps_info:jar_path)',
needed by
'obj/components/cronet/android/extract_cronet_test_jars.inputdeps.stamp',
missing and no known rule to make it

+  ]
+
+  _stamp_file = "$target_gen_dir/$target_name.stamp"
+  outputs = [
+    _stamp_file,
+  ]
+
+  _rebased_sources = rebase_path(sources, root_build_dir)
+
+  args = [
+    "--classes-dir",
+    rebase_path(_extract_cronet_test_jars_dir, root_build_dir),
+    "--jars=${_rebased_sources}",
+    "--depfile",
+    rebase_path(depfile, root_build_dir),
+    "--stamp",
+    rebase_path(_stamp_file, root_build_dir),
+  ]
+
+  deps = [
+    ":cronet_javatests",
+    ":cronet_test_apk_java",
+    "//base:base_java",
+    "//base:base_java_test_support",
+    "//net/android:net_java",
+    "//net/android:net_java_test_support",
+    "//third_party/netty-tcnative:netty-tcnative_java",
+    "//third_party/netty4:netty_all_java",
+    "//url:url_java",
+  ]
+}
+
+action("repackage_extracted_test_jars") {
+  _output_jar = "$_test_package_dir/cronet_tests_java.jar"
+  testonly = true
+
+  script = "//build/android/gyp/jar.py"
+  outputs = [
+    _output_jar,
+  ]
+
+  args = [
+    "--classes-dir",
+    rebase_path(_extract_cronet_test_jars_dir, root_build_dir),
+    "--jar-path",
+    rebase_path(_output_jar, root_build_dir),
+  ]
+
+  deps = [
+    ":extract_cronet_test_jars",
+  ]
+}
+
 template("jar_src") {
   action(target_name) {
     _rebased_src_search_dirs =
@@ -1084,6 +1155,77 @@ copy("cronet_package_copy_native_lib_unstripped") {
   ]
 }
 
+copy("cronet_package_copy_native_test_lib") {

[mef, 2016/12/14 23:04:51]

I wonder whether we need stripped native test lib at all.
The goal is to use it for testing, and unstripped seems like a proper target.

[pauljensen, 2016/12/15 04:08:35]

libcronet_tests.so unstripped is more than 100 MB bigger than libcronet_tests.so
stripped, so I imagine using the unstripped one would make our testing quite
slow as installing large APKs on emulators can take several minutes, which is
significant because running our tests takes several minutes.

[mef, 2016/12/15 18:06:34]

Acknowledged.

[jbudorick, 2016/12/15 18:19:34]

This is something we're currently wrestling with for the rest of our android
native tests, too.

+  testonly = true
+  sources = [
+    "$root_out_dir/libcronet_tests.so",
+    "$root_out_dir/libnetty-tcnative.so",
+  ]
+  outputs = [
+    "$_test_package_dir/libs/${android_app_abi}/{{source_file_part}}",
+  ]
+  deps = [
+    ":cronet_tests",
+    "//third_party/netty-tcnative:netty-tcnative-so",
+  ]
+}
+
+copy("cronet_package_copy_native_test_lib_unstripped") {
+  testonly = true
+  sources = [
+    "$root_out_dir/lib.unstripped/libcronet_tests.so",

[mef, 2016/12/14 23:04:51]

based on the comment above, would we ever need to manually symbolicate test
crash if test app used unstripped native library?

[pauljensen, 2016/12/15 04:08:35]

The crash handler could probably symbolicate if we used unstrupped native
libraries, but as I said above, I think it would slow our testing down
significantly to download and install the larger APKs.  I'd rather not slow down
the common case of running the tests to speed up the rare case of wanting to
symbolicate.

[mef, 2016/12/15 18:06:34]

Acknowledged.

+    "$root_out_dir/lib.unstripped/libnetty-tcnative.so",
+  ]
+  outputs = [
+    "$_test_package_dir/symbols/${android_app_abi}/{{source_file_part}}",
+  ]
+  deps = [
+    ":cronet_tests",
+    "//third_party/netty-tcnative:netty-tcnative-so",
+  ]
+}
+
+copy("cronet_package_copy_test_assets") {
+  testonly = true
+  sources = [
+    "test/assets",
+  ]
+  outputs = [
+    "$_test_package_dir/assets",
+  ]
+}
+
+copy("cronet_package_copy_test_support_apks") {
+  testonly = true
+  sources = [
+    "$root_out_dir/apks/ChromiumNetTestSupport.apk",

[mef, 2016/12/14 23:04:51]

Interesting. Maybe add comment that this provides EmbeddedTestServer?

[pauljensen, 2016/12/15 04:08:35]

Done.

+  ]
+  outputs = [
+    "$_test_package_dir/apks/${android_app_abi}/{{source_file_part}}",

[mef, 2016/12/14 23:04:51]

kudos for including abi into output path.

[pauljensen, 2016/12/15 04:08:34]

Acknowledged.

+  ]
+  deps = [
+    "//net/android:net_test_support_apk",
+  ]
+}
+
+copy("cronet_package_copy_test_files") {
+  testonly = true
+  sources = [
+    "//net/data/ssl/certificates/quic_test.example.com.crt",
+    "//net/data/ssl/certificates/quic_test.example.com.key",
+    "//net/data/ssl/certificates/quic_test.example.com.key.pkcs8",
+    "//net/data/ssl/certificates/quic_test.example.com.key.sct",
+  ]
+  outputs = [
+    "$_test_package_dir/assets/test_files/net/data/ssl/certificates/{{source_file_part}}",
+  ]
+  deps = [
+    # Not really dependent, but builds can fail if these two targets attempt
+    # to create the "assets" subdirectory simultaneously.
+    ":cronet_package_copy_test_assets",
+  ]
+}
+
 # Enforce that ARM Neon is not used when building for ARMv7
 if (target_cpu == "arm" && arm_version == 7 && !arm_use_neon) {
   action("enforce_no_neon") {
@@ -1109,6 +1251,9 @@ if (target_cpu == "arm" && arm_version == 7 && !arm_use_neon) {
 }
 
 group("cronet_package") {
+  # Marked as testonly as it contains test-only targets too.
+  testonly = true
+
   # Enforce that arm_use_neon==false when building for ARMv7 by
   # not including any deps in cronet_package target otherwise.
   if (!(target_cpu == "arm" && arm_version == 7) || !arm_use_neon) {
@@ -1116,6 +1261,7 @@ group("cronet_package") {
       ":cronet_package_copy",
       ":cronet_package_copy_native_lib",
       ":cronet_package_copy_native_lib_unstripped",
+      ":cronet_test_package",
       ":generate_javadoc",
       ":generate_licenses",
       ":jar_cronet_api_source",
@@ -1130,3 +1276,20 @@ group("cronet_package") {
     }
   }
 }
+
+group("cronet_test_package") {
+  testonly = true
+
+  # Enforce that arm_use_neon==false when building for ARMv7 by
+  # not including any deps in cronet_package target otherwise.
+  if (!(target_cpu == "arm" && arm_version == 7) || !arm_use_neon) {

[mef, 2016/12/14 23:04:51]

Do we really need to enforce it here? 
What exactly would fail if these conditions are not met?
Also, should we limit a set of arch+config configurations for which we create
cronet_test_package?
I doubt that we'll run those tests for MIPS, for example.

[pauljensen, 2016/12/15 04:08:34]

No, removed.
The check in cronet_package should prevent any failures.
I changed it to avoided MIPS, where we don't test.

+    deps = [
+      ":cronet_package_copy_native_test_lib",
+      ":cronet_package_copy_native_test_lib_unstripped",
+      ":cronet_package_copy_test_assets",
+      ":cronet_package_copy_test_files",
+      ":cronet_package_copy_test_support_apks",
+      ":repackage_extracted_test_jars",
+    ]
+  }
+}