Windows: add skeleton for Bash-less test wrapper

In this commit:

- Add the not-yet-documented
  `--windows_native_test_wrapper` flag. This flag
  will guard the new feature and allow the Bazel
  team to roll out the feature gradually, as well
  as to deprecate the old test execution mechanism
  gradually.

- Add a target for the native implementation of
  the test wrapper.

- Hook up the test action creator logic to use the
  new binary, and add a test to assert this.

See bazelbuild#5508

Change-Id: I980a1a88cfabd04ddcf8a5b26620f8f3255f77ef

src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java

@@ -165,6 +165,11 @@ public Object getDefault(AttributeMap rule) {
                   .nonconfigurable("policy decision: should be consistent across configurations"))
           .add(attr("args", STRING_LIST))
           // Input files for every test action
+          .add(
+              attr("$test_wrapper", LABEL)
+                  .cfg(HostTransition.INSTANCE)
+                  .singleArtifact()
+                  .value(env.getToolsLabel("//tools/test:test_wrapper")))
           .add(
               attr("$test_runtime", LABEL_LIST)
                   .cfg(HostTransition.INSTANCE)

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java

@@ -165,6 +165,12 @@ public static final RuleClass getTestBaseRule(String toolsRepository) {
                     "policy decision: this should be consistent across configurations"))
         .add(attr("args", STRING_LIST))
         // Input files for every test action
