From 77fc08558ca2a3bfe082b2369c19fb25ccad001d Mon Sep 17 00:00:00 2001
From: Bo Zhang <bo@gradle.com>
Date: Mon, 15 Aug 2022 19:46:42 +0800
Subject: [PATCH] Report error instead of crash if test uses reserved env
 variable

This closes https://github.com/bazelbuild/bazel/issues/16094

Previously Bazel will crash if test uses reserved env variable
(like `TEST_NAME`). Let's report an error in this case.
---
 .../lib/exec/StandaloneTestStrategy.java      | 29 ++++++++++++-------
 src/test/shell/bazel/bazel_test_test.sh       | 18 ++++++++++++
 2 files changed, 37 insertions(+), 10 deletions(-)

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
index 09dbe442945bbd..584ca4409ea290 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -83,6 +83,7 @@
 /** Runs TestRunnerAction actions. */
 // TODO(bazel-team): add tests for this strategy.
 public class StandaloneTestStrategy extends TestStrategy {
+  private static final String TEST_NAME_ENV = "TEST_NAME";
   private static final ImmutableMap<String, String> ENV_VARS =
       ImmutableMap.<String, String>builder()
           .put("TZ", "UTC")
@@ -105,24 +106,32 @@ public StandaloneTestStrategy(
     this.tmpDirRoot = tmpDirRoot;
   }
 
+  private static TestExecException createTestExecException(String errorMessage) {
+    return new TestExecException(
+        errorMessage,
+        FailureDetail.newBuilder()
+            .setTestAction(
+                TestAction.newBuilder().setCode(TestAction.Code.LOCAL_TEST_PREREQ_UNMET))
+            .setMessage(errorMessage)
+            .build()
+    );
+  }
+
   @Override
   public TestRunnerSpawn createTestRunnerSpawn(
       TestRunnerAction action, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
     if (action.getExecutionSettings().getInputManifest() == null) {
-      String errorMessage = "cannot run local tests with --nobuild_runfile_manifests";
-      throw new TestExecException(
-          errorMessage,
-          FailureDetail.newBuilder()
-              .setTestAction(
-                  TestAction.newBuilder().setCode(TestAction.Code.LOCAL_TEST_PREREQ_UNMET))
-              .setMessage(errorMessage)
-              .build());
+      throw createTestExecException("cannot run local tests with --nobuild_runfile_manifests");
     }
     Map<String, String> testEnvironment =
         createEnvironment(
             actionExecutionContext, action, tmpDirRoot, executionOptions.splitXmlGeneration);
 
+    if (testEnvironment.containsKey(TEST_NAME_ENV)) {
+      throw createTestExecException(String.format("cannot set env variable TEST_NAME=%s because TEST_NAME is reserved", testEnvironment.get(TEST_NAME_ENV)));
+    }
+
     Map<String, String> executionInfo =
         new TreeMap<>(action.getTestProperties().getExecutionInfo());
     if (!action.shouldCacheResult()) {
@@ -582,7 +591,7 @@ private static Spawn createXmlGeneratingSpawn(
     // "PATH" and "TEST_BINARY" are also required, they should always be set in testEnv.
     Preconditions.checkArgument(testEnv.containsKey("PATH"));
     Preconditions.checkArgument(testEnv.containsKey("TEST_BINARY"));
-    envBuilder.putAll(testEnv).put("TEST_NAME", action.getTestName());
+    envBuilder.putAll(testEnv).put(TEST_NAME_ENV, action.getTestName());
     // testEnv only contains TEST_SHARD_INDEX and TEST_TOTAL_SHARDS if the test action is sharded,
     // we need to set the default value when the action isn't sharded.
     if (!action.isSharded()) {
@@ -630,7 +639,7 @@ private static Spawn createCoveragePostProcessingSpawn(
     testEnvironment.put("TEST_SHARD_INDEX", Integer.toString(action.getShardNum()));
     testEnvironment.put(
         "TEST_TOTAL_SHARDS", Integer.toString(action.getExecutionSettings().getTotalShards()));
-    testEnvironment.put("TEST_NAME", action.getTestName());
+    testEnvironment.put(TEST_NAME_ENV, action.getTestName());
     testEnvironment.put("IS_COVERAGE_SPAWN", "1");
     return new SimpleSpawn(
         action,
diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh
index 709530a3ae0a84..1100ddc720cc96 100755
--- a/src/test/shell/bazel/bazel_test_test.sh
+++ b/src/test/shell/bazel/bazel_test_test.sh
@@ -953,6 +953,24 @@ EOF
   expect_log "cannot run local tests with --nobuild_runfile_manifests"
 }
 
+function test_test_with_reserved_env_variable() {
+  mkdir -p dir
+
+  touch dir/test.sh
+  chmod u+x dir/test.sh
+  cat <<'EOF' > dir/BUILD
+sh_test(
+    name = 'test',
+    srcs = ['test.sh'],
+    env = {
+      "TEST_NAME": "foo"
+    },
+)
+EOF
+  bazel test //dir:test >& $TEST_log && fail "should have failed"
+  expect_log "cannot set env variable TEST_NAME=foo because TEST_NAME is reserved"
+}
+
 function test_run_from_external_repo_sibling_repository_layout() {
   cat <<EOF > WORKSPACE
 local_repository(