From 12afc6dcd71f00ea08deee4df84173aed4cc7f97 Mon Sep 17 00:00:00 2001 From: Alex Konradi Date: Thu, 11 Mar 2021 22:31:36 -0500 Subject: [PATCH] Improve bazel test experience (#440) Make it possible to test all functionality using bazel test //tests/... instead of having to use bazel run with a target name and arguments, and speed up test execution on multi-core machines by sharding first by validator language and then by splitting the test cases for each language among multiple parallel invocations of the test executor using standard Bazel sharding infrastructure. As a side effect of running bazel test //tests/... in CI, we're now running some tests that were previously skipped, like //tests/harness/cc:cc_diamond_test. See https://docs.bazel.build/versions/master/test-encyclopedia.html#test-sharding for details on Bazel sharding. Signed-off-by: Alex Konradi --- Makefile | 10 +++--- README.md | 2 +- tests/harness/executor/BUILD | 17 ++++++++++ tests/harness/executor/executor.go | 41 +++++++++++++++++++++++-- tests/harness/executor/executor_test.sh | 8 +++++ 5 files changed, 69 insertions(+), 9 deletions(-) create mode 100755 tests/harness/executor/executor_test.sh diff --git a/Makefile b/Makefile index 4c8720016..d064bbfe4 100644 --- a/Makefile +++ b/Makefile @@ -58,10 +58,10 @@ harness: testcases tests/harness/go/harness.pb.go tests/harness/go/main/go-harne # runs the test harness, validating a series of test cases in all supported languages ./bin/harness -go -cc -.PHONY: bazel-harness -bazel-harness: - # runs the test harness via bazel - bazel run //tests/harness/executor:executor --incompatible_new_actions_api=false -- -go -cc -java -python +.PHONY: bazel-tests +bazel-tests: + # Runs all tests with Bazel + bazel test //tests/... .PHONY: testcases testcases: bin/protoc-gen-go @@ -116,7 +116,7 @@ tests/harness/java/java-harness: mvn -q -f java/pom.xml clean package -DskipTests .PHONY: ci -ci: lint bazel testcases bazel-harness build_generation_tests +ci: lint bazel testcases bazel-tests build_generation_tests .PHONY: clean clean: diff --git a/README.md b/README.md index 489ac9c0b..5da6ad3a2 100644 --- a/README.md +++ b/README.md @@ -859,7 +859,7 @@ All PGV dependencies are currently checked into the project. To test PGV, `proto Ensure that your `PATH` is setup to include `protoc-gen-go` and `protoc`, then: ``` -bazel run //tests/harness/executor:executor +bazel test //tests/... ``` ### Docker diff --git a/tests/harness/executor/BUILD b/tests/harness/executor/BUILD index 568aa5e3d..eeed9134a 100644 --- a/tests/harness/executor/BUILD +++ b/tests/harness/executor/BUILD @@ -45,3 +45,20 @@ go_binary( importpath = "github.com/envoyproxy/protoc-gen-validate/tests/harness/executor", visibility = ["//visibility:private"], ) + +[ + sh_test( + name = "executor_" + lang + "_test", + srcs = ["executor_test.sh"], + args = [ + "$(location :executor)", + "-" + lang, + ], + data = [":executor"], + # This could be sharded more, but each shard incurs overhead and test + # execution is already sharded by having separate test rules for each language. + shard_count = 5, + deps = ["@bazel_tools//tools/bash/runfiles"], + ) + for lang in ("cc", "java", "python") +] diff --git a/tests/harness/executor/executor.go b/tests/harness/executor/executor.go index e4023c0de..461039ef4 100644 --- a/tests/harness/executor/executor.go +++ b/tests/harness/executor/executor.go @@ -3,8 +3,10 @@ package main import ( "flag" "log" + "math" "os" "runtime" + "strconv" "sync" "sync/atomic" "time" @@ -23,9 +25,10 @@ func main() { externalHarnessFlag := flag.String("external_harness", "", "Path to a binary to be executed as an external test harness") flag.Parse() + test_cases := shardTestCases(TestCases) start := time.Now() harnesses := Harnesses(*goFlag, *ccFlag, *javaFlag, *pythonFlag, *externalHarnessFlag) - successes, failures, skips := run(*parallelism, harnesses) + successes, failures, skips := run(*parallelism, harnesses, test_cases) log.Printf("Successes: %d | Failures: %d | Skips: %d (%v)", successes, failures, skips, time.Since(start)) @@ -35,7 +38,39 @@ func main() { } } -func run(parallelism int, harnesses []Harness) (successes, failures, skips uint64) { +func shardTestCases(test_cases []TestCase) []TestCase { + // Support Bazel test sharding by slicing up the list of test cases if requested. + shard_count, err := strconv.Atoi(os.Getenv("TEST_TOTAL_SHARDS")) + if err != nil { + return test_cases + } + + shard_index, err := strconv.Atoi(os.Getenv("TEST_SHARD_INDEX")) + if err != nil { + return test_cases + } + + // Bazel expects that the test will create or modify the file with the provided name to show that it supports sharding. + status_file := os.Getenv("TEST_SHARD_STATUS_FILE") + if status_file == "" { + return test_cases + } + + if file, err := os.Create(status_file); err != nil { + return test_cases + } else { + file.Close() + } + shard_length := int(math.Ceil(float64(len(test_cases)) / float64(shard_count))) + shard_start := shard_index * shard_length + shard_end := (shard_index + 1) * shard_length + if shard_end >= len(test_cases) { + shard_end = len(test_cases) + } + return test_cases[shard_start:shard_end] +} + +func run(parallelism int, harnesses []Harness, test_cases []TestCase) (successes, failures, skips uint64) { wg := new(sync.WaitGroup) if parallelism <= 0 { panic("Parallelism must be > 0") @@ -66,7 +101,7 @@ func run(parallelism int, harnesses []Harness) (successes, failures, skips uint6 close(done) }() - for _, test := range TestCases { + for _, test := range test_cases { in <- test } close(in) diff --git a/tests/harness/executor/executor_test.sh b/tests/harness/executor/executor_test.sh new file mode 100755 index 000000000..b876cd0a3 --- /dev/null +++ b/tests/harness/executor/executor_test.sh @@ -0,0 +1,8 @@ +#!/bin/bash +# +set -x + +EXECUTOR_BIN=$1 +shift + +$EXECUTOR_BIN "$@"