+        .add(
+            attr("$test_wrapper", LABEL)
+                .cfg(HostTransition.INSTANCE)
+                .singleArtifact()
+                .value(
+                    labelCache.getUnchecked(toolsRepository + "//tools/test:test_wrapper")))
         .add(
             attr("$test_runtime", LABEL_LIST)
                 .cfg(HostTransition.INSTANCE)

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -188,12 +188,19 @@ private TestParams createTestAction(int shards) {
     BuildConfiguration config = ruleContext.getConfiguration();
     AnalysisEnvironment env = ruleContext.getAnalysisEnvironment();
     ArtifactRoot root = config.getTestLogsDirectory(ruleContext.getRule().getRepository());
+    final boolean useWindowsNativeTestWrapper =
+        ruleContext
+            .getConfiguration()
+            .getFragment(TestConfiguration.class)
+            .getWindowsNativeTestWrapper();
 
     NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder();
     inputsBuilder.addTransitive(
         NestedSetBuilder.create(Order.STABLE_ORDER, runfilesSupport.getRunfilesMiddleman()));
-    NestedSet<Artifact> testRuntime = PrerequisiteArtifacts.nestedSet(
-        ruleContext, "$test_runtime", Mode.HOST);
+    NestedSet<Artifact> testRuntime =
+        useWindowsNativeTestWrapper
+            ? PrerequisiteArtifacts.nestedSet(ruleContext, "$test_wrapper", Mode.HOST)
+            : PrerequisiteArtifacts.nestedSet(ruleContext, "$test_runtime", Mode.HOST);
     inputsBuilder.addTransitive(testRuntime);
     TestTargetProperties testProperties = new TestTargetProperties(
         ruleContext, executionRequirements);
@@ -202,8 +209,11 @@ private TestParams createTestAction(int shards) {
     final boolean collectCodeCoverage = config.isCodeCoverageEnabled()
         && instrumentedFiles != null;
 
-    Artifact testSetupScript = ruleContext.getHostPrerequisiteArtifact("$test_setup_script");
-    inputsBuilder.add(testSetupScript);
+    Artifact testWrapper =
+        useWindowsNativeTestWrapper
+            ? ruleContext.getHostPrerequisiteArtifact("$test_wrapper")
+            : ruleContext.getHostPrerequisiteArtifact("$test_setup_script");
+    inputsBuilder.add(testWrapper);
     Artifact testXmlGeneratorScript =
         ruleContext.getHostPrerequisiteArtifact("$xml_generator_script");
     inputsBuilder.add(testXmlGeneratorScript);
@@ -309,7 +319,8 @@ private TestParams createTestAction(int shards) {
             new TestRunnerAction(
                 ruleContext.getActionOwner(),
                 inputs,
-                testSetupScript,
+                testWrapper,
+                useWindowsNativeTestWrapper,
                 testXmlGeneratorScript,
                 collectCoverageScript,
                 testLog,

src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java

@@ -216,6 +216,22 @@ public static class TestOptions extends FragmentOptions {
     )
     public Label coverageReportGenerator;
 
+    @Option(
+      name = "windows_native_test_wrapper",
+      // Undocumented: this features is under development and not yet ready for production use.
+      // We define the flag only in order to be able to test the feature.
+      // Design: https://github.com/laszlocsomor/proposals/blob/win-test-runner/designs/2018-07-18-windows-native-test-runner.md
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      // Affects loading and analysis: this flag affects which target Bazel loads and creates test
+      // actions with on Windows.
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      defaultValue = "false",
+      help =
+          "On Windows: if true, uses the C++ test wrapper to run tests, otherwise uses "
+              + "tools/test/test-setup.sh as on other platforms. On other platforms: no-op."
+    )
+    public boolean windowsNativeTestWrapper;
+
     @Override
     public Map<String, Set<Label>> getDefaultsLabels() {
       return ImmutableMap.<String, Set<Label>>of(
@@ -230,6 +246,7 @@ public FragmentOptions getHost() {
       // configuration.
       hostOptions.coverageSupport = this.coverageSupport;
       hostOptions.coverageReportGenerator = this.coverageReportGenerator;
+      hostOptions.windowsNativeTestWrapper = this.windowsNativeTestWrapper;
       return hostOptions;
     }
   }
@@ -298,6 +315,10 @@ public Label getCoverageReportGenerator(){
     return options.coverageReportGenerator;
   }
 
+  public boolean getWindowsNativeTestWrapper() {
+    return options.windowsNativeTestWrapper;
+  }
+
   /**
    * @return number of times the given test should run. If the test doesn't match any of the
    *     filters, runs it once.

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -72,7 +72,7 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
 
   private static final String GUID = "cc41f9d0-47a6-11e7-8726-eb6ce83a8cc8";
 
-  private final Artifact testSetupScript;
+  private final Artifact testWrapper;
   private final Artifact testXmlGeneratorScript;
   private final Artifact collectCoverageScript;
   private final BuildConfiguration configuration;
@@ -102,6 +102,9 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
   private final int runNumber;
   private final String workspaceName;
 
+  // Takes the value of the `--windows_native_test_wrapper` flag.
+  private final boolean useWindowsNativeTestWrapper;
+
   // Mutable state related to test caching. Lazily initialized: null indicates unknown.
   private Boolean unconditionalExecution;
 
@@ -135,7 +138,8 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
   TestRunnerAction(
       ActionOwner owner,
       Iterable<Artifact> inputs,
-      Artifact testSetupScript, // Must be in inputs
+      Artifact testWrapper, // Must be in inputs
+      boolean useWindowsNativeTestWrapper,
       Artifact testXmlGeneratorScript, // Must be in inputs
       @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null
       Artifact testLog,
@@ -158,7 +162,8 @@ private static ImmutableList<Artifact> list(Artifact... artifacts) {
         list(testLog, cacheStatus, coverageArtifact),
         configuration.getActionEnvironment());
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
-    this.testSetupScript = testSetupScript;
+    this.testWrapper = testWrapper;
+    this.useWindowsNativeTestWrapper = useWindowsNativeTestWrapper;
     this.testXmlGeneratorScript = testXmlGeneratorScript;
     this.collectCoverageScript = collectCoverageScript;
     this.configuration = Preconditions.checkNotNull(configuration);
@@ -740,8 +745,12 @@ public ImmutableSet<Artifact> getMandatoryOutputs() {
     return getOutputs();
   }
 
-  public Artifact getTestSetupScript() {
-    return testSetupScript;
+  public Artifact getTestWrapper() {
+    return testWrapper;
+  }
+
+  public boolean isUsingWindowsNativeTestWrapper() {
+    return useWindowsNativeTestWrapper;
   }
 
   public Artifact getTestXmlGeneratorScript() {

src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java

@@ -139,17 +139,22 @@ public abstract List<SpawnResult> exec(
    */
   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:
-    // testAction.getConfiguration().getTargetOS() == OS.WINDOWS
-    if (OS.getCurrent() == OS.WINDOWS) {
+    // TODO(ulfjack): Using OS.getCurrent() for `executedOnWindows` is incorrect in case of remote
+    // execution, where we should consider the target configuration, not the machine Bazel happens
+    // to run on. Change this to something like:
+    // executedOnWindows = testAction.getConfiguration().getTargetOS() == OS.WINDOWS
+    final boolean executedOnWindows = OS.getCurrent() == OS.WINDOWS;
+
+    final boolean useWindowsNativeTestWrapper = testAction.isUsingWindowsNativeTestWrapper();
+
+    if (executedOnWindows && !useWindowsNativeTestWrapper) {
       args.add(testAction.getShExecutable().getPathString());
       args.add("-c");
       args.add("$0 $*");
     }
 
-    Artifact testSetup = testAction.getTestSetupScript();
-    args.add(testSetup.getExecPath().getCallablePathString());
+    Artifact testWrapper = testAction.getTestWrapper();
+    args.add(testWrapper.getExecPath().getCallablePathString());
 
     if (testAction.isCoverageMode()) {
       args.add(testAction.getCollectCoverageScript().getExecPathString());
@@ -159,7 +164,7 @@ public static ImmutableList<String> getArgs(TestRunnerAction testAction) throws
 
     // Insert the command prefix specified by the "--run_under=<command-prefix>" option, if any.
     if (execSettings.getRunUnder() != null) {
-      addRunUnderArgs(testAction, args);
+      addRunUnderArgs(testAction, args, executedOnWindows);
     }
 
     // Execute the test using the alias in the runfiles tree, as mandated by the Test Encyclopedia.
@@ -172,7 +177,8 @@ public static ImmutableList<String> getArgs(TestRunnerAction testAction) throws
     return ImmutableList.copyOf(args);
   }
 
-  private static void addRunUnderArgs(TestRunnerAction testAction, List<String> args) {
+  private static void addRunUnderArgs(
+      TestRunnerAction testAction, List<String> args, boolean executedOnWindows) {
     TestTargetExecutionSettings execSettings = testAction.getExecutionSettings();
     if (execSettings.getRunUnderExecutable() != null) {
       args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getCallablePathString());
@@ -181,12 +187,14 @@ private static void addRunUnderArgs(TestRunnerAction testAction, List<String> ar
       // --run_under commands that do not contain '/' are either shell built-ins or need to be
       // located on the PATH env, so we wrap them in a shell invocation. Note that we shell tokenize
       // the --run_under parameter and getCommand only returns the first such token.
-      boolean needsShell = !command.contains("/");
+      boolean needsShell =
+          !command.contains("/") && (!executedOnWindows || !command.contains("\\"));
       if (needsShell) {
-        args.add(testAction.getShExecutable().getPathString());
+        String shellExecutable = testAction.getShExecutable().getPathString();
+        args.add(shellExecutable);
         args.add("-c");
         args.add("\"$@\"");
-        args.add("/bin/sh"); // Sets $0.
+        args.add(shellExecutable);  // Sets $0.
       }
       args.add(command);
     }

src/test/java/com/google/devtools/build/lib/exec/StandaloneTestStrategyTest.java

@@ -72,7 +72,7 @@ private static class TestedStandaloneTestStrategy extends StandaloneTestStrategy
 
     public TestedStandaloneTestStrategy(
         ExecutionOptions executionOptions, BinTools binTools, Path tmpDirRoot) {
-      super(executionOptions, binTools, tmpDirRoot);
+      super(executionOptions, binTools, tmpDirRoot, false);
     }
 
     @Override

src/test/py/bazel/BUILD

@@ -138,6 +138,18 @@ py_test(
     }),
 )
 
+py_test(
+    name = "test_wrapper_test",
+    srcs = select({
+        "//src/conditions:windows": ["test_wrapper_test.py"],
+        "//conditions:default": ["empty_test.py"],
+    }),
+    deps = select({
+        "//src/conditions:windows": [":test_base"],
+        "//conditions:default": [],
+    }),
+)
+
 py_test(
     name = "query_test",
     size = "medium",

src/test/py/bazel/test_wrapper_test.py

@@ -0,0 +1,64 @@
+# Copyright 2018 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import unittest
+from src.test.py.bazel import test_base
+
+
+class BazelCleanTest(test_base.TestBase):
+
+  def testTestExecutionWithTestSetupShAndWithTestWrapperExe(self):
+    self.ScratchFile('WORKSPACE')
+    self.ScratchFile('foo/BUILD', [
+        'py_test(',
+        '    name = "x_test",',
+        '    srcs = ["x_test.py"],',
+        ')',
+    ])
+    self.ScratchFile('foo/x_test.py', [
+        'from __future__ import print_function',
+        'import unittest',
+        '',
+        'class XTest(unittest.TestCase):',
+        '    def testFoo(self):',
+        '        print("lorem ipsum")',
+        '',
+        'if __name__ == "__main__":',
+        '  unittest.main()',
+    ], executable = True)
+
+    # Run test with test-setup.sh
+    exit_code, stdout, stderr = self.RunBazel([
+        'test', '//foo:x_test', '--test_output=streamed', '-t-',
+        '--nowindows_native_test_wrapper'])
+    self.AssertExitCode(exit_code, 0, stderr)
+    found = False
+    for line in stdout + stderr:
+        if 'lorem ipsum' in line:
+            found = True
+    if not found:
+        self.fail('FAIL: output:\n%s\n---' % '\n'.join(stderr + stdout))
+
+    # Run test with test_wrapper.exe
+    exit_code, _, stderr = self.RunBazel([
+        'test', '//foo:x_test', '--test_output=streamed', '-t-',
+        '--windows_native_test_wrapper'])
+
+    # As of 2018-08-07, test_wrapper.exe cannot yet run tests.
+    self.AssertExitCode(exit_code, 3, stderr)
+
+if __name__ == '__main__':
+  unittest.main()
+

tools/BUILD

@@ -66,7 +66,7 @@ filegroup(
         "//tools/objc:srcs",
         "//tools/python:embedded_tools",
         "//tools/runfiles:embedded_tools",
-        "//tools/test:srcs",
+        "//tools/test:embedded_tools",
         "//tools/test/LcovMerger/java/com/google/devtools/lcovmerger:embedded_tools",
         "//tools/osx/crosstool:srcs",
         "//tools/osx:srcs",

tools/test/BUILD

@@ -33,7 +33,33 @@ filegroup(
     srcs = ["@bazel_tools//tools/test/LcovMerger/java/com/google/devtools/lcovmerger:Main"],
 )
 
+cc_binary(
+    name = "test_wrapper",
+    srcs = select({
+        "@bazel_tools//src/conditions:windows": ["windows/test_wrapper.cc"],
+
+        # Dummy source file, so this rule can be built on non-Windows platforms
+        # (when building all targets in this package).
+        "//conditions:default": ["windows/dummy.cc"],
+    }),
+    visibility = ["//visibility:private"],
+)
+
 filegroup(
     name = "srcs",
     srcs = glob(["**"]),
 )
+
+filegroup(
+    name = "embedded_tools",
+    srcs = [
+        "BUILD.tools",
+        "test-setup.sh",
+        "generate-xml.sh",
+        "collect_coverage.sh",
+    ] + glob(["LcovMerger/**"]) + select({
+        "@bazel_tools//src/conditions:windows": ["test_wrapper"],
+        "//conditions:default": [],
+    }),
+    visibility = ["//tools:__pkg__"],
+)

tools/test/windows/dummy.cc

@@ -0,0 +1,27 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Empty implementation of the test wrapper.
+//
+// As of 2018-08-06, every platform uses //tools/test:test-setup.sh as the test
+// wrapper, and we are working on introducing a C++ test wrapper for Windows.
+// See https://github.com/laszlocsomor/proposals/blob/win-test-runner/designs/2018-07-18-windows-native-test-runner.md
+
+#include <stdio.h>
+
+int main(int, char**) {
+  fprintf(stderr,
+          __FILE__ ": The C++ test wrapper is not used on this platform.\n");
+  return 1;
+}