Skip to content

Commit

Permalink
Post TestAttempt/TestResult events when a test can be built but t…
Browse files Browse the repository at this point in the history
…est-exec-config-inputs fail to build.

Usually, when a test target can be built, the `TestRunnerAction` will be
buildable, but this is not always the case. If the special `exec`-config inputs
fail to build, the test action may fail to build, which will only happen in the
`test` command. The build tool (and BEP) assume that if a test-type target is
buildable, and the command is `test`, then the corresponding
`TestAttempt`/`TestResult` events will eventually be posted.

This change ensures those events are posted with minimal, correct information indicating the test `FAILED_TO_BUILD`.

RELNOTES: BEP will include correct \`TestResult\` and \`TargetSummary\` events when special test inputs like \`$test_runtime\` fail to build.
PiperOrigin-RevId: 660904505
Change-Id: Ie44d558be5393ee910e6c4171ac295af2f34b182
  • Loading branch information
michaeledgar authored and copybara-github committed Aug 8, 2024
1 parent df38c20 commit db4c550
Show file tree
Hide file tree
Showing 7 changed files with 401 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -198,20 +198,20 @@ public static SkyKey key(Artifact artifact) {
return ((DerivedArtifact) artifact).getGeneratingActionKey();
}

public static Iterable<SkyKey> keys(Iterable<Artifact> artifacts) {
return artifacts instanceof Collection<Artifact> collection
public static <T extends Artifact> Iterable<SkyKey> keys(Iterable<T> artifacts) {
return artifacts instanceof Collection<T> collection
? keys(collection)
: Iterables.transform(artifacts, Artifact::key);
}

public static Collection<SkyKey> keys(Collection<Artifact> artifacts) {
return artifacts instanceof List<Artifact> list
public static <T extends Artifact> Collection<SkyKey> keys(Collection<T> artifacts) {
return artifacts instanceof List<T> list
? keys(list)
// Use Collections2 instead of Iterables#transform to ensure O(1) size().
: Collections2.transform(artifacts, Artifact::key);
}

public static List<SkyKey> keys(List<Artifact> artifacts) {
public static <T extends Artifact> List<SkyKey> keys(List<T> artifacts) {
return Lists.transform(artifacts, Artifact::key);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@ public static TestAttempt forExecutedTestResult(
lastAttempt);
}

/**
* Creates a test attempt result from cached test data, providing a result while indicating to
* consumers that the test did not actually execute.
*/
public static TestAttempt fromCachedTestResult(
TestRunnerAction testAction,
TestResultData attemptData,
Expand All @@ -131,6 +135,29 @@ public static TestAttempt fromCachedTestResult(
lastAttempt);
}

/**
* Creates a test result for rare cases where the test itself was built, but the {@link
* TestRunnerAction} could not be started by a test strategy.
*
* <p>This overload should be very rarely used, and in particular must not be used by an
* implementation of a {@link TestStrategy}.
*/
public static TestAttempt forUnstartableTestResult(
TestRunnerAction testAction, TestResultData attemptData) {
return new TestAttempt(
false,
testAction,
/* executionInfo= */ BuildEventStreamProtos.TestResult.ExecutionInfo.getDefaultInstance(),
/* attempt= */ 1,
attemptData.getStatus(),
attemptData.getStatusDetails(),
attemptData.getStartTimeMillisEpoch(),
attemptData.getRunDurationMillis(),
/* files= */ ImmutableList.of(),
attemptData.getWarningList(),
/* lastAttempt= */ true);
}

@VisibleForTesting
public Artifact getTestStatusArtifact() {
return testAction.getCacheStatusArtifact();
Expand Down Expand Up @@ -205,7 +232,10 @@ public ImmutableList<LocalFile> referencedLocalFiles() {
// TODO(b/199940216): Can we populate metadata for these files?
localFiles.add(
new LocalFile(
file.getSecond(), localFileType, /*artifact=*/ null, /*artifactMetadata=*/ null));
file.getSecond(),
localFileType,
/* artifact= */ null,
/* artifactMetadata= */ null));
}
}
return localFiles.build();
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/util:TestType",
"//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
"//src/main/java/com/google/devtools/build/lib/util:exit_code",
"//src/main/java/com/google/devtools/build/lib/util:heap_offset_helper",
"//src/main/java/com/google/devtools/build/lib/util:resource_usage",
"//src/main/java/com/google/devtools/build/lib/util:string",
Expand All @@ -370,6 +371,7 @@ java_library(
"//src/main/java/net/starlark/java/syntax",
"//src/main/protobuf:failure_details_java_proto",
"//src/main/protobuf:memory_pressure_java_proto",
"//src/main/protobuf:test_status_java_proto",
"//third_party:auto_value",
"//third_party:caffeine",
"//third_party:flogger",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,28 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;

import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionLookupData;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.test.TestAttempt;
import com.google.devtools.build.lib.analysis.test.TestProvider;
import com.google.devtools.build.lib.analysis.test.TestResult;
import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.skyframe.GraphTraversingHelper;
import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.view.test.TestStatus.BlazeTestStatus;
import com.google.devtools.build.lib.view.test.TestStatus.TestResultData;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.util.List;
import javax.annotation.Nullable;

/**
Expand All @@ -39,7 +50,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
(TestCompletionValue.TestCompletionKey) skyKey.argument();
ConfiguredTargetKey ctKey = key.configuredTargetKey();
TopLevelArtifactContext ctx = key.topLevelArtifactContext();
if (env.getValue(TargetCompletionValue.key(ctKey, ctx, /*willTest=*/ true)) == null) {
if (env.getValue(TargetCompletionValue.key(ctKey, ctx, /* willTest= */ true)) == null) {
return null;
}

Expand All @@ -58,13 +69,27 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
}
}
} else {
if (GraphTraversingHelper.declareDependenciesAndCheckIfValuesMissingMaybeWithExceptions(
env,
Iterables.transform(
TestProvider.getTestStatusArtifacts(ct),
Artifact.DerivedArtifact::getGeneratingActionKey))) {
List<SkyKey> skyKeys = Artifact.keys(TestProvider.getTestStatusArtifacts(ct));
SkyframeLookupResult result = env.getValuesAndExceptions(skyKeys);
if (env.valuesMissing()) {
return null;
}
for (SkyKey actionKey : skyKeys) {
try {
if (result.getOrThrow(actionKey, ActionExecutionException.class) == null) {
return null;
}
} catch (ActionExecutionException e) {
DetailedExitCode detailedExitCode = e.getDetailedExitCode();
if (detailedExitCode.getExitCode().equals(ExitCode.BUILD_FAILURE)
&& ctValue instanceof ActionLookupValue actionLookupValue) {
postTestResultEventsForUnbuildableTestInputs(
env, (ActionLookupData) actionKey, actionLookupValue, detailedExitCode);
} else {
return null;
}
}
}
}
return TestCompletionValue.TEST_COMPLETION_MARKER;
}
Expand All @@ -73,4 +98,42 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
public String extractTag(SkyKey skyKey) {
return Label.print(((ConfiguredTargetKey) skyKey.argument()).getLabel());
}

/**
* Posts events for test actions that could not run because one or more exec-configuration inputs
* common to all tests failed to build.
*
* <p>When we run this SkyFunction we will have already built the test executable and its inputs,
* but we might fail to build the exec-configuration attributes providing inputs to the {@link
* TestRunnerAction} such as {@code $test_runtime}, {@code $test_wrapper}, {@code
* test_setup_script} and others (see {@link
* com.google.devtools.build.lib.analysis.BaseRuleClasses.TestBaseRule#build(com.google.devtools.build.lib.packages.RuleClass.Builder,
* com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment)} where all Test-type rules
* have additional attributes added).
*
* <p>When these exec-configuration inputs cannot be built, we do not get to use any {@code
* TestStrategy} that is responsible for posting {@link TestAttempt} and {@link TestResult}
* events. We need to handle this case here and post minimal events indicating the test {@link
* BlazeTestStatus.FAILED_TO_BUILD FAILED_TO_BUILD}.
*/
private static void postTestResultEventsForUnbuildableTestInputs(
Environment env,
ActionLookupData actionKey,
ActionLookupValue actionLookupValue,
DetailedExitCode detailedExitCode) {
BlazeTestStatus status = BlazeTestStatus.FAILED_TO_BUILD;
if (detailedExitCode
.getFailureDetail()
.getExecution()
.getCode()
.equals(Code.ACTION_NOT_UP_TO_DATE)) {
status = BlazeTestStatus.NO_STATUS;
}
TestRunnerAction testRunnerAction =
(TestRunnerAction) actionLookupValue.getAction(actionKey.getActionIndex());
TestResultData testData = TestResultData.newBuilder().setStatus(status).build();
env.getListener().post(TestAttempt.forUnstartableTestResult(testRunnerAction, testData));
env.getListener()
.post(new TestResult(testRunnerAction, testData, /* cached= */ false, detailedExitCode));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,50 @@ def _impl(ctx):
)
""");

// Create fake, minimal implementations of test-setup.sh and test-xml-generator.sh for test
// cases that actually execute tests. Does not support coverage, interruption, signals, etc.
// For proper test execution support, the actual test-setup.sh will need to be included in the
// Java test's runfiles and copied/symlinked into the MockToolsConfig's workspace.
config
.create(
"embedded_tools/tools/test/test-setup.sh",
"""
#!/bin/bash
set -e
function is_absolute {
[[ "$1" = /* ]] || [[ "$1" =~ ^[a-zA-Z]:[/\\].* ]]
}
is_absolute "$TEST_SRCDIR" || TEST_SRCDIR="$PWD/$TEST_SRCDIR"
RUNFILES_MANIFEST_FILE="${TEST_SRCDIR}/MANIFEST"
cd ${TEST_SRCDIR}
function rlocation() {
if is_absolute "$1" ; then
# If the file path is already fully specified, simply return it.
echo "$1"
elif [[ -e "$TEST_SRCDIR/$1" ]]; then
# If the file exists in the $TEST_SRCDIR then just use it.
echo "$TEST_SRCDIR/$1"
elif [[ -e "$RUNFILES_MANIFEST_FILE" ]]; then
# If a runfiles manifest file exists then use it.
echo "$(grep "^$1 " "$RUNFILES_MANIFEST_FILE" | sed 's/[^ ]* //')"
fi
}
EXE="${1#./}"
shift
if is_absolute "$EXE"; then
TEST_PATH="$EXE"
else
TEST_PATH="$(rlocation $TEST_WORKSPACE/$EXE)"
fi
exec $TEST_PATH
""")
.chmod(0755);
config
.create("embedded_tools/tools/test/test-xml-generator.sh", "#!/bin/sh", "cp \"$1\" \"$2\"")
.chmod(0755);

// Use an alias package group to allow for modification at the simpler path
config.create(
"embedded_tools/tools/allowlists/config_feature_flag/BUILD",
Expand Down
24 changes: 24 additions & 0 deletions src/test/java/com/google/devtools/build/lib/buildtool/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,30 @@ java_test(
],
)

java_test(
name = "TargetSummaryEventTest",
srcs = ["TargetSummaryEventTest.java"],
data = ["//src/test/java/com/google/devtools/build/lib:embedded_scripts"],
tags = [
"no_windows",
],
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper:credential_module",
"//src/main/java/com/google/devtools/build/lib/buildeventservice",
"//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/test/java/com/google/devtools/build/lib/analysis/util",
"//src/test/java/com/google/devtools/build/lib/buildtool/util",
"//src/test/java/com/google/devtools/build/lib/testutil",
"//third_party:guava",
"//third_party:jsr305",
"//third_party:junit4",
"//third_party:truth",
],
)

java_test(
name = "BuildResultTestCase",
srcs = ["BuildResultTestCase.java"],
Expand Down
Loading

0 comments on commit db4c550

Please sign in to comment.