Skip to content

Commit 27bc896

Browse files
coeuvreShreeM01
coeuvre
and
ShreeM01
authored
[6.1.0] Make bazel coverage work with minimal mode (bazelbuild#17397)
* Returns null if filesystem of test outputs is not ActionFS when processing test attempt event. Previously, we assert that the filesystem of test outputs is ActionFS when we are processing test attempt event. However this is not true when the test hits action cache. This CL looses the check to return null. PiperOrigin-RevId: 501023752 Change-Id: I17cbb26e0a2b5fd30cb781818e42172ac672919e * Make `bazel coverage` work with minimal mode This PR solves the problem in a different way that bazelbuild#16475 tries to solve: 1. bazelbuild#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 bazelbuild#16556. PiperOrigin-RevId: 502854552 Change-Id: I2796baaa962857831ff161423be6dffa6eb73e5c --------- Co-authored-by: kshyanashree <[email protected]>
1 parent 0759081 commit 27bc896

File tree

9 files changed

+192
-28
lines changed

9 files changed

+192
-28
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/BUILD‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2527,6 +2527,16 @@ java_library(
25272527
],
25282528
)
25292529

2530+
java_library(
2531+
name = "test/coverage_report",
2532+
srcs = ["test/CoverageReport.java"],
2533+
deps = [
2534+
"//src/main/java/com/google/devtools/build/lib/events",
2535+
"//src/main/java/com/google/devtools/build/lib/vfs",
2536+
"//third_party:guava",
2537+
],
2538+
)
2539+
25302540
java_library(
25312541
name = "test/coverage_report_action_factory",
25322542
srcs = ["test/CoverageReportActionFactory.java"],

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright 2023 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.analysis.test;
15+
16+
import com.google.common.collect.ImmutableList;
17+
import com.google.devtools.build.lib.events.ExtendedEventHandler;
18+
import com.google.devtools.build.lib.vfs.Path;
19+
20+
/** This event is used to notify about a successfully generated coverage report. */
21+
public final class CoverageReport implements ExtendedEventHandler.Postable {
22+
private final ImmutableList<Path> files;
23+
24+
public CoverageReport(ImmutableList<Path> files) {
25+
this.files = files;
26+
}
27+
28+
public ImmutableList<Path> getFiles() {
29+
return files;
30+
}
31+
}

‎src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,14 @@ java_library(
2525
"//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
2626
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
2727
"//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider",
28+
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report",
2829
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
2930
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
3031
"//src/main/java/com/google/devtools/build/lib/concurrent",
3132
"//src/main/java/com/google/devtools/build/lib/events",
3233
"//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver",
3334
"//src/main/java/com/google/devtools/build/lib/util",
35+
"//src/main/java/com/google/devtools/build/lib/vfs",
3436
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
3537
"//src/main/java/com/google/devtools/common/options",
3638
"//third_party:auto_value",

‎src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

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

17+
import static com.google.common.collect.ImmutableList.toImmutableList;
18+
1719
import com.google.common.base.Joiner;
1820
import com.google.common.collect.ImmutableList;
1921
import com.google.common.collect.ImmutableMap;
@@ -30,6 +32,7 @@
3032
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
3133
import com.google.devtools.build.lib.actions.ArtifactFactory;
3234
import com.google.devtools.build.lib.actions.ArtifactOwner;
35+
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
3336
import com.google.devtools.build.lib.actions.ArtifactRoot;
3437
import com.google.devtools.build.lib.actions.BaseSpawn;
3538
import com.google.devtools.build.lib.actions.ExecException;
@@ -45,6 +48,7 @@
4548
import com.google.devtools.build.lib.analysis.RunfilesSupport;
4649
import com.google.devtools.build.lib.analysis.actions.Compression;
4750
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
51+
import com.google.devtools.build.lib.analysis.test.CoverageReport;
4852
import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
4953
import com.google.devtools.build.lib.analysis.test.TestProvider;
5054
import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
@@ -57,6 +61,7 @@
5761
import com.google.devtools.build.lib.events.EventHandler;
5862
import com.google.devtools.build.lib.exec.SpawnStrategyResolver;
5963
import com.google.devtools.build.lib.util.Fingerprint;
64+
import com.google.devtools.build.lib.vfs.Path;
6065
import com.google.devtools.build.lib.vfs.PathFragment;
6166
import java.util.ArrayList;
6267
import java.util.Collection;
@@ -146,6 +151,12 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
146151
.getContext(SpawnStrategyResolver.class)
147152
.exec(spawn, actionExecutionContext);
148153
actionExecutionContext.getEventHandler().handle(Event.info(locationMessage));
154+
ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
155+
ImmutableList<Path> files =
156+
getOutputs().stream()
157+
.map(artifact -> pathResolver.convertPath(artifact.getPath()))
158+
.collect(toImmutableList());
159+
actionExecutionContext.getEventHandler().post(new CoverageReport(files));
149160
return ActionResult.create(spawnResults);
150161
} catch (ExecException e) {
151162
throw ActionExecutionException.fromExecException(e, this);

‎src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -827,7 +827,10 @@ public TestAttemptContinuation execute()
827827

828828
Verify.verify(
829829
!(testAction.isCoverageMode() && testAction.getSplitCoveragePostProcessing())
830-
|| testAction.getCoverageData().getPath().exists());
830+
|| actionExecutionContext
831+
.getPathResolver()
832+
.convertPath(testAction.getCoverageData().getPath())
833+
.exists());
831834
Verify.verifyNotNull(spawnResults);
832835
Verify.verifyNotNull(testResultDataBuilder);
833836

‎src/main/java/com/google/devtools/build/lib/remote/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ java_library(
201201
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
202202
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
203203
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
204+
"//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report",
204205
"//src/main/java/com/google/devtools/build/lib/packages",
205206
"//src/main/java/com/google/devtools/build/lib/remote/util",
206207
"//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",

‎src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -930,11 +930,11 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
930930
actionInputFetcher,
931931
(path) -> {
932932
FileSystem fileSystem = path.getFileSystem();
933-
Preconditions.checkState(
934-
fileSystem instanceof RemoteActionFileSystem,
935-
"fileSystem must be an instance of RemoteActionFileSystem");
936-
return ((RemoteActionFileSystem) path.getFileSystem())
937-
.getRemoteMetadata(path.asFragment());
933+
if (fileSystem instanceof RemoteActionFileSystem) {
934+
return ((RemoteActionFileSystem) path.getFileSystem())
935+
.getRemoteMetadata(path.asFragment());
936+
}
937+
return null;
938938
});
939939
env.getEventBus().register(toplevelArtifactsDownloader);
940940
}

‎src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java‎

Lines changed: 44 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
3636
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
3737
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
38+
import com.google.devtools.build.lib.analysis.test.CoverageReport;
3839
import com.google.devtools.build.lib.analysis.test.TestAttempt;
3940
import com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher.Priority;
4041
import com.google.devtools.build.lib.remote.util.StaticMetadataProvider;
@@ -56,7 +57,8 @@ private enum CommandMode {
5657
UNKNOWN,
5758
BUILD,
5859
TEST,
59-
RUN;
60+
RUN,
61+
COVERAGE;
6062
}
6163

6264
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
@@ -83,6 +85,9 @@ public ToplevelArtifactsDownloader(
8385
case "run":
8486
this.commandMode = CommandMode.RUN;
8587
break;
88+
case "coverage":
89+
this.commandMode = CommandMode.COVERAGE;
90+
break;
8691
default:
8792
this.commandMode = CommandMode.UNKNOWN;
8893
}
@@ -104,32 +109,48 @@ public interface PathToMetadataConverter {
104109
FileArtifactValue getMetadata(Path path);
105110
}
106111

112+
private void downloadTestOutput(Path path) {
113+
// Since the event is fired within action execution, the skyframe doesn't know the outputs of
114+
// test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
115+
// of the path is an ActionFileSystem, we use it to get the metadata for this file.
116+
//
117+
// If the test hit action cache, the filesystem is local filesystem because the actual test
118+
// action didn't get the chance to execute. In this case the metadata is null which is fine
119+
// because test outputs are already downloaded (otherwise it cannot hit the action cache).
120+
FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path);
121+
if (metadata != null) {
122+
ListenableFuture<Void> future =
123+
actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW);
124+
addCallback(
125+
future,
126+
new FutureCallback<Void>() {
127+
@Override
128+
public void onSuccess(Void unused) {}
129+
130+
@Override
131+
public void onFailure(Throwable throwable) {
132+
logger.atWarning().withCause(throwable).log(
133+
"Failed to download test output %s.", path);
134+
}
135+
},
136+
directExecutor());
137+
}
138+
}
139+
107140
@Subscribe
108141
@AllowConcurrentEvents
109142
public void onTestAttempt(TestAttempt event) {
110143
for (Pair<String, Path> pair : event.getFiles()) {
111144
Path path = checkNotNull(pair.getSecond());
112-
// Since the event is fired within action execution, the skyframe doesn't know the outputs of
113-
// test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
114-
// of the path is an ActionFileSystem, we use it to get the metadata for this file.
115-
FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path);
116-
if (metadata != null) {
117-
ListenableFuture<Void> future =
118-
actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW);
119-
addCallback(
120-
future,
121-
new FutureCallback<Void>() {
122-
@Override
123-
public void onSuccess(Void unused) {}
145+
downloadTestOutput(path);
146+
}
147+
}
124148

125-
@Override
126-
public void onFailure(Throwable throwable) {
127-
logger.atWarning().withCause(throwable).log(
128-
"Failed to download test output %s.", path);
129-
}
130-
},
131-
directExecutor());
132-
}
149+
@Subscribe
150+
@AllowConcurrentEvents
151+
public void onCoverageReport(CoverageReport event) {
152+
for (var file : event.getFiles()) {
153+
downloadTestOutput(file);
133154
}
134155
}
135156

@@ -168,8 +189,9 @@ private boolean shouldDownloadToplevelOutputs(ConfiguredTargetKey configuredTarg
168189
case RUN:
169190
// Always download outputs of toplevel targets in RUN mode
170191
return true;
192+
case COVERAGE:
171193
case TEST:
172-
// Do not download test binary in test mode.
194+
// Do not download test binary in test/coverage mode.
173195
try {
174196
var configuredTargetValue =
175197
(ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey);

‎src/test/shell/bazel/remote/build_without_the_bytes_test.sh‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,4 +1363,88 @@ EOF
13631363
expect_log "bin-message"
13641364
}
13651365

1366+
function test_java_rbe_coverage_produces_report() {
1367+
mkdir -p java/factorial
1368+
1369+
JAVA_TOOLS_ZIP="released"
1370+
COVERAGE_GENERATOR_DIR="released"
1371+
1372+
cd java/factorial
1373+
1374+
cat > BUILD <<'EOF'
1375+
java_library(
1376+
name = "fact",
1377+
srcs = ["Factorial.java"],
1378+
)
1379+
java_test(
1380+
name = "fact-test",
1381+
size = "small",
1382+
srcs = ["FactorialTest.java"],
1383+
test_class = "factorial.FactorialTest",
1384+
deps = [
1385+
":fact",
1386+
],
1387+
)
1388+
EOF
1389+
1390+
cat > Factorial.java <<'EOF'
1391+
package factorial;
1392+
public class Factorial {
1393+
public static int factorial(int x) {
1394+
return x <= 0 ? 1 : x * factorial(x-1);
1395+
}
1396+
}
1397+
EOF
1398+
1399+
cat > FactorialTest.java <<'EOF'
1400+
package factorial;
1401+
import static org.junit.Assert.*;
1402+
import org.junit.Test;
1403+
public class FactorialTest {
1404+
@Test
1405+
public void testFactorialOfZeroIsOne() throws Exception {
1406+
assertEquals(Factorial.factorial(3),6);
1407+
}
1408+
}
1409+
EOF
1410+
cd ../..
1411+
1412+
cat $(rlocation io_bazel/src/test/shell/bazel/testdata/jdk_http_archives) >> WORKSPACE
1413+
1414+
bazel coverage \
1415+
--test_output=all \
1416+
--experimental_fetch_all_coverage_outputs \
1417+
--experimental_split_coverage_postprocessing \
1418+
--remote_download_minimal \
1419+
--combined_report=lcov \
1420+
--spawn_strategy=remote \
1421+
--remote_executor=grpc://localhost:${worker_port} \
1422+
--instrumentation_filter=//java/factorial \
1423+
//java/factorial:fact-test &> $TEST_log || fail "Shouldn't fail"
1424+
1425+
# Test binary shouldn't be downloaded
1426+
[[ ! -e "bazel-bin/java/factorial/libfact.jar" ]] || fail "bazel-bin/java/factorial/libfact.jar shouldn't exist!"
1427+
1428+
local expected_result="SF:java/factorial/Factorial.java
1429+
FN:2,factorial/Factorial::<init> ()V
1430+
FN:4,factorial/Factorial::factorial (I)I
1431+
FNDA:0,factorial/Factorial::<init> ()V
1432+
FNDA:1,factorial/Factorial::factorial (I)I
1433+
FNF:2
1434+
FNH:1
1435+
BRDA:4,0,0,1
1436+
BRDA:4,0,1,1
1437+
BRF:2
1438+
BRH:2
1439+
DA:2,0
1440+
DA:4,1
1441+
LH:1
1442+
LF:2
1443+
end_of_record"
1444+
cat bazel-testlogs/java/factorial/fact-test/coverage.dat > $TEST_log
1445+
expect_log "$expected_result"
1446+
cat bazel-out/_coverage/_coverage_report.dat > $TEST_log
1447+
expect_log "$expected_result"
1448+
}
1449+
13661450
run_suite "Build without the Bytes tests"

0 commit comments

Comments
 (0)