[bazel] skia_android_unit_test rule: Support select() in extra_args attribute.
Also:
- Skip some test cases that make an existing skia_android_unit_test fail.
- Add a CI task for said test.
Suggested review order:
- //tests/android.bzl
- //tests/skia_binary_with_cmdline_flags_test.bzl
- //tests/adb_test.bzl
- Everything else.
Bug: skia:14227
Change-Id: I1a953519ed295617bf9e005876b9e2845d9d59b8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/705156
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
diff --git a/infra/bots/jobs.json b/infra/bots/jobs.json
index c425d92..8dee2ea 100644
--- a/infra/bots/jobs.json
+++ b/infra/bots/jobs.json
@@ -26,6 +26,7 @@
{"name": "BazelBuild-tests-gl_ganesh-linux_x64"},
{"name": "BazelBuild-tests-vulkan_ganesh-linux_x64"},
{"name": "BazelBuild-android_codec_test-pixel_5-linux_x64"},
+ {"name": "BazelBuild-android_cpu_only_test-pixel_5-linux_x64"},
{"name": "BazelTest-canvaskit_gold-modules_canvaskit_js_tests-ck_full_cpu_release_chrome-linux_x64",
"cq_config": {"location_regexes": ["modules/canvaskit/.*"]}
},
@@ -40,6 +41,7 @@
},
{"name": "BazelTest-toolchain_layering_check-experimental_bazel_test_client-release-linux_x64"},
{"name": "BazelTest-precompiled-android_codec_test-pixel_5-linux_arm64"},
+ {"name": "BazelTest-precompiled-android_cpu_only_test-pixel_5-linux_arm64"},
{"name": "Build-Debian10-GCC-x86-Debug-Docker"},
{"name": "Build-Debian10-GCC-x86-Release-Docker"},
{"name": "Build-Debian10-GCC-x86_64-Debug-Docker"},
diff --git a/infra/bots/tasks.json b/infra/bots/tasks.json
index 2f9319a..5f0a861 100644
--- a/infra/bots/tasks.json
+++ b/infra/bots/tasks.json
@@ -5,6 +5,11 @@
"BazelBuild-android_codec_test-pixel_5-linux_x64"
]
},
+ "BazelBuild-android_cpu_only_test-pixel_5-linux_x64": {
+ "tasks": [
+ "BazelBuild-android_cpu_only_test-pixel_5-linux_x64"
+ ]
+ },
"BazelBuild-base-enforce_iwyu-linux_x64": {
"tasks": [
"BazelBuild-base-enforce_iwyu-linux_x64"
@@ -90,6 +95,11 @@
"BazelTest-precompiled-android_codec_test-pixel_5-linux_arm64"
]
},
+ "BazelTest-precompiled-android_cpu_only_test-pixel_5-linux_arm64": {
+ "tasks": [
+ "BazelTest-precompiled-android_cpu_only_test-pixel_5-linux_arm64"
+ ]
+ },
"BazelTest-toolchain_layering_check-experimental_bazel_test_client-release-linux_x64": {
"tasks": [
"BazelTest-toolchain_layering_check-experimental_bazel_test_client-release-linux_x64"
@@ -3689,6 +3699,52 @@
],
"service_account": "skia-external-compile-tasks@skia-swarming-bots.iam.gserviceaccount.com"
},
+ "BazelBuild-android_cpu_only_test-pixel_5-linux_x64": {
+ "casSpec": "bazel",
+ "cipd_packages": [
+ {
+ "name": "skia/bots/bazelisk_linux_amd64",
+ "path": "bazelisk_linux_amd64",
+ "version": "version:0"
+ }
+ ],
+ "command": [
+ "./bazel_build",
+ "--project_id=skia-swarming-bots",
+ "--task_id=<(TASK_ID)",
+ "--task_name=BazelBuild-android_cpu_only_test-pixel_5-linux_x64",
+ "--label=//tests:android_cpu_only_test",
+ "--config=pixel_5",
+ "--workdir=.",
+ "--out_path=bazel_output",
+ "--saved_output_dir=tests",
+ "--bazel_arg=--config=linux_rbe",
+ "--bazel_arg=--jobs=100",
+ "--bazel_arg=--remote_download_toplevel",
+ "--bazel_arg=--adb_platform=linux_arm64"
+ ],
+ "dependencies": [
+ "Housekeeper-PerCommit-BuildTaskDrivers_linux_amd64"
+ ],
+ "dimensions": [
+ "cpu:x86-64-Haswell_GCE",
+ "gpu:none",
+ "machine_type:n1-standard-16",
+ "os:Debian-10.3",
+ "pool:Skia"
+ ],
+ "env_prefixes": {
+ "PATH": [
+ "bazelisk_linux_amd64"
+ ]
+ },
+ "idempotent": true,
+ "max_attempts": 1,
+ "outputs": [
+ "bazel_output"
+ ],
+ "service_account": "skia-external-compile-tasks@skia-swarming-bots.iam.gserviceaccount.com"
+ },
"BazelBuild-base-enforce_iwyu-linux_x64": {
"casSpec": "bazel",
"cipd_packages": [
@@ -4372,6 +4428,31 @@
"max_attempts": 1,
"service_account": "skia-external-compile-tasks@skia-swarming-bots.iam.gserviceaccount.com"
},
+ "BazelTest-precompiled-android_cpu_only_test-pixel_5-linux_arm64": {
+ "casSpec": "bazel",
+ "command": [
+ "./bazel_test_precompiled",
+ "--project_id=skia-swarming-bots",
+ "--task_id=<(TASK_ID)",
+ "--task_name=BazelTest-precompiled-android_cpu_only_test-pixel_5-linux_arm64",
+ "--workdir=.",
+ "--command=bazel_output/tests/android_cpu_only_test",
+ "--command_workdir=bazel_output/tests/android_cpu_only_test.runfiles/skia"
+ ],
+ "dependencies": [
+ "BazelBuild-android_cpu_only_test-pixel_5-linux_x64",
+ "Housekeeper-PerCommit-BuildTaskDrivers_linux_arm64"
+ ],
+ "dimensions": [
+ "os:Android",
+ "device_type:redfin",
+ "device_os:SP2A.220305.012",
+ "pool:Skia"
+ ],
+ "idempotent": true,
+ "max_attempts": 1,
+ "service_account": "skia-external-compile-tasks@skia-swarming-bots.iam.gserviceaccount.com"
+ },
"BazelTest-toolchain_layering_check-experimental_bazel_test_client-release-linux_x64": {
"casSpec": "bazel",
"cipd_packages": [
diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel
index 4cd8658..c6f6fcf 100644
--- a/tests/BUILD.bazel
+++ b/tests/BUILD.bazel
@@ -356,10 +356,22 @@
],
)
-# Some test cases fail with --config=pixel_7.
skia_android_unit_test(
name = "android_cpu_only_test",
srcs = CPU_ONLY_TESTS,
+ # TODO(lovisolo): We might want to extract these --skip flags into a separate file.
+ extra_args = select({
+ "//bazel/devices:pixel_5": [
+ "--skip",
+ "DrawText_dashout",
+ "FontHostStream",
+ "LegacyMakeTypeface",
+ "TextBlob_getIntercepts",
+ "TextBlob_iter",
+ "glyphs_to_unichars",
+ ],
+ "//conditions:default": [],
+ }),
flags = {
"fontmgr_factory": ["custom_directory_fontmgr_factory"],
"enable_sksl": ["True"],
diff --git a/tests/BazelTestRunner.cpp b/tests/BazelTestRunner.cpp
index 909cd02..e3b016b 100644
--- a/tests/BazelTestRunner.cpp
+++ b/tests/BazelTestRunner.cpp
@@ -152,6 +152,17 @@
gSkDebugToStdOut = true;
#endif
+ if (argc < 2) {
+ SkDebugf("Test runner invoked with no arguments.\n");
+ } else {
+ std::ostringstream oss;
+ oss << "Test runner invoked with arguments:";
+ for (int i = 1; i < argc; i++) {
+ oss << " " << argv[i];
+ }
+ SkDebugf("%s\n", oss.str().c_str());
+ }
+
CommandLineFlags::Parse(argc, argv);
BazelReporter reporter;
diff --git a/tests/adb_test.bzl b/tests/adb_test.bzl
index f48eee1..350c479 100644
--- a/tests/adb_test.bzl
+++ b/tests/adb_test.bzl
@@ -131,7 +131,7 @@
"Test runner script that calls the compiled C++ binary with any necessary " +
"command-line arguments. This script will be executed on the Android device."
),
- allow_single_file = [".sh"],
+ allow_single_file = True,
mandatory = True,
),
"archive": attr.label(
diff --git a/tests/android.bzl b/tests/android.bzl
index b5c5ae2..bdbe3ad 100644
--- a/tests/android.bzl
+++ b/tests/android.bzl
@@ -1,8 +1,8 @@
"""This module defines the skia_android_unit_test macro."""
load("//bazel:cc_binary_with_flags.bzl", "cc_binary_with_flags")
-load("//bazel:remove_indentation.bzl", "remove_indentation")
load(":adb_test.bzl", "adb_test")
+load(":skia_binary_with_cmdline_flags_test.bzl", "skia_binary_with_cmdline_flags_test")
def skia_test(
name,
@@ -16,16 +16,15 @@
size = None):
"""Defines a generic Skia C++ unit test.
- This macro produces a <name>_binary C++ binary and a <name>.sh wrapper script that runs the
+ This macro produces a <name>_binary C++ binary and a <name> wrapper script that runs the
binary with the desired command-line arguments (see the extra_args and requires_resources_dir
- arguments). The <name>.sh wrapper script is exposed as a Bazel test target via the sh_target
- rule.
+ arguments).
The reason why we place command-line arguments in a wrapper script is that it makes it easier
to run a Bazel-built skia_test outside of Bazel. This is useful e.g. for CI jobs where we want
to perform test compilation and execution as different steps on different hardware (e.g.
compile on a GCE machine, run tests on a Skolo device). In this scenario, the test could be
- executed outside of Bazel by simply running the <name>.sh script without any arguments. See the
+ executed outside of Bazel by simply running the <name> script without any arguments. See the
skia_android_unit_test macro for an example.
Note: The srcs attribute must explicitly include a test runner (e.g.
@@ -66,42 +65,12 @@
testonly = True, # Needed to gain access to test-only files.
)
- test_runner = "%s.sh" % name
-
- test_args = ([
- "--resourcePath",
- "$$(realpath $$(dirname $(rootpath //resources:README)))",
- ] if requires_resources_dir else []) + extra_args
-
- # This test runner might run on Android devices, which might not have a /bin/bash binary.
- test_runner_template = remove_indentation("""
- #!/bin/sh
- $(rootpath {test_binary}) {test_args}
- """)
-
- # TODO(lovisolo): This should be an actual rule. This will allow us to select() the arguments
- # based on the device (e.g. for device-specific --skip flags to skip tests).
- native.genrule(
- name = "%s_runner" % name,
- srcs = [test_binary] + (
- # The script template computes the path to //resources under the runfiles tree via
- # $$(dirname $(rootpath //resources:README)), so we need to list //resources:README
- # here explicitly. This file was chosen arbitrarily; there is nothing special about it.
- ["//resources", "//resources:README"] if requires_resources_dir else []
- ),
- outs = [test_runner],
- cmd = "echo '%s' > $@" % test_runner_template.format(
- test_binary = test_binary,
- test_args = "\\\n ".join(test_args),
- ),
- testonly = True,
- )
-
- native.sh_test(
+ skia_binary_with_cmdline_flags_test(
name = name,
+ test_binary = test_binary,
+ requires_resources_dir = requires_resources_dir,
+ extra_args = extra_args,
size = size,
- srcs = [test_runner],
- data = [test_binary] + (["//resources"] if requires_resources_dir else []),
tags = tags,
)
@@ -130,16 +99,16 @@
- It produces a <name>.tar.gz archive containing the Android binary, a minimal launcher script
that invokes the binary with the necessary command-line arguments, and any static resources
needed by the test, such as fonts and images under //resources.
- - It produces a <name>.sh test runner script that extracts the tarball into the device via
- `adb`, sets up the device, runs the test, cleans up and pipes through the test's exit code.
+ - It produces a <name> test runner script that extracts the tarball into the device via `adb`,
+ sets up the device, runs the test, cleans up and pipes through the test's exit code.
For CI jobs, rather than invoking "bazel test" on a Raspberry Pi attached to the Android device
under test, we compile and run the test in two separate tasks:
- A build task running on a GCE machine compiles the test on RBE with Bazel and stores the
- <name>.tar.gz and <name>.sh output files to CAS.
- - A test task running on a Skolo Raspberry Pi downloads <name>.tar.gz and <name>.sh from CAS
- and executes <name>.sh *outside of Bazel*.
+ <name>.tar.gz and <name> output files to CAS.
+ - A test task running on a Skolo Raspberry Pi downloads <name>.tar.gz and <name> from CAS and
+ executes <name> *outside of Bazel*.
The reason why we don't want to run Bazel on a Raspberry Pi is due to its constrained
resources.
@@ -172,8 +141,11 @@
to the path to said directory.
"""
+ test_runner = "%s_cpp_test" % name
+ test_binary = "%s_cpp_test_binary" % name # Produced by the skia_test macro.
+
skia_test(
- name = "%s_cpp_test" % name,
+ name = test_runner,
srcs = select({
requires_condition: srcs + ["//tests:BazelTestRunner.cpp"],
"//conditions:default": ["//tests:BazelNoopRunner.cpp"],
@@ -194,9 +166,6 @@
size = "large", # Can take several minutes.
)
- test_binary = "%s_cpp_test_binary" % name
- test_runner = "%s_cpp_test.sh" % name
-
archive = "%s_archive" % name
archive_srcs = [test_runner, test_binary] + (
["//resources"] if requires_resources_dir else []
diff --git a/tests/skia_binary_with_cmdline_flags_test.bzl b/tests/skia_binary_with_cmdline_flags_test.bzl
new file mode 100644
index 0000000..ca724e0
--- /dev/null
+++ b/tests/skia_binary_with_cmdline_flags_test.bzl
@@ -0,0 +1,75 @@
+"""This module defines the skia_binary_with_cmdline_flags_test rule."""
+
+load("//bazel:remove_indentation.bzl", "remove_indentation")
+
+# https://bazel.build/rules/lib/builtins/ctx
+def _skia_binary_with_cmdline_flags_test_impl(ctx):
+ test_args = ([
+ "--resourcePath",
+ "$(dirname $(realpath $(rootpath %s)))" % ctx.attr._arbitrary_file_in_resources_dir.label,
+ ] if ctx.attr.requires_resources_dir else []) + ctx.attr.extra_args
+
+ template = remove_indentation("""
+ #!/bin/sh
+ $(rootpath {test_binary}) {test_args}
+ """)
+
+ template = ctx.expand_location(template.format(
+ test_binary = ctx.attr.test_binary.label,
+ test_args = " ".join(test_args),
+ ), targets = [
+ ctx.attr.test_binary,
+ ctx.attr._arbitrary_file_in_resources_dir,
+ ])
+
+ # https://bazel.build/rules/lib/builtins/actions.html#declare_file
+ output_file = ctx.actions.declare_file(ctx.attr.name)
+ ctx.actions.write(output_file, template, is_executable = True)
+
+ runfiles = ctx.runfiles(
+ files = ctx.files._resources_dir if ctx.attr.requires_resources_dir else [],
+ )
+ runfiles = runfiles.merge(ctx.attr.test_binary[DefaultInfo].default_runfiles)
+
+ return [DefaultInfo(
+ executable = output_file,
+ runfiles = runfiles,
+ )]
+
+skia_binary_with_cmdline_flags_test = rule(
+ doc = """Runs a C++ test binary with any necessary command-line flags.
+
+ This test rule produces a wrapper script that invokes a C++ test with a predetermined set of
+ command-line flags.
+
+ The reason why we use a custom rule rather than a genrule is that we wish to select() the
+ extra_args attribute based e.g. on the device under test and various build settings.
+ """,
+ implementation = _skia_binary_with_cmdline_flags_test_impl,
+ attrs = {
+ "test_binary": attr.label(
+ mandatory = True,
+ executable = True,
+ allow_single_file = True,
+ cfg = "target",
+ ),
+ "requires_resources_dir": attr.bool(
+ doc = (
+ "If true, the test will be passed flag --resourcePath <path to //resources dir>."
+ ),
+ ),
+ "extra_args": attr.string_list(
+ doc = "Any additional command-line arguments to pass to the C++ test binary.",
+ ),
+ "_resources_dir": attr.label(
+ doc = "This directory will optionally be added to the test's runfiles.",
+ default = Label("//resources"),
+ ),
+ "_arbitrary_file_in_resources_dir": attr.label(
+ doc = "Used to compute the --resourcePath flag.",
+ default = Label("//resources:README"),
+ allow_single_file = True,
+ ),
+ },
+ test = True,
+)
