Skip to content

Commit

Permalink
Make bazel coverage work with minimal mode
Browse files Browse the repository at this point in the history
This PR solves the problem in a different way that #16475 tries to solve:

1. #16812 allows skyframe read metadata from ActionFS.
2. Use `ActionFileSystem` to check existence of coverage data.
3. Fire event `CoverageReport` in the action after the coverage report is generated and listen to it in `ToplevelArtifactsDownloader` to download the report.

Closes #16556.

PiperOrigin-RevId: 502854552
Change-Id: I2796baaa962857831ff161423be6dffa6eb73e5c
  • Loading branch information
coeuvre authored and hvadehra committed Feb 14, 2023
1 parent b530baa commit c4f6c36
Show file tree
Hide file tree
Showing 8 changed files with 187 additions and 27 deletions.
10 changes: 10 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2518,6 +2518,16 @@ java_library(
],
)

java_library(
name = "test/coverage_report",
srcs = ["test/CoverageReport.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//third_party:guava",
],
)

java_library(
name = "test/coverage_configuration",
srcs = ["test/CoverageConfiguration.java"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright 2023 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.
package com.google.devtools.build.lib.analysis.test;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.vfs.Path;

/** This event is used to notify about a successfully generated coverage report. */
public final class CoverageReport implements ExtendedEventHandler.Postable {
private final ImmutableList<Path> files;

public CoverageReport(ImmutableList<Path> files) {
this.files = files;
}

public ImmutableList<Path> getFiles() {
return files;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report",
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/common/options",
"//third_party:auto_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.devtools.build.lib.bazel.coverage;

import static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand All @@ -30,6 +32,7 @@
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.BaseSpawn;
import com.google.devtools.build.lib.actions.ExecException;
Expand All @@ -45,6 +48,7 @@
import com.google.devtools.build.lib.analysis.RunfilesSupport;
import com.google.devtools.build.lib.analysis.actions.Compression;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.test.CoverageReport;
import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
import com.google.devtools.build.lib.analysis.test.TestProvider;
import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
Expand All @@ -57,6 +61,7 @@
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.exec.SpawnStrategyResolver;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -146,6 +151,12 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
.getContext(SpawnStrategyResolver.class)
.exec(spawn, actionExecutionContext);
actionExecutionContext.getEventHandler().handle(Event.info(locationMessage));
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
ImmutableList<Path> files =
getOutputs().stream()
.map(artifact -> pathResolver.convertPath(artifact.getPath()))
.collect(toImmutableList());
actionExecutionContext.getEventHandler().post(new CoverageReport(files));
return ActionResult.create(spawnResults);
} catch (ExecException e) {
throw ActionExecutionException.fromExecException(e, this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,10 @@ private TestAttemptResult runTestAttempt(

Verify.verify(
!(testAction.isCoverageMode() && testAction.getSplitCoveragePostProcessing())
|| testAction.getCoverageData().getPath().exists());
|| actionExecutionContext
.getPathResolver()
.convertPath(testAction.getCoverageData().getPath())
.exists());
Verify.verifyNotNull(spawnResults);
Verify.verifyNotNull(testResultDataBuilder);

Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/remote/util",
"//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.test.CoverageReport;
import com.google.devtools.build.lib.analysis.test.TestAttempt;
import com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher.Priority;
import com.google.devtools.build.lib.remote.util.StaticMetadataProvider;
Expand All @@ -56,7 +57,8 @@ private enum CommandMode {
UNKNOWN,
BUILD,
TEST,
RUN;
RUN,
COVERAGE;
}

private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
Expand All @@ -83,6 +85,9 @@ public ToplevelArtifactsDownloader(
case "run":
this.commandMode = CommandMode.RUN;
break;
case "coverage":
this.commandMode = CommandMode.COVERAGE;
break;
default:
this.commandMode = CommandMode.UNKNOWN;
}
Expand All @@ -104,36 +109,48 @@ public interface PathToMetadataConverter {
FileArtifactValue getMetadata(Path path);
}

private void downloadTestOutput(Path path) {
// Since the event is fired within action execution, the skyframe doesn't know the outputs of
// test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
// of the path is an ActionFileSystem, we use it to get the metadata for this file.
//
// If the test hit action cache, the filesystem is local filesystem because the actual test
// action didn't get the chance to execute. In this case the metadata is null which is fine
// because test outputs are already downloaded (otherwise it cannot hit the action cache).
FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path);
if (metadata != null) {
ListenableFuture<Void> future =
actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW);
addCallback(
future,
new FutureCallback<Void>() {
@Override
public void onSuccess(Void unused) {}

@Override
public void onFailure(Throwable throwable) {
logger.atWarning().withCause(throwable).log(
"Failed to download test output %s.", path);
}
},
directExecutor());
}
}

@Subscribe
@AllowConcurrentEvents
public void onTestAttempt(TestAttempt event) {
for (Pair<String, Path> pair : event.getFiles()) {
Path path = checkNotNull(pair.getSecond());
// Since the event is fired within action execution, the skyframe doesn't know the outputs of
// test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
// of the path is an ActionFileSystem, we use it to get the metadata for this file.
//
// If the test hit action cache, the filesystem is local filesystem because the actual test
// action didn't get the chance to execute. In this case the metadata is null which is fine
// because test outputs are already downloaded (otherwise it cannot hit the action cache).
FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path);
if (metadata != null) {
ListenableFuture<Void> future =
actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW);
addCallback(
future,
new FutureCallback<Void>() {
@Override
public void onSuccess(Void unused) {}
downloadTestOutput(path);
}
}

@Override
public void onFailure(Throwable throwable) {
logger.atWarning().withCause(throwable).log(
"Failed to download test output %s.", path);
}
},
directExecutor());
}
@Subscribe
@AllowConcurrentEvents
public void onCoverageReport(CoverageReport event) {
for (var file : event.getFiles()) {
downloadTestOutput(file);
}
}

Expand Down Expand Up @@ -172,8 +189,9 @@ private boolean shouldDownloadToplevelOutputs(ConfiguredTargetKey configuredTarg
case RUN:
// Always download outputs of toplevel targets in RUN mode
return true;
case COVERAGE:
case TEST:
// Do not download test binary in test mode.
// Do not download test binary in test/coverage mode.
try {
var configuredTargetValue =
(ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey);
Expand Down
84 changes: 84 additions & 0 deletions src/test/shell/bazel/remote/build_without_the_bytes_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1583,4 +1583,88 @@ EOF
expect_log "bin-message"
}

function test_java_rbe_coverage_produces_report() {
mkdir -p java/factorial

JAVA_TOOLS_ZIP="released"
COVERAGE_GENERATOR_DIR="released"

cd java/factorial

cat > BUILD <<'EOF'
java_library(
name = "fact",
srcs = ["Factorial.java"],
)
java_test(
name = "fact-test",
size = "small",
srcs = ["FactorialTest.java"],
test_class = "factorial.FactorialTest",
deps = [
":fact",
],
)
EOF

cat > Factorial.java <<'EOF'
package factorial;
public class Factorial {
public static int factorial(int x) {
return x <= 0 ? 1 : x * factorial(x-1);
}
}
EOF

cat > FactorialTest.java <<'EOF'
package factorial;
import static org.junit.Assert.*;
import org.junit.Test;
public class FactorialTest {
@Test
public void testFactorialOfZeroIsOne() throws Exception {
assertEquals(Factorial.factorial(3),6);
}
}
EOF
cd ../..

cat $(rlocation io_bazel/src/test/shell/bazel/testdata/jdk_http_archives) >> WORKSPACE

bazel coverage \
--test_output=all \
--experimental_fetch_all_coverage_outputs \
--experimental_split_coverage_postprocessing \
--remote_download_minimal \
--combined_report=lcov \
--spawn_strategy=remote \
--remote_executor=grpc://localhost:${worker_port} \
--instrumentation_filter=//java/factorial \
//java/factorial:fact-test &> $TEST_log || fail "Shouldn't fail"

# Test binary shouldn't be downloaded
[[ ! -e "bazel-bin/java/factorial/libfact.jar" ]] || fail "bazel-bin/java/factorial/libfact.jar shouldn't exist!"

local expected_result="SF:java/factorial/Factorial.java
FN:2,factorial/Factorial::<init> ()V
FN:4,factorial/Factorial::factorial (I)I
FNDA:0,factorial/Factorial::<init> ()V
FNDA:1,factorial/Factorial::factorial (I)I
FNF:2
FNH:1
BRDA:4,0,0,1
BRDA:4,0,1,1
BRF:2
BRH:2
DA:2,0
DA:4,1
LH:1
LF:2
end_of_record"
cat bazel-testlogs/java/factorial/fact-test/coverage.dat > $TEST_log
expect_log "$expected_result"
cat bazel-out/_coverage/_coverage_report.dat > $TEST_log
expect_log "$expected_result"
}

run_suite "Build without the Bytes tests"

0 comments on commit c4f6c36

Please sign in to comment.