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 <akonradi@google.com>

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:

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

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")
+]

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)

tests/harness/executor/executor_test.sh

@@ -0,0 +1,8 @@
+#!/bin/bash
+#
+set -x
+
+EXECUTOR_BIN=$1
+shift
+
+$EXECUTOR_BIN "$@"