Fix null pointer crash with bazel coverage on only incompatible tests

philsc · copybara-github · commit 3dd2b932d42f · 2022-06-10T04:17:57.000-07:00
This is a follow-up to 2f1ff6f. That patch accidentally introduced a crash when running coverage on tests where all tests are incompatible. FATAL: bazel crashed due to an internal error. Printing stack trace: java.lang.NullPointerException: Null reportGenerator at com.google.devtools.build.lib.bazel.coverage.AutoValue_CoverageArgs.<init>(AutoValue_CoverageArgs.java:68) at com.google.devtools.build.lib.bazel.coverage.CoverageArgs.create(CoverageArgs.java:58) at com.google.devtools.build.lib.bazel.coverage.CoverageReportActionBuilder.createCoverageActionsWrapper(CoverageReportActionBuilder.java:226) at com.google.devtools.build.lib.bazel.coverage.BazelCoverageReportModule$1.createCoverageReportActionsWrapper(BazelCoverageReportModule.java:98) at com.google.devtools.build.lib.analysis.BuildView.createResult(BuildView.java:558) at com.google.devtools.build.lib.analysis.BuildView.update(BuildView.java:492) at com.google.devtools.build.lib.buildtool.AnalysisPhaseRunner.runAnalysisPhase(AnalysisPhaseRunner.java:227) at com.google.devtools.build.lib.buildtool.AnalysisPhaseRunner.execute(AnalysisPhaseRunner.java:137) at com.google.devtools.build.lib.buildtool.BuildTool.buildTargets(BuildTool.java:266) at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:506) at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:474) at com.google.devtools.build.lib.runtime.commands.TestCommand.doTest(TestCommand.java:148) at com.google.devtools.build.lib.runtime.commands.TestCommand.exec(TestCommand.java:113) at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.execExclusively(BlazeCommandDispatcher.java:584) at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:231) at com.google.devtools.build.lib.server.GrpcServerImpl.executeCommand(GrpcServerImpl.java:550) at com.google.devtools.build.lib.server.GrpcServerImpl.lambda$run$1(GrpcServerImpl.java:614) at io.grpc.Context$1.run(Context.java:566) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) at java.base/java.lang.Thread.run(Unknown Source) This patch fixes the crash by handling the situation properly. This results in all tests being skipped and reported as being skipped. A new test validates the correct behaviour. Closes bazelbuild#15645. PiperOrigin-RevId: 454132787 Change-Id: Id1cd4109f96d90bbdc114b0bbe7f5c5046d47c27
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java
@@ -212,6 +212,10 @@ public CoverageReportActionsWrapper createCoverageActionsWrapper(
         reportGenerator = testParams.getCoverageReportGenerator();
       }
     }
+    // If all tests are incompatible, there's nothing to do.
+    if (reportGenerator == null) {
+      return null;
+    }
     builder.addAll(baselineCoverageArtifacts.toList());
 
     ImmutableList<Artifact> coverageArtifacts = builder.build();
diff --git a/src/test/shell/bazel/bazel_coverage_compatibility_test.sh b/src/test/shell/bazel/bazel_coverage_compatibility_test.sh
@@ -28,6 +28,7 @@ constraint_setting(name = "incompatible_setting")
 constraint_value(
     name = "incompatible",
     constraint_setting = ":incompatible_setting",
+    visibility = ["//visibility:public"],
 )
 
 sh_test(
@@ -40,6 +41,11 @@ sh_test(
     srcs = ["incompatible_test.sh"],
     target_compatible_with = [":incompatible"],
 )
+
+exports_files([
+    "compatible_test.sh",
+    "incompatible_test.sh",
+])
 EOF
   cat <<EOF > compatible_test.sh
 #!/bin/bash
@@ -51,17 +57,44 @@ exit 1
 EOF
   chmod +x compatible_test.sh
   chmod +x incompatible_test.sh
+
+  mkdir all_incompatible/
+  cat <<EOF > all_incompatible/BUILD
+sh_test(
+    name = "incompatible1_test",
+    srcs = ["//:incompatible_test.sh"],
+    target_compatible_with = ["//:incompatible"],
+)
+
+sh_test(
+    name = "incompatible2_test",
+    srcs = ["//:incompatible_test.sh"],
+    target_compatible_with = ["//:incompatible"],
+)
+EOF
 }
 
-# Validates that coverage skips incompatible tests.  This is a regression test for
-# https://github.com/bazelbuild/bazel/issues/15385.
+# Validates that coverage skips incompatible tests.  This is a regression test
+# for https://github.com/bazelbuild/bazel/issues/15385.
 function test_sh_test_coverage() {
   set_up_sh_test_coverage
   bazel coverage --test_output=all --combined_report=lcov //:all &>$TEST_log \
     || fail "Coverage for //:all failed"
   expect_log "INFO: Build completed successfully"
   expect_log "//:compatible_test .* PASSED"
   expect_log "//:incompatible_test .* SKIPPED"
+  expect_log "Executed 1 out of 2 tests: 1 were skipped"
+}
+
+# Validates that coverage correctly handles all tests being incompatible.
+function test_sh_test_coverage() {
+  set_up_sh_test_coverage
+  bazel coverage --test_output=all --combined_report=lcov //all_incompatible:all &>$TEST_log \
+    || fail "Coverage for //:all failed"
+  expect_log "INFO: Build completed successfully"
+  expect_log "//all_incompatible:incompatible1_test .* SKIPPED"
+  expect_log "//all_incompatible:incompatible2_test .* SKIPPED"
+  expect_log "Executed 0 out of 2 tests: 2 were skipped"
 }
 
 run_suite "test tests"

Original file line number	Diff line number	Diff line change
`@@ -212,6 +212,10 @@ public CoverageReportActionsWrapper createCoverageActionsWrapper(`
`212`	`212`	`reportGenerator = testParams.getCoverageReportGenerator();`
`213`	`213`	`}`
`214`	`214`	`}`
	`215`	`+ // If all tests are incompatible, there's nothing to do.`
	`216`	`+ if (reportGenerator == null) {`
	`217`	`+ return null;`
	`218`	`+ }`
`215`	`219`	`builder.addAll(baselineCoverageArtifacts.toList());`
`216`	`220`
`217`	`221`	`ImmutableList<Artifact> coverageArtifacts = builder.build();`