New approach

bazelbuild · Apr 27, 2023 · 1745813 · 1745813
1 parent 2c32643
commit 1745813
Show file tree

Hide file tree

Showing 5 changed files with 91 additions and 4 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
@@ -291,6 +291,17 @@ public static class TestOptions extends FragmentOptions {
                 + "the test exec group.")
     public boolean useTargetPlatformForTests;
 
+    @Option(
+        name = "incompatible_check_sharding_support",
+        defaultValue = "false",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+        effectTags = {OptionEffectTag.UNKNOWN},
+        help =
+            "If true, Bazel will fail a sharded test if the test runner does not indicate that it "
+                + "supports sharding by touching the file at the path in TEST_SHARD_STATUS_FILE.")
+    public boolean checkShardingSupport;
+
     @Override
     public FragmentOptions getExec() {
       // Options here are either:
@@ -395,6 +406,10 @@ public boolean useTargetPlatformForTests() {
     return options.useTargetPlatformForTests;
   }
 
+  public boolean checkShardingSupport() {
+    return options.checkShardingSupport;
+  }
+
   /**
    * Option converter that han handle two styles of value for "--runs_per_test":
    *

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -332,6 +332,10 @@ public boolean showsOutputUnconditionally() {
     return true;
   }
 
+  public boolean checkShardingSupport() {
+    return testConfiguration.checkShardingSupport();
+  }
+
   public List<ActionInput> getSpawnOutputs() {
     final List<ActionInput> outputs = new ArrayList<>();
     outputs.add(ActionInputHelper.fromPath(getXmlOutputPath()));

diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -684,6 +684,22 @@ private TestAttemptResult runTestAttempt(
     }
     long endTimeMillis = actionExecutionContext.getClock().currentTimeMillis();
 
+    if (testAction.isSharded()) {
+      if (testAction.checkShardingSupport() && !actionExecutionContext.getPathResolver()
+          .convertPath(resolvedPaths.getTestShard()).exists()) {
+        TestExecException e =
+            createTestExecException(
+                TestAction.Code.LOCAL_TEST_PREREQ_UNMET,
+                "Sharding requested, but the test runner did not advertise support for it by "
+                    + "touching TEST_SHARD_STATUS_FILE. Either remove the 'shard_count' attribute, "
+                    + "use a test runner that supports sharding or temporarily disable this check "
+                    + "via --noincompatible_check_sharding_support.");
+        closeSuppressed(e, streamed);
+        closeSuppressed(e, fileOutErr);
+        throw e;
+      }
+    }
+
     // SpawnActionContext guarantees the first entry to correspond to the spawn passed in (there
     // may be additional entries due to tree artifact handling).
     SpawnResult primaryResult = spawnResults.get(0);

diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh
@@ -1008,13 +1008,15 @@ EOF
   touch x.sh
   chmod +x x.sh
 
-  bazel test //:x --test_output=errors &> $TEST_log \
-      && fail "expected failure"
+  bazel test \
+      --incompatible_check_sharding_support \
+      //:x  &> $TEST_log && fail "expected failure"
   expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE."
 
   echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh
-  bazel test //:x --test_output=errors &> $TEST_log \
-      || fail "expected success"
+  bazel test \
+      --incompatible_check_sharding_support \
+      //:x  &> $TEST_log || fail "expected success"
 }
 
 run_suite "bazel test tests"
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -3012,4 +3012,54 @@ function test_external_cc_binary_tool_with_dynamic_deps_sibling_repository_layou
       @other_repo//pkg:rule >& $TEST_log || fail "Build should succeed"
 }
 
+function test_shard_status_file_checked() {
+  cat <<'EOF' > BUILD
+sh_test(
+    name = 'x',
+    srcs = ['x.sh'],
+    shard_count = 2,
+)
+EOF
+  touch x.sh
+  chmod +x x.sh
+
+  bazel test \
+      --remote_executor=grpc://localhost:${worker_port} \
+      --incompatible_check_sharding_support \
+      //:x  &> $TEST_log && fail "expected failure"
+  expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE."
+
+  echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh
+  bazel test \
+      --remote_executor=grpc://localhost:${worker_port} \
+      --incompatible_check_sharding_support \
+      //:x  &> $TEST_log || fail "expected success"
+}
+
+function test_shard_status_file_checked_remote_download_minimal() {
+  cat <<'EOF' > BUILD
+sh_test(
+    name = 'x',
+    srcs = ['x.sh'],
+    shard_count = 2,
+)
+EOF
+  touch x.sh
+  chmod +x x.sh
+
+  bazel test \
+      --remote_executor=grpc://localhost:${worker_port} \
+      --incompatible_check_sharding_support \
+      --remote_download_minimal \
+      //:x  &> $TEST_log && fail "expected failure"
+  expect_log "Sharding requested, but the test runner did not advertise support for it by touching TEST_SHARD_STATUS_FILE."
+
+  echo 'touch "$TEST_SHARD_STATUS_FILE"' > x.sh
+  bazel test \
+      --remote_executor=grpc://localhost:${worker_port} \
+      --incompatible_check_sharding_support \
+      --remote_download_minimal \
+      //:x  &> $TEST_log || fail "expected success"
+}
+
 run_suite "Remote execution and remote cache tests"