diff --git a/site/en/docs/user-manual.md b/site/en/docs/user-manual.md index ec2edc6b7d7a74..75dd11e99559a4 100644 --- a/site/en/docs/user-manual.md +++ b/site/en/docs/user-manual.md @@ -1521,9 +1521,14 @@ or total number of passed tests. Tests that fail all allowed attempts are considered to be failed. By default (when this option is not specified, or when it is set to -default), only a single attempt is allowed for regular tests, and +`default`), only a single attempt is allowed for regular tests, and 3 for test rules with the `flaky` attribute set. You can specify -an integer value to override the maximum limit of test attempts. Bazel allows +a single integer value to override the maximum limit of test attempts for +all tests. Alternatively, you can specify two comma-separated integers +(`,`) to set different attempt limits for tests without +the `flaky` attribute and tests with `flaky=1`, respectively. For example, +`--flaky_test_attempts=1,3` retries only flaky-marked tests. Per-target +overrides using `regex@attempts` accept a single integer only. Bazel allows a maximum of 10 test attempts in order to prevent abuse of the system. #### `--runs_per_test={{ "" }}[regex@]number{{ "" }}` {:#runs-per-test} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java index 63c27226b1c0d8..2e76320d974cff 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.exec.BinTools; import com.google.devtools.build.lib.exec.ExecutionOptions; +import com.google.devtools.build.lib.exec.FlakyTestAttempts; import com.google.devtools.build.lib.exec.StreamedTestOutput; import com.google.devtools.build.lib.exec.TestLogHelper; import com.google.devtools.build.lib.exec.TestXmlOutputParser; @@ -283,33 +284,24 @@ private static void addRunUnderArgs(TestRunnerAction testAction, List ar */ @VisibleForTesting /* protected */ public int getTestAttempts(TestRunnerAction action) { - return action.getTestProperties().isFlaky() - ? getTestAttemptsForFlakyTest(action) - : getTestAttempts(action, /* defaultTestAttempts= */ 1); - } - - private int getTestAttempts(TestRunnerAction action, int defaultTestAttempts) { Label testLabel = action.getOwner().getLabel(); - return getTestAttemptsPerLabel(executionOptions, testLabel, defaultTestAttempts); + FlakyTestAttempts attempts = getFlakyTestAttemptsForLabel(executionOptions, testLabel); + return attempts.getAttempts(action.getTestProperties().isFlaky()); } - public int getTestAttemptsForFlakyTest(TestRunnerAction action) { - return getTestAttempts(action, /* defaultTestAttempts= */ 3); - } - - private static int getTestAttemptsPerLabel( - ExecutionOptions options, Label label, int defaultTestAttempts) { + @VisibleForTesting + static FlakyTestAttempts getFlakyTestAttemptsForLabel(ExecutionOptions options, Label label) { // Check from the last provided, so that the last option provided takes precedence. for (PerLabelOptions perLabelAttempts : Lists.reverse(options.testAttempts)) { if (perLabelAttempts.isIncluded(label)) { - String attempts = Iterables.getOnlyElement(perLabelAttempts.getOptions()); - if ("default".equals(attempts)) { - return defaultTestAttempts; + try { + return FlakyTestAttempts.parse(Iterables.getOnlyElement(perLabelAttempts.getOptions())); + } catch (com.google.devtools.common.options.OptionsParsingException e) { + throw new IllegalStateException("Invalid --flaky_test_attempts value", e); } - return Integer.parseInt(attempts); } } - return defaultTestAttempts; + return FlakyTestAttempts.DEFAULT; } /** diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD index 156a5ae39d161b..f898447f1d1054 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD @@ -95,7 +95,10 @@ java_library( java_library( name = "execution_options", - srcs = ["ExecutionOptions.java"], + srcs = [ + "ExecutionOptions.java", + "FlakyTestAttempts.java", + ], deps = [ ":regex_filter_assignment_converter", "//src/main/java/com/google/devtools/build/lib/actions", diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java index f32d1c1504bacb..9923d72ea275f0 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExecutionOptions.java @@ -221,13 +221,15 @@ public boolean shouldMaterializeParamFiles() { help = "Each test will be retried up to the specified number of times in case of any test" + " failure. Tests that required more than one attempt to pass are marked as 'FLAKY'" - + " in the test summary. Normally the value specified is just an integer or the" - + " string 'default'. If an integer, then all tests will be run up to N times. If" - + " 'default', then only a single test attempt will be made for regular tests and" - + " three for tests marked explicitly as flaky by their rule (flaky=1 attribute)." - + " Alternate syntax: regex_filter@flaky_test_attempts. Where flaky_test_attempts is" - + " as above and regex_filter stands for a list of include and exclude regular" - + " expression patterns (Also see --runs_per_test). Example:" + + " in the test summary. The value may be 'default', a single positive integer, or two" + + " comma-separated integers (,). If 'default', then only a single" + + " test attempt will be made for regular tests and three for tests marked explicitly" + + " as flaky by their rule (flaky=1 attribute). If a single integer N is specified," + + " then all tests will be run up to N times. If two comma-separated integers are" + + " specified, the first applies to tests without the flaky attribute and the second" + + " applies to tests with flaky=1. Alternate syntax: regex_filter@attempts, where" + + " attempts is a single positive integer that applies to all matching tests (Also" + + " see --runs_per_test). Example:" + " --flaky_test_attempts=//foo/.*,-//foo/bar/.*@3 deflakes all tests in //foo/" + " except those under foo/bar three times. This option can be passed multiple" + " times. The most recently passed argument that matches takes precedence. If" @@ -628,55 +630,46 @@ public Converter() { /** Converter for the --flaky_test_attempts option. */ public static class TestAttemptsConverter extends PerLabelOptions.PerLabelOptionsConverter { - private static final int MIN_VALUE = 1; - private static final int MAX_VALUE = 10; - - private void validateInput(String input) throws OptionsParsingException { - if (!Objects.equals(input, "default")) { - int value = Integer.parseInt(input); - if (value < MIN_VALUE) { - throw new OptionsParsingException("'" + input + "' should be >= " + MIN_VALUE); - } else if (value > MAX_VALUE) { - throw new OptionsParsingException("'" + input + "' should be <= " + MAX_VALUE); - } - } - } + static final int MIN_VALUE = 1; + static final int MAX_VALUE = 10; @Override public PerLabelOptions convert(String input) throws OptionsParsingException { - try { - return parseAsInteger(input); - } catch (NumberFormatException ignored) { + int atIndex = input.indexOf('@'); + if (atIndex >= 0) { return parseAsRegex(input); } + return parseGlobal(input); } - private PerLabelOptions parseAsInteger(String input) - throws NumberFormatException, OptionsParsingException { - validateInput(input); + private PerLabelOptions parseGlobal(String input) throws OptionsParsingException { + FlakyTestAttempts attempts = FlakyTestAttempts.parse(input); RegexFilter catchAll = new RegexFilter(Collections.singletonList(".*"), Collections.emptyList()); - return new PerLabelOptions(catchAll, Collections.singletonList(input)); + return new PerLabelOptions( + catchAll, Collections.singletonList(attempts.toCanonicalString())); } private PerLabelOptions parseAsRegex(String input) throws OptionsParsingException { + int atIndex = input.indexOf('@'); + if (input.substring(atIndex + 1).contains(",")) { + throw new OptionsParsingException( + "'" + + input + + "' uses comma-separated attempts, which is only supported for global values;" + + " per-target overrides must use a single integer"); + } PerLabelOptions testRegexps = super.convert(input); if (testRegexps.getOptions().size() != 1) { throw new OptionsParsingException("'" + input + "' has multiple runs for a single pattern"); } - String runsPerTest = Iterables.getOnlyElement(testRegexps.getOptions()); - try { - // Run this in order to catch errors. - validateInput(runsPerTest); - } catch (NumberFormatException e) { - throw new OptionsParsingException("'" + input + "' has a non-numeric value", e); - } + FlakyTestAttempts.parse(Iterables.getOnlyElement(testRegexps.getOptions())); return testRegexps; } @Override public String getTypeDescription() { - return "a positive integer, the string \"default\", or test_regex@attempts. " + return "default, a positive integer, ,, or test_regex@attempts. " + "This flag may be passed more than once"; } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/FlakyTestAttempts.java b/src/main/java/com/google/devtools/build/lib/exec/FlakyTestAttempts.java new file mode 100644 index 00000000000000..bf4ec27e166b02 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/exec/FlakyTestAttempts.java @@ -0,0 +1,120 @@ +// Copyright 2026 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.exec; + +import com.google.common.base.Splitter; +import com.google.devtools.common.options.OptionsParsingException; +import java.util.List; +import java.util.Objects; + +/** Parsed value of {@code --flaky_test_attempts}. */ +public final class FlakyTestAttempts { + public static final FlakyTestAttempts DEFAULT = new FlakyTestAttempts(1, 3); + + private final int stableAttempts; + private final int flakyAttempts; + + public FlakyTestAttempts(int stableAttempts, int flakyAttempts) { + this.stableAttempts = stableAttempts; + this.flakyAttempts = flakyAttempts; + } + + public int getStableAttempts() { + return stableAttempts; + } + + public int getFlakyAttempts() { + return flakyAttempts; + } + + public int getAttempts(boolean isFlaky) { + return isFlaky ? flakyAttempts : stableAttempts; + } + + /** Returns the canonical string stored in {@link PerLabelOptions}. */ + public String toCanonicalString() { + if (stableAttempts == flakyAttempts) { + return Integer.toString(stableAttempts); + } + return stableAttempts + "," + flakyAttempts; + } + + public static FlakyTestAttempts parse(String input) throws OptionsParsingException { + if (Objects.equals(input, "default")) { + return DEFAULT; + } + List parts = Splitter.on(',').splitToList(input); + if (parts.size() == 1) { + int attempts = parseAttemptCount(parts.get(0), input); + return new FlakyTestAttempts(attempts, attempts); + } + if (parts.size() == 2) { + return new FlakyTestAttempts( + parseAttemptCount(parts.get(0), input), parseAttemptCount(parts.get(1), input)); + } + throw new OptionsParsingException( + "'" + + input + + "' must be 'default', a single integer, or two comma-separated integers" + + " (,)"); + } + + private static int parseAttemptCount(String token, String originalInput) + throws OptionsParsingException { + if (token.isEmpty()) { + throw new OptionsParsingException( + "'" + + originalInput + + "' must be 'default', a single integer, or two comma-separated integers" + + " (,)"); + } + try { + return validateAttemptCount(Integer.parseInt(token), originalInput); + } catch (NumberFormatException e) { + throw new OptionsParsingException("'" + originalInput + "' is not an integer", e); + } + } + + static int validateAttemptCount(int value, String originalInput) throws OptionsParsingException { + if (value < ExecutionOptions.TestAttemptsConverter.MIN_VALUE) { + throw new OptionsParsingException( + "'" + + originalInput + + "' should be >= " + + ExecutionOptions.TestAttemptsConverter.MIN_VALUE); + } + if (value > ExecutionOptions.TestAttemptsConverter.MAX_VALUE) { + throw new OptionsParsingException( + "'" + + originalInput + + "' should be <= " + + ExecutionOptions.TestAttemptsConverter.MAX_VALUE); + } + return value; + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof FlakyTestAttempts other)) { + return false; + } + return stableAttempts == other.stableAttempts && flakyAttempts == other.flakyAttempts; + } + + @Override + public int hashCode() { + return Objects.hash(stableAttempts, flakyAttempts); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/exec/BUILD b/src/test/java/com/google/devtools/build/lib/exec/BUILD index 0cb981060c519b..b896d8c9d90ed1 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/BUILD +++ b/src/test/java/com/google/devtools/build/lib/exec/BUILD @@ -45,6 +45,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception", "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", "//src/main/java/com/google/devtools/build/lib/analysis:server_directories", + "//src/main/java/com/google/devtools/build/lib/analysis:config/per_label_options", "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/bazel/rules/python", "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto", diff --git a/src/test/java/com/google/devtools/build/lib/exec/FlakyTestAttemptsTest.java b/src/test/java/com/google/devtools/build/lib/exec/FlakyTestAttemptsTest.java new file mode 100644 index 00000000000000..e4f7f6ff93648e --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/exec/FlakyTestAttemptsTest.java @@ -0,0 +1,76 @@ +// Copyright 2026 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.exec; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import com.google.devtools.common.options.OptionsParsingException; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class FlakyTestAttemptsTest { + @Test + public void parseDefault() throws Exception { + assertThat(FlakyTestAttempts.parse("default")).isEqualTo(FlakyTestAttempts.DEFAULT); + } + + @Test + public void parseSingleIntegerAppliesToBothBuckets() throws Exception { + FlakyTestAttempts attempts = FlakyTestAttempts.parse("3"); + assertThat(attempts.getStableAttempts()).isEqualTo(3); + assertThat(attempts.getFlakyAttempts()).isEqualTo(3); + assertThat(attempts.getAttempts(false)).isEqualTo(3); + assertThat(attempts.getAttempts(true)).isEqualTo(3); + } + + @Test + public void parseStableFlakyPair() throws Exception { + FlakyTestAttempts attempts = FlakyTestAttempts.parse("1,3"); + assertThat(attempts.getStableAttempts()).isEqualTo(1); + assertThat(attempts.getFlakyAttempts()).isEqualTo(3); + assertThat(attempts.getAttempts(false)).isEqualTo(1); + assertThat(attempts.getAttempts(true)).isEqualTo(3); + } + + @Test + public void toCanonicalStringUsesSingleIntegerWhenEqual() { + assertThat(new FlakyTestAttempts(3, 3).toCanonicalString()).isEqualTo("3"); + assertThat(new FlakyTestAttempts(1, 3).toCanonicalString()).isEqualTo("1,3"); + } + + @Test + public void parseRejectsTooManyValues() { + OptionsParsingException e = + assertThrows(OptionsParsingException.class, () -> FlakyTestAttempts.parse("1,2,3")); + assertThat(e).hasMessageThat().contains("two comma-separated integers"); + } + + @Test + public void parseRejectsEmptyValues() { + OptionsParsingException e = + assertThrows(OptionsParsingException.class, () -> FlakyTestAttempts.parse("1,")); + assertThat(e).hasMessageThat().contains("two comma-separated integers"); + } + + @Test + public void parseRejectsOutOfRangeValues() { + OptionsParsingException e = + assertThrows(OptionsParsingException.class, () -> FlakyTestAttempts.parse("0,3")); + assertThat(e).hasMessageThat().contains("should be >="); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/exec/TestAttemptsConverterTest.java b/src/test/java/com/google/devtools/build/lib/exec/TestAttemptsConverterTest.java new file mode 100644 index 00000000000000..4c5e1ee4b9bbf0 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/exec/TestAttemptsConverterTest.java @@ -0,0 +1,57 @@ +// Copyright 2026 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.exec; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import com.google.devtools.build.lib.analysis.config.PerLabelOptions; +import com.google.devtools.common.options.OptionsParsingException; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class TestAttemptsConverterTest { + private final ExecutionOptions.TestAttemptsConverter converter = + new ExecutionOptions.TestAttemptsConverter(); + + @Test + public void globalStableFlakyPair() throws Exception { + PerLabelOptions options = converter.convert("1,3"); + assertThat(options.getOptions()).containsExactly("1,3"); + assertThat(FlakyTestAttempts.parse(options.getOptions().get(0)).getAttempts(false)).isEqualTo(1); + assertThat(FlakyTestAttempts.parse(options.getOptions().get(0)).getAttempts(true)).isEqualTo(3); + } + + @Test + public void globalSingleInteger() throws Exception { + PerLabelOptions options = converter.convert("3"); + assertThat(options.getOptions()).containsExactly("3"); + } + + @Test + public void regexOverrideAllowsSingleInteger() throws Exception { + PerLabelOptions options = converter.convert("//foo/.*@3"); + assertThat(options.getOptions()).containsExactly("3"); + } + + @Test + public void regexOverrideRejectsStableFlakyPair() { + OptionsParsingException e = + assertThrows(OptionsParsingException.class, () -> converter.convert("//foo/.*@1,3")); + assertThat(e).hasMessageThat().contains("per-target overrides must use a single integer"); + } +} diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh index 6a3f767be13c42..5dbf2e4c5d65be 100755 --- a/src/test/shell/bazel/bazel_test_test.sh +++ b/src/test/shell/bazel/bazel_test_test.sh @@ -956,6 +956,67 @@ EOF assert_equals "fail" "$(awk "NR == $(wc -l < $TEST_log)" $TEST_log)" } +function test_flaky_test_attempts_stable_flaky_split() { + add_rules_shell "MODULE.bazel" + cat >BUILD <stable_flaky.sh < "${STABLE_FLAKE_FILE}" + echo "fail" + exit 1 +fi +echo "pass" +exit 0 +EOF + cat >marked_flaky.sh < "${MARKED_FLAKE_FILE}" + echo "fail" + exit 1 +fi +echo "pass" +exit 0 +EOF + chmod +x stable_flaky.sh marked_flaky.sh + + if is_darwin; then + export JAVA_TOOL_OPTIONS="-Djava.net.preferIPv6Addresses=true" + export STARTUP_OPTS="--host_jvm_args=-Djava.net.preferIPv6Addresses=true" + else + export STARTUP_OPTS="" + fi + + rm -f "${STABLE_FLAKE_FILE}" + bazel --ignore_all_rc_files $STARTUP_OPTS test --spawn_strategy=standalone \ + --flaky_test_attempts=1,2 //:stable_flaky &> $TEST_log \ + && fail "stable test should fail without retries" || true + expect_log_once "FAIL.*: //:stable_flaky" + [ ! -d bazel-testlogs/stable_flaky/test_attempts ] \ + || fail "stable test should not have been retried with --flaky_test_attempts=1,2" + + rm -f "${MARKED_FLAKE_FILE}" + bazel --ignore_all_rc_files $STARTUP_OPTS test --spawn_strategy=standalone \ + --flaky_test_attempts=1,2 //:marked_flaky &> $TEST_log \ + || fail "flaky-marked test should pass with one retry" + expect_log_once "FLAKY: //:marked_flaky" + assert_equals 1 $(ls bazel-testlogs/marked_flaky/test_attempts/*.log | wc -l) + + rm -f "${STABLE_FLAKE_FILE}" + bazel --ignore_all_rc_files $STARTUP_OPTS test --spawn_strategy=standalone \ + --flaky_test_attempts=2 //:stable_flaky &> $TEST_log \ + || fail "stable test should pass when single integer applies to all tests" + expect_log_once "FLAKY: //:stable_flaky" +} + function setup_undeclared_outputs_test() { mkdir -p dir