test command now adds 'default' output group if it is absent.

The --output_groups option determines which output groups are built for the
targets and aspects specified by command-line patterns, and artifacts in
other output groups do not affect the completion of those targets/aspects.

When testing is requested, the targets' executables in the 'default' output
group will be built even if the 'default' output group was not requested. The
executables will be built after the targets have reported their status however,
and if building the executable fails, the status will not be reported
correctly.

Now, 'test' commands will always include the 'default' output group during
the build phase, and failure to build the test executable will lead to the
target reporting a failure to build.

PiperOrigin-RevId: 371771843

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

@@ -218,13 +218,16 @@ public static OutputGroupInfo merge(List<OutputGroupInfo> providers)
   }
 
   public static ImmutableSortedSet<String> determineOutputGroups(
-      List<String> outputGroups, ValidationMode validationMode) {
-    return determineOutputGroups(DEFAULT_GROUPS, outputGroups, validationMode);
+      List<String> outputGroups, ValidationMode validationMode, boolean shouldRunTests) {
+    return determineOutputGroups(DEFAULT_GROUPS, outputGroups, validationMode, shouldRunTests);
   }
 
   @VisibleForTesting
   static ImmutableSortedSet<String> determineOutputGroups(
-      Set<String> defaultOutputGroups, List<String> outputGroups, ValidationMode validationMode) {
+      Set<String> defaultOutputGroups,
+      List<String> outputGroups,
+      ValidationMode validationMode,
+      boolean shouldRunTests) {
 
     Set<String> current = Sets.newHashSet();
 
@@ -264,6 +267,13 @@ static ImmutableSortedSet<String> determineOutputGroups(
       case OFF: // fall out
     }
 
+    // The `test` command ultimately requests artifacts from the `default` output group in order to
+    // execute the tests, so we should ensure these artifacts are requested by the targets for
+    // proper failure reporting.
+    if (shouldRunTests) {
+      current.add(DEFAULT);
+    }
+
     return ImmutableSortedSet.copyOf(current);
   }
 

src/main/java/com/google/devtools/build/lib/buildtool/BuildRequest.java

@@ -373,7 +373,8 @@ public TopLevelArtifactContext getTopLevelArtifactContext() {
         getOptions(ExecutionOptions.class).testStrategy.equals("exclusive"),
         getOptions(BuildEventProtocolOptions.class).expandFilesets,
         getOptions(BuildEventProtocolOptions.class).fullyResolveFilesetSymlinks,
-        OutputGroupInfo.determineOutputGroups(buildOptions.outputGroups, validationMode()));
+        OutputGroupInfo.determineOutputGroups(
+            buildOptions.outputGroups, validationMode(), /*shouldRunTests=*/ shouldRunTests()));
   }
 
   public ImmutableSortedSet<String> getMultiCpus() {

src/test/java/com/google/devtools/build/lib/analysis/BUILD

@@ -172,6 +172,7 @@ java_library(
         "//third_party:mockito",
         "//third_party:truth",
         "//third_party/protobuf:protobuf_java",
+        "@com_google_testparameterinjector//:testparameterinjector",
     ],
 )
 

src/test/java/com/google/devtools/build/lib/analysis/OutputGroupProviderTest.java

@@ -15,125 +15,212 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.analysis.OutputGroupInfo.DEFAULT;
 import static com.google.devtools.build.lib.analysis.OutputGroupInfo.determineOutputGroups;
 import static java.util.Arrays.asList;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo.ValidationMode;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import java.util.Set;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
 
-/**
- * Tests for {@link OutputGroupInfo}.
- */
-@RunWith(JUnit4.class)
+/** Tests for {@link OutputGroupInfo}. */
+@RunWith(TestParameterInjector.class)
 public final class OutputGroupProviderTest {
 
   @Test
-  public void testDetermineOutputGroupsOverridesDefaults() throws Exception {
+  public void testDetermineOutputGroupsOverridesDefaults(@TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("a", "b", "c"), ValidationMode.OFF);
-    assertThat(outputGroups).containsExactly("a", "b", "c");
+            ImmutableSet.of("x", "y", "z"),
+            asList("a", "b", "c"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(shouldRunTests, "a", "b", "c"));
   }
 
   @Test
-  public void testDetermineOutputGroupsAddsToDefaults() throws Exception {
+  public void testDetermineOutputGroupsAddsToDefaults(@TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
-        determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("+a"), ValidationMode.OFF);
-    assertThat(outputGroups).containsExactly("x", "y", "z", "a");
+        determineOutputGroups(
+            ImmutableSet.of("x", "y", "z"),
+            asList("+a"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(shouldRunTests, "x", "y", "z", "a"));
   }
 
   @Test
-  public void testDetermineOutputGroupsRemovesFromDefaults() throws Exception {
+  public void testDetermineOutputGroupsRemovesFromDefaults(@TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
-        determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-y"), ValidationMode.OFF);
-    assertThat(outputGroups).containsExactly("x", "z");
+        determineOutputGroups(
+            ImmutableSet.of("x", "y", "z"),
+            asList("-y"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(outputGroupsWithDefaultIfRunningTests(shouldRunTests, "x", "z"));
   }
 
   @Test
-  public void testDetermineOutputGroupsMixedOverrideAdditionOverrides() throws Exception {
+  public void testDetermineOutputGroupsMixedOverrideAdditionOverrides(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("a", "+b"), ValidationMode.OFF);
+            ImmutableSet.of("x", "y", "z"),
+            asList("a", "+b"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
     // The plain "a" causes the default output groups to be overridden.
-    assertThat(outputGroups).containsExactly("a", "b");
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(outputGroupsWithDefaultIfRunningTests(shouldRunTests, "a", "b"));
   }
 
   @Test
-  public void testDetermineOutputGroupsIgnoresUnknownGroup() throws Exception {
+  public void testDetermineOutputGroupsIgnoresUnknownGroup(@TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
-        determineOutputGroups(ImmutableSet.of("x", "y", "z"), asList("-foo"), ValidationMode.OFF);
+        determineOutputGroups(
+            ImmutableSet.of("x", "y", "z"),
+            asList("-foo"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
     // "foo" doesn't exist, but that shouldn't be a problem.
-    assertThat(outputGroups).containsExactly("x", "y", "z");
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(shouldRunTests, "x", "y", "z"));
   }
 
   @Test
-  public void testDetermineOutputGroupsRemovesPreviouslyAddedGroup() throws Exception {
+  public void testDetermineOutputGroupsRemovesPreviouslyAddedGroup(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups;
     outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("+a", "-a"), ValidationMode.OFF);
-    assertThat(outputGroups).containsExactly("x", "y", "z");
+            ImmutableSet.of("x", "y", "z"),
+            asList("+a", "-a"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(shouldRunTests, "x", "y", "z"));
 
     // Order matters here.
     outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("-a", "+a"), ValidationMode.OFF);
-    assertThat(outputGroups).containsExactly("x", "y", "z", "a");
+            ImmutableSet.of("x", "y", "z"),
+            asList("-a", "+a"),
+            ValidationMode.OFF,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(shouldRunTests, "x", "y", "z", "a"));
   }
 
   @Test
-  public void testDetermineOutputGroupsContainsValidationGroup() throws Exception {
+  public void testDetermineOutputGroupsContainsValidationGroup(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList(), ValidationMode.OUTPUT_GROUP);
-    assertThat(outputGroups).containsExactly("x", "y", "z", OutputGroupInfo.VALIDATION);
+            ImmutableSet.of("x", "y", "z"),
+            asList(),
+            ValidationMode.OUTPUT_GROUP,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(
+                shouldRunTests, "x", "y", "z", OutputGroupInfo.VALIDATION));
   }
 
   @Test
-  public void testDetermineOutputGroupsContainsValidationGroupAfterOverride() throws Exception {
+  public void testDetermineOutputGroupsContainsValidationGroupAfterOverride(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("foo"), ValidationMode.OUTPUT_GROUP);
-    assertThat(outputGroups).containsExactly("foo", OutputGroupInfo.VALIDATION);
+            ImmutableSet.of("x", "y", "z"),
+            asList("foo"),
+            ValidationMode.OUTPUT_GROUP,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(
+                shouldRunTests, "foo", OutputGroupInfo.VALIDATION));
   }
 
   @Test
-  public void testDetermineOutputGroupsContainsValidationGroupAfterAdd() throws Exception {
+  public void testDetermineOutputGroupsContainsValidationGroupAfterAdd(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("+a"), ValidationMode.OUTPUT_GROUP);
-    assertThat(outputGroups).containsExactly("x", "y", "z", "a", OutputGroupInfo.VALIDATION);
+            ImmutableSet.of("x", "y", "z"),
+            asList("+a"),
+            ValidationMode.OUTPUT_GROUP,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(
+                shouldRunTests, "x", "y", "z", "a", OutputGroupInfo.VALIDATION));
   }
 
   @Test
-  public void testDetermineOutputGroupsContainsValidationGroupAfterRemove() throws Exception {
+  public void testDetermineOutputGroupsContainsValidationGroupAfterRemove(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
-            ImmutableSet.of("x", "y", "z"), asList("-x"), ValidationMode.OUTPUT_GROUP);
-    assertThat(outputGroups).containsExactly("y", "z", OutputGroupInfo.VALIDATION);
+            ImmutableSet.of("x", "y", "z"),
+            asList("-x"),
+            ValidationMode.OUTPUT_GROUP,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(
+                shouldRunTests, "y", "z", OutputGroupInfo.VALIDATION));
   }
 
   @Test
-  public void testDetermineOutputGroupsContainsValidationGroupDespiteRemove() throws Exception {
+  public void testDetermineOutputGroupsContainsValidationGroupDespiteRemove(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
             ImmutableSet.of("x", "y", "z"),
             asList("-" + OutputGroupInfo.VALIDATION),
-            ValidationMode.OUTPUT_GROUP);
-    assertThat(outputGroups).containsExactly("x", "y", "z", OutputGroupInfo.VALIDATION);
+            ValidationMode.OUTPUT_GROUP,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(
+                shouldRunTests, "x", "y", "z", OutputGroupInfo.VALIDATION));
   }
 
   @Test
-  public void testDetermineOutputGroupsContainsTopLevelValidationGroup() throws Exception {
+  public void testDetermineOutputGroupsContainsTopLevelValidationGroup(
+      @TestParameter boolean shouldRunTests) {
     Set<String> outputGroups =
         determineOutputGroups(
             ImmutableSet.of("x", "y", "z"),
             asList("-" + OutputGroupInfo.VALIDATION_TOP_LEVEL),
-            ValidationMode.ASPECT);
-    assertThat(outputGroups).containsExactly("x", "y", "z", OutputGroupInfo.VALIDATION_TOP_LEVEL);
+            ValidationMode.ASPECT,
+            /*shouldRunTests=*/ shouldRunTests);
+    assertThat(outputGroups)
+        .containsExactlyElementsIn(
+            outputGroupsWithDefaultIfRunningTests(
+                shouldRunTests, "x", "y", "z", OutputGroupInfo.VALIDATION_TOP_LEVEL));
+  }
+
+  private static Iterable<String> outputGroupsWithDefaultIfRunningTests(
+      boolean shouldRunTests, String... groups) {
+    ImmutableList.Builder<String> result = ImmutableList.builder();
+    result.add(groups);
+    if (shouldRunTests) {
+      result.add(DEFAULT);
+    }
+    return result.build();
   }
 }

src/test/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorTest.java

@@ -1877,7 +1877,9 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
                         false,
                         false,
                         OutputGroupInfo.determineOutputGroups(
-                            ImmutableList.of(), OutputGroupInfo.ValidationMode.OUTPUT_GROUP)),
+                            ImmutableList.of(),
+                            OutputGroupInfo.ValidationMode.OUTPUT_GROUP,
+                            /*shouldRunTests=*/ false)),
                     /* trustRemoteArtifacts= */ false));
     // The catastrophic exception should be propagated into the BuildFailedException whether or not
     // --keep_going is set.

src/test/shell/integration/build_event_stream_test.sh

@@ -1089,6 +1089,21 @@ function test_test_fails_to_build() {
   expect_log_once '^build_tool_logs'
 }
 
+function test_test_fails_to_build_without_default_output_group() {
+  (bazel test --experimental_bep_target_summary \
+         --build_event_text_file=$TEST_log \
+         --output_groups=extra \
+         pkg:test_that_fails_to_build && fail "test failure expected") || true
+  expect_not_log 'test_summary'  # no test_summary events or references to them
+  expect_log_once '^target_summary '
+  expect_not_log 'overall_build_success'
+  expect_log 'last_message: true'
+  expect_log 'BUILD_FAILURE'
+  expect_log 'last_message: true'
+  expect_log 'command_line:.*This build will fail'
+  expect_log_once '^build_tool_logs'
+}
+
 function test_no_tests_found() {
   (bazel test --experimental_bep_target_summary \
          --build_event_text_file=$TEST_log \