Skip to content

Commit 4f89f43

Browse files
tjgqcopybara-github
tjgq
authored and
copybara-github
committed
Return a multimap instead of a list of pairs from TestRunnerAction#getTestOutputsMapping.
Cleanup CL in preparation for fixing bazelbuild#17911. PiperOrigin-RevId: 671735596 Change-Id: I2f477d4d49fd298895a59ef88fc98d72cad1f974
1 parent dcc3669 commit 4f89f43

File tree

7 files changed

+64
-73
lines changed

7 files changed

+64
-73
lines changed

‎src/main/java/com/google/devtools/build/lib/actions/ImportantOutputHandler.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
import com.google.devtools.build.lib.vfs.Path;
2323
import com.google.devtools.build.lib.vfs.PathFragment;
2424
import java.time.Duration;
25-
import java.util.List;
25+
import java.util.Collection;
2626
import java.util.Map;
2727

2828
/** Context to be informed of top-level outputs and their runfiles. */
@@ -82,7 +82,7 @@ LostArtifacts processRunfilesAndGetLostArtifacts(
8282
* Actions#dependsOnBuildId}), so outputs passed to this method come from a just-executed test
8383
* action.
8484
*/
85-
void processTestOutputs(List<Path> testOutputs)
85+
void processTestOutputs(Collection<Path> testOutputs)
8686
throws ImportantOutputException, InterruptedException;
8787

8888
/**

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

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.google.common.annotations.VisibleForTesting;
1919
import com.google.common.base.Preconditions;
2020
import com.google.common.collect.ImmutableList;
21+
import com.google.common.collect.ImmutableMultimap;
2122
import com.google.devtools.build.lib.actions.Artifact;
2223
import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
2324
import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
@@ -29,14 +30,14 @@
2930
import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
3031
import com.google.devtools.build.lib.buildeventstream.PathConverter;
3132
import com.google.devtools.build.lib.runtime.BuildEventStreamerUtils;
32-
import com.google.devtools.build.lib.util.Pair;
3333
import com.google.devtools.build.lib.vfs.Path;
3434
import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
3535
import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
3636
import com.google.protobuf.util.Durations;
3737
import com.google.protobuf.util.Timestamps;
3838
import java.util.Collection;
3939
import java.util.List;
40+
import java.util.Map;
4041

4142
/** This event is raised whenever an individual test attempt is completed. */
4243
public class TestAttempt implements BuildEventWithOrderConstraint {
@@ -47,7 +48,7 @@ public class TestAttempt implements BuildEventWithOrderConstraint {
4748
private final boolean cachedLocally;
4849
private final int attempt;
4950
private final boolean lastAttempt;
50-
private final Collection<Pair<String, Path>> files;
51+
private final ImmutableMultimap<String, Path> files;
5152
private final List<String> testWarnings;
5253
private final long durationMillis;
5354
private final long startTimeMillis;
@@ -69,7 +70,7 @@ private TestAttempt(
6970
String statusDetails,
7071
long startTimeMillis,
7172
long durationMillis,
72-
Collection<Pair<String, Path>> files,
73+
ImmutableMultimap<String, Path> files,
7374
List<String> testWarnings,
7475
boolean lastAttempt) {
7576
this.testAction = testAction;
@@ -93,7 +94,7 @@ public static TestAttempt forExecutedTestResult(
9394
TestRunnerAction testAction,
9495
TestResultData attemptData,
9596
int attempt,
96-
Collection<Pair<String, Path>> files,
97+
ImmutableMultimap<String, Path> files,
9798
BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
9899
boolean lastAttempt) {
99100
return new TestAttempt(
@@ -118,7 +119,7 @@ public static TestAttempt fromCachedTestResult(
118119
TestRunnerAction testAction,
119120
TestResultData attemptData,
120121
int attempt,
121-
Collection<Pair<String, Path>> files,
122+
ImmutableMultimap<String, Path> files,
122123
BuildEventStreamProtos.TestResult.ExecutionInfo executionInfo,
123124
boolean lastAttempt) {
124125
return new TestAttempt(
@@ -153,7 +154,7 @@ public static TestAttempt forUnstartableTestResult(
153154
attemptData.getStatusDetails(),
154155
attemptData.getStartTimeMillisEpoch(),
155156
attemptData.getRunDurationMillis(),
156-
/* files= */ ImmutableList.of(),
157+
/* files= */ ImmutableMultimap.of(),
157158
attemptData.getWarningList(),
158159
/* lastAttempt= */ true);
159160
}
@@ -164,7 +165,7 @@ public Artifact getTestStatusArtifact() {
164165
}
165166

166167
@VisibleForTesting
167-
public Collection<Pair<String, Path>> getFiles() {
168+
public ImmutableMultimap<String, Path> getFiles() {
168169
return files;
169170
}
170171

@@ -227,12 +228,12 @@ public ImmutableList<LocalFile> referencedLocalFiles() {
227228
? LocalFileType.SUCCESSFUL_TEST_OUTPUT
228229
: LocalFileType.FAILED_TEST_OUTPUT;
229230
ImmutableList.Builder<LocalFile> localFiles = ImmutableList.builder();
230-
for (Pair<String, Path> file : files) {
231-
if (file.getSecond() != null) {
231+
for (Map.Entry<String, Path> file : files.entries()) {
232+
if (file.getValue() != null) {
232233
// TODO(b/199940216): Can we populate metadata for these files?
233234
localFiles.add(
234235
new LocalFile(
235-
file.getSecond(),
236+
file.getValue(),
236237
localFileType,
237238
/* artifact= */ null,
238239
/* artifactMetadata= */ null));
@@ -264,11 +265,11 @@ public BuildEventStreamProtos.TestResult asTestResult(BuildEventContext converte
264265
}
265266
builder.setTestAttemptDurationMillis(durationMillis);
266267
builder.addAllWarning(testWarnings);
267-
for (Pair<String, Path> file : files) {
268-
String uri = pathConverter.apply(file.getSecond());
268+
for (Map.Entry<String, Path> file : files.entries()) {
269+
String uri = pathConverter.apply(file.getValue());
269270
if (uri != null) {
270271
builder.addTestActionOutput(
271-
BuildEventStreamProtos.File.newBuilder().setName(file.getFirst()).setUri(uri).build());
272+
BuildEventStreamProtos.File.newBuilder().setName(file.getKey()).setUri(uri).build());
272273
}
273274
}
274275
return builder.build();

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import com.google.common.base.Preconditions;
2020
import com.google.common.collect.ImmutableList;
21+
import com.google.common.collect.ImmutableMultimap;
2122
import com.google.devtools.build.lib.actions.Artifact;
2223
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
2324
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -26,12 +27,10 @@
2627
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
2728
import com.google.devtools.build.lib.events.ExtendedEventHandler;
2829
import com.google.devtools.build.lib.util.DetailedExitCode;
29-
import com.google.devtools.build.lib.util.Pair;
3030
import com.google.devtools.build.lib.vfs.FileSystem;
3131
import com.google.devtools.build.lib.vfs.Path;
3232
import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
3333
import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
34-
import java.util.Collection;
3534
import java.util.List;
3635
import javax.annotation.Nullable;
3736

@@ -183,7 +182,7 @@ public DetailedExitCode getSystemFailure() {
183182
* Returns the collection of files created by the test, tagged by their name indicating usage
184183
* (e.g., "test.log").
185184
*/
186-
private Collection<Pair<String, Path>> getFiles() {
185+
private ImmutableMultimap<String, Path> getFiles() {
187186
// TODO(ulfjack): Cache the set of generated files in the TestResultData.
188187
return testAction.getTestOutputsMapping(ArtifactPathResolver.forExecRoot(execRoot), execRoot);
189188
}

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

Lines changed: 29 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@
2222
import com.google.common.annotations.VisibleForTesting;
2323
import com.google.common.base.Preconditions;
2424
import com.google.common.base.Strings;
25-
import com.google.common.collect.ImmutableList;
2625
import com.google.common.collect.ImmutableMap;
26+
import com.google.common.collect.ImmutableMultimap;
2727
import com.google.common.collect.ImmutableSet;
2828
import com.google.common.collect.Iterators;
2929
import com.google.common.flogger.GoogleLogger;
@@ -65,7 +65,6 @@
6565
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
6666
import com.google.devtools.build.lib.util.DetailedExitCode;
6767
import com.google.devtools.build.lib.util.Fingerprint;
68-
import com.google.devtools.build.lib.util.Pair;
6968
import com.google.devtools.build.lib.vfs.FileSystemUtils;
7069
import com.google.devtools.build.lib.vfs.Path;
7170
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -401,73 +400,64 @@ public List<ActionInput> getSpawnOutputs() {
401400
* file system for existence of these output files, so it must only be used after test execution.
402401
*/
403402
// TODO(ulfjack): Instead of going to local disk here, use SpawnResult (add list of files there).
404-
public ImmutableList<Pair<String, Path>> getTestOutputsMapping(
403+
public ImmutableMultimap<String, Path> getTestOutputsMapping(
405404
ArtifactPathResolver resolver, Path execRoot) {
406-
ImmutableList.Builder<Pair<String, Path>> builder = ImmutableList.builder();
405+
ImmutableMultimap.Builder<String, Path> builder = ImmutableMultimap.builder();
407406
if (resolver.toPath(getTestLog()).exists()) {
408-
builder.add(Pair.of(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog())));
407+
builder.put(TestFileNameConstants.TEST_LOG, resolver.toPath(getTestLog()));
409408
}
410409
if (getCoverageData() != null && resolver.toPath(getCoverageData()).exists()) {
411-
builder.add(Pair.of(TestFileNameConstants.TEST_COVERAGE, resolver.toPath(getCoverageData())));
410+
builder.put(TestFileNameConstants.TEST_COVERAGE, resolver.toPath(getCoverageData()));
412411
}
413412
if (execRoot != null) {
414413
ResolvedPaths resolvedPaths = resolve(execRoot);
415414
if (resolvedPaths.getTestStderr().exists()) {
416-
builder.add(Pair.of(TestFileNameConstants.TEST_STDERR, resolvedPaths.getTestStderr()));
415+
builder.put(TestFileNameConstants.TEST_STDERR, resolvedPaths.getTestStderr());
417416
}
418417
if (resolvedPaths.getXmlOutputPath().exists()) {
419-
builder.add(Pair.of(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath()));
418+
builder.put(TestFileNameConstants.TEST_XML, resolvedPaths.getXmlOutputPath());
420419
}
421420
if (resolvedPaths.getSplitLogsPath().exists()) {
422-
builder.add(Pair.of(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath()));
421+
builder.put(TestFileNameConstants.SPLIT_LOGS, resolvedPaths.getSplitLogsPath());
423422
}
424423
if (resolvedPaths.getTestWarningsPath().exists()) {
425-
builder.add(
426-
Pair.of(TestFileNameConstants.TEST_WARNINGS, resolvedPaths.getTestWarningsPath()));
424+
builder.put(TestFileNameConstants.TEST_WARNINGS, resolvedPaths.getTestWarningsPath());
427425
}
428426
if (testConfiguration.getZipUndeclaredTestOutputs()
429427
&& resolvedPaths.getUndeclaredOutputsZipPath().exists()) {
430-
builder.add(
431-
Pair.of(
432-
TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
433-
resolvedPaths.getUndeclaredOutputsZipPath()));
428+
builder.put(
429+
TestFileNameConstants.UNDECLARED_OUTPUTS_ZIP,
430+
resolvedPaths.getUndeclaredOutputsZipPath());
434431
}
435432
if (!testConfiguration.getZipUndeclaredTestOutputs()
436433
&& resolvedPaths.getUndeclaredOutputsDir().exists()) {
437-
builder.add(
438-
Pair.of(
439-
TestFileNameConstants.UNDECLARED_OUTPUTS_DIR,
440-
resolvedPaths.getUndeclaredOutputsDir()));
434+
435+
builder.put(
436+
TestFileNameConstants.UNDECLARED_OUTPUTS_DIR, resolvedPaths.getUndeclaredOutputsDir());
441437
}
442438
if (resolvedPaths.getUndeclaredOutputsManifestPath().exists()) {
443-
builder.add(
444-
Pair.of(
445-
TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
446-
resolvedPaths.getUndeclaredOutputsManifestPath()));
439+
builder.put(
440+
TestFileNameConstants.UNDECLARED_OUTPUTS_MANIFEST,
441+
resolvedPaths.getUndeclaredOutputsManifestPath());
447442
}
448443
if (resolvedPaths.getUndeclaredOutputsAnnotationsPath().exists()) {
449-
builder.add(
450-
Pair.of(
451-
TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
452-
resolvedPaths.getUndeclaredOutputsAnnotationsPath()));
444+
builder.put(
445+
TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS,
446+
resolvedPaths.getUndeclaredOutputsAnnotationsPath());
453447
}
454448
if (resolvedPaths.getUndeclaredOutputsAnnotationsPbPath().exists()) {
455-
builder.add(
456-
Pair.of(
457-
TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB,
458-
resolvedPaths.getUndeclaredOutputsAnnotationsPbPath()));
449+
builder.put(
450+
TestFileNameConstants.UNDECLARED_OUTPUTS_ANNOTATIONS_PB,
451+
resolvedPaths.getUndeclaredOutputsAnnotationsPbPath());
459452
}
460453
if (resolvedPaths.getUnusedRunfilesLogPath().exists()) {
461-
builder.add(
462-
Pair.of(
463-
TestFileNameConstants.UNUSED_RUNFILES_LOG,
464-
resolvedPaths.getUnusedRunfilesLogPath()));
454+
builder.put(
455+
TestFileNameConstants.UNUSED_RUNFILES_LOG, resolvedPaths.getUnusedRunfilesLogPath());
465456
}
466457
if (resolvedPaths.getInfrastructureFailureFile().exists()) {
467-
builder.add(
468-
Pair.of(
469-
TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
470-
resolvedPaths.getInfrastructureFailureFile()));
458+
builder.put(
459+
TestFileNameConstants.TEST_INFRASTRUCTURE_FAILURE,
460+
resolvedPaths.getInfrastructureFailureFile());
471461
}
472462
}
473463
return builder.build();

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

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import com.google.common.base.Verify;
2020
import com.google.common.collect.ImmutableList;
2121
import com.google.common.collect.ImmutableMap;
22+
import com.google.common.collect.ImmutableMultimap;
2223
import com.google.common.collect.ImmutableSet;
2324
import com.google.common.collect.Maps;
2425
import com.google.common.io.ByteStreams;
@@ -51,7 +52,6 @@
5152
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
5253
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
5354
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
54-
import com.google.devtools.build.lib.util.Pair;
5555
import com.google.devtools.build.lib.util.io.FileOutErr;
5656
import com.google.devtools.build.lib.vfs.FileStatus;
5757
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -146,10 +146,10 @@ public TestRunnerSpawn createTestRunnerSpawn(
146146
return new StandaloneTestRunnerSpawn(action, actionExecutionContext, spawn, tmpDir, execRoot);
147147
}
148148

149-
private static ImmutableList<Pair<String, Path>> renameOutputs(
149+
private static ImmutableMultimap<String, Path> renameOutputs(
150150
ActionExecutionContext actionExecutionContext,
151151
TestRunnerAction action,
152-
ImmutableList<Pair<String, Path>> testOutputs,
152+
ImmutableMultimap<String, Path> testOutputs,
153153
int attemptId)
154154
throws IOException {
155155
// Rename outputs
@@ -163,12 +163,12 @@ private static ImmutableList<Pair<String, Path>> renameOutputs(
163163

164164
// Get the normal test output paths, and then update them to use "attempt_N" names, and
165165
// attemptDir, before adding them to the outputs.
166-
ImmutableList.Builder<Pair<String, Path>> testOutputsBuilder = new ImmutableList.Builder<>();
167-
for (Pair<String, Path> testOutput : testOutputs) {
166+
ImmutableMultimap.Builder<String, Path> testOutputsBuilder = ImmutableMultimap.builder();
167+
for (Map.Entry<String, Path> testOutput : testOutputs.entries()) {
168168
// e.g. /testRoot/test.dir/file, an example we follow throughout this loop's comments.
169-
Path testOutputPath = testOutput.getSecond();
169+
Path testOutputPath = testOutput.getValue();
170170
Path destinationPath;
171-
if (testOutput.getFirst().equals(TestFileNameConstants.TEST_LOG)) {
171+
if (testOutput.getKey().equals(TestFileNameConstants.TEST_LOG)) {
172172
// The rename rules for the test log are different than for all the other files.
173173
destinationPath = testLog;
174174
} else {
@@ -188,7 +188,7 @@ private static ImmutableList<Pair<String, Path>> renameOutputs(
188188
// Move to the destination.
189189
testOutputPath.renameTo(destinationPath);
190190

191-
testOutputsBuilder.add(Pair.of(testOutput.getFirst(), destinationPath));
191+
testOutputsBuilder.put(testOutput.getKey(), destinationPath);
192192
}
193193
return testOutputsBuilder.build();
194194
}
@@ -240,7 +240,7 @@ private StandaloneFailedAttemptResult processTestAttempt(
240240
TestRunnerAction action,
241241
StandaloneTestResult result)
242242
throws IOException {
243-
ImmutableList<Pair<String, Path>> testOutputs =
243+
ImmutableMultimap<String, Path> testOutputs =
244244
action.getTestOutputsMapping(
245245
actionExecutionContext.getPathResolver(), actionExecutionContext.getExecRoot());
246246
if (!isLastAttempt) {
@@ -249,10 +249,10 @@ private StandaloneFailedAttemptResult processTestAttempt(
249249

250250
// Recover the test log path, which may have been renamed, and add it to the data builder.
251251
Path renamedTestLog = null;
252-
for (Pair<String, Path> pair : testOutputs) {
253-
if (TestFileNameConstants.TEST_LOG.equals(pair.getFirst())) {
252+
for (Map.Entry<String, Path> pair : testOutputs.entries()) {
253+
if (TestFileNameConstants.TEST_LOG.equals(pair.getKey())) {
254254
Preconditions.checkState(renamedTestLog == null, "multiple test_log matches");
255-
renamedTestLog = pair.getSecond();
255+
renamedTestLog = pair.getValue();
256256
}
257257
}
258258

‎src/test/java/com/google/devtools/build/lib/runtime/TestResultAggregatorTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static org.mockito.Mockito.when;
2121

2222
import com.google.common.collect.ImmutableList;
23+
import com.google.common.collect.ImmutableMultimap;
2324
import com.google.common.eventbus.EventBus;
2425
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
2526
import com.google.devtools.build.lib.actions.ArtifactRoot;
@@ -264,13 +265,13 @@ private TestResultAggregator createAggregatorWithTestRuns(int testRuns) {
264265

265266
private static TestResult testResult(TestResultData.Builder data, boolean locallyCached) {
266267
TestRunnerAction mockTestAction = mock(TestRunnerAction.class);
267-
when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableList.of());
268+
when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableMultimap.of());
268269
return new TestResult(mockTestAction, data.build(), locallyCached, /*systemFailure=*/ null);
269270
}
270271

271272
private static TestResult shardedTestResult(TestResultData.Builder data, int shardNum) {
272273
TestRunnerAction mockTestAction = mock(TestRunnerAction.class);
273-
when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableList.of());
274+
when(mockTestAction.getTestOutputsMapping(any(), any())).thenReturn(ImmutableMultimap.of());
274275
when(mockTestAction.getShardNum()).thenReturn(shardNum);
275276
return new TestResult(mockTestAction, data.build(), /*cached=*/ false, /*systemFailure=*/ null);
276277
}

‎src/test/java/com/google/devtools/build/lib/skyframe/rewinding/LostImportantOutputHandlerModule.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
import com.google.devtools.build.lib.vfs.PathFragment;
4040
import com.google.errorprone.annotations.ForOverride;
4141
import java.io.IOException;
42-
import java.util.List;
42+
import java.util.Collection;
4343
import java.util.Map;
4444
import java.util.Set;
4545
import java.util.function.Function;
@@ -115,7 +115,7 @@ public LostArtifacts processRunfilesAndGetLostArtifacts(
115115
}
116116

117117
@Override
118-
public void processTestOutputs(List<Path> testOutputs) {
118+
public void processTestOutputs(Collection<Path> testOutputs) {
119119
throw new UnsupportedOperationException();
120120
}
121121

0 commit comments

Comments
 (0)