Remove hard-wired basenames for the test setup and the coverage collection script.

Accomplished by creating an explicit attribute for both of them on test actions.

RELNOTES: None.
PiperOrigin-RevId: 185132460

10 files changed, 56 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index 2292c9d857..5ea8584bbe 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -138,6 +138,14 @@ public class BaseRuleClasses {
               attr("$test_runtime", LABEL_LIST)
                   .cfg(HostTransition.INSTANCE)
                   .value(ImmutableList.of(env.getToolsLabel("//tools/test:runtime"))))
+          .add(attr("$test_setup_script", LABEL)
+              .cfg(HostTransition.INSTANCE)
+              .singleArtifact()
+              .value(env.getToolsLabel("//tools/test:test_setup")))
+          .add(attr("$collect_coverage_script", LABEL)
+              .cfg(HostTransition.INSTANCE)
+              .singleArtifact()
+              .value(env.getToolsLabel("//tools/test:collect_coverage")))
           // Input files for test actions collecting code coverage
           .add(
               attr("$coverage_support", LABEL)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index 7a83ac553b..6f7a1bfd62 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -190,6 +190,14 @@ public class SkylarkRuleClassFunctions {
                 .value(
                     ImmutableList.of(
                         labelCache.getUnchecked(toolsRepository + "//tools/test:runtime"))))
+        .add(attr("$test_setup_script", LABEL)
+            .cfg(HostTransition.INSTANCE)
+            .singleArtifact()
+            .value(labelCache.getUnchecked(toolsRepository + "//tools/test:test_setup")))
+        .add(attr("$collect_coverage_script", LABEL)
+            .cfg(HostTransition.INSTANCE)
+            .singleArtifact()
+            .value(labelCache.getUnchecked(toolsRepository + "//tools/test:collect_coverage")))
         // Input files for test actions collecting code coverage
         .add(
             attr("$coverage_support", LABEL)
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
index 37cfbbe39f..bbce349638 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -202,10 +202,16 @@ public final class TestActionBuilder {
     final boolean collectCodeCoverage = config.isCodeCoverageEnabled()
         && instrumentedFiles != null;
 
+    Artifact testSetupScript = ruleContext.getHostPrerequisiteArtifact("$test_setup_script");
+    inputsBuilder.add(testSetupScript);
+
+    Artifact collectCoverageScript = null;
     TreeMap<String, String> extraTestEnv = new TreeMap<>();
 
     TestTargetExecutionSettings executionSettings;
     if (collectCodeCoverage) {
+      collectCoverageScript = ruleContext.getHostPrerequisiteArtifact("$collect_coverage_script");
+      inputsBuilder.add(collectCoverageScript);
       inputsBuilder.addTransitive(instrumentedFiles.getCoverageSupportFiles());
       // Add instrumented file manifest artifact to the list of inputs. This file will contain
       // exec paths of all source files that should be included into the code coverage output.
@@ -301,7 +307,8 @@ public final class TestActionBuilder {
         }
 
         env.registerAction(new TestRunnerAction(
-            ruleContext.getActionOwner(), inputs, testRuntime,
+            ruleContext.getActionOwner(), inputs,
+            testSetupScript, collectCoverageScript,
             testLog, cacheStatus,
             coverageArtifact,
             testProperties, extraTestEnv, executionSettings,
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
index 8a7661f443..0166545c99 100644
--- 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
@@ -32,13 +32,11 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
-import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.ImmutableIterable;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -71,7 +69,8 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
 
   private static final String GUID = "cc41f9d0-47a6-11e7-8726-eb6ce83a8cc8";
 
-  private final NestedSet<Artifact> runtime;
+  private final Artifact testSetupScript;
+  private final Artifact collectCoverageScript;
   private final BuildConfiguration configuration;
   private final TestConfiguration testConfiguration;
   private final Artifact testLog;
@@ -133,7 +132,8 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   TestRunnerAction(
       ActionOwner owner,
       Iterable<Artifact> inputs,
-      NestedSet<Artifact> runtime,   // Must be a subset of inputs
+      Artifact testSetupScript,  // Must be in inputs
+      @Nullable Artifact collectCoverageScript,  // Must be in inputs, if not null
       Artifact testLog,
       Artifact cacheStatus,
       Artifact coverageArtifact,
@@ -151,7 +151,9 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
         // Note that this action only cares about the runfiles, not the mapping.
         new RunfilesSupplierImpl(PathFragment.create("runfiles"), executionSettings.getRunfiles()),
         list(testLog, cacheStatus, coverageArtifact));
-    this.runtime = runtime;
+    Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
+    this.testSetupScript = testSetupScript;
+    this.collectCoverageScript = collectCoverageScript;
     this.configuration = Preconditions.checkNotNull(configuration);
     this.testConfiguration =
         Preconditions.checkNotNull(configuration.getFragment(TestConfiguration.class));
@@ -693,14 +695,12 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     return getOutputs();
   }
 
-  public Artifact getRuntimeArtifact(String basename) throws ExecException {
-    for (Artifact runtimeArtifact : runtime) {
-      if (runtimeArtifact.getExecPath().getBaseName().equals(basename)) {
-        return runtimeArtifact;
-      }
-    }
+  public Artifact getTestSetupScript() {
+    return testSetupScript;
+  }
 
-    throw new UserExecException("'" + basename + "' not found in test runtime");
+  @Nullable public Artifact getCollectCoverageScript() {
+    return collectCoverageScript;
   }
 
   public PathFragment getShExecutable() {
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 a6fbd41fd8..b5748ee847 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
@@ -53,15 +53,12 @@ import java.util.List;
 import java.util.Map;
 
 /** Runs TestRunnerAction actions. */
+// TODO(bazel-team): add tests for this strategy.
 @ExecutionStrategy(
   contextType = TestActionContext.class,
   name = {"standalone"}
 )
 public class StandaloneTestStrategy extends TestStrategy {
-  // TODO(bazel-team) - add tests for this strategy.
-  public static final String COLLECT_COVERAGE =
-      "external/bazel_tools/tools/test/collect_coverage.sh";
-
   private static final ImmutableMap<String, String> ENV_VARS =
       ImmutableMap.<String, String>builder()
           .put("TZ", "UTC")
@@ -121,7 +118,7 @@ public class StandaloneTestStrategy extends TestStrategy {
     Spawn spawn =
         new SimpleSpawn(
             action,
-            getArgs(COLLECT_COVERAGE, action),
+            getArgs(action),
             ImmutableMap.copyOf(env),
             executionInfo.build(),
             new RunfilesSupplierImpl(
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index 3b3e6914cb..0e0dab07ed 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -156,14 +156,11 @@ public abstract class TestStrategy implements TestActionContext {
    * Generates a command line to run for the test action, taking into account coverage and {@code
    * --run_under} settings.
    *
-   * @param coverageScript a script interjected between setup script and rest of command line to
-   *     collect coverage data. If this is an empty string, it is ignored.
    * @param testAction The test action.
    * @return the command line as string list.
    * @throws ExecException 
    */
-  public static ImmutableList<String> getArgs(String coverageScript, TestRunnerAction testAction)
-      throws ExecException {
+  public static ImmutableList<String> getArgs(TestRunnerAction testAction) throws ExecException {
     List<String> args = Lists.newArrayList();
     // TODO(ulfjack): This is incorrect for remote execution, where we need to consider the target
     // configuration, not the machine Bazel happens to run on. Change this to something like:
@@ -174,11 +171,11 @@ public abstract class TestStrategy implements TestActionContext {
       args.add("$0 $*");
     }
 
-    Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME);
+    Artifact testSetup = testAction.getTestSetupScript();
     args.add(testSetup.getExecPath().getCallablePathString());
 
     if (testAction.isCoverageMode()) {
-      args.add(coverageScript);
+      args.add(testAction.getCollectCoverageScript().getExecPathString());
     }
 
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index af7a541fb3..5c50705231 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -461,7 +461,7 @@ public class RunCommand implements BlazeCommand  {
         // It's unfortunate that this method requires the path to the coverage collection script.
         // Fortunately, this is "blaze run" and not "blaze coverage", so it's okay unless someone
         // calls "blaze run --collect_code_coverage".
-        cmdLine = TestStrategy.getArgs("dummy-coverage-script", testAction);
+        cmdLine = TestStrategy.getArgs(testAction);
       } catch (ExecException e) {
         env.getReporter().handle(Event.error(e.getMessage()));
         return BlazeCommandResult.exitCode(ExitCode.LOCAL_ENVIRONMENTAL_ERROR);
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
index 274c953484..0fb173cf75 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java
@@ -242,7 +242,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy {
   }
 
   private List<String> getStartUpArgs(TestRunnerAction action) throws ExecException {
-    List<String> args = getArgs(/*coverageScript=*/ "coverage-is-not-supported", action);
+    List<String> args = getArgs(action);
     ImmutableList.Builder<String> startupArgs = ImmutableList.builder();
     // Add test setup with no echo to prevent stdout corruption.
     startupArgs.add(args.get(0)).add("--no_echo");
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
index eacc3f609e..2d68367ffa 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -125,7 +125,9 @@ public final class BazelAnalysisMock extends AnalysisMock {
         "/bazel_tools_workspace/tools/genrule/BUILD", "exports_files(['genrule-setup.sh'])");
 
     config.create("/bazel_tools_workspace/tools/test/BUILD",
-        "filegroup(name = 'runtime', srcs = ['test-setup.sh'],)",
+        "filegroup(name = 'runtime', srcs = ['test-setup.sh'])",
+        "filegroup(name = 'test_setup', srcs = ['test-setup.sh'])",
+        "filegroup(name = 'collect_coverage', srcs = ['collect_coverage.sh'])",
         "filegroup(name='coverage_support', srcs=['collect_coverage.sh','LcovMerger'])",
         "filegroup(name = 'coverage_report_generator', srcs = ['coverage_report_generator.sh'])");
 
diff --git a/tools/test/BUILD b/tools/test/BUILD
index 5b9dd5e5af..bc70912a07 100644
--- a/tools/test/BUILD
+++ b/tools/test/BUILD
@@ -7,6 +7,16 @@ filegroup(
     srcs = ["test-setup.sh"],
 )
 
+filegroup(
+    name = "test_setup",
+    srcs = ["test-setup.sh"],
+)
+
+filegroup(
+    name = "collect_coverage",
+    srcs = ["collect_coverage.sh"],
+)
+
 java_binary(
     name = "LcovMerger",
     srcs = glob(["LcovMerger/java/**/*.java"]),