Update --flaky_test_attempts to also take a repeatable argument of the form
regex@attempts, similarly to --runs_per_test.
Also re-arrange some option converters to be closer to their options.
RELNOTES: flaky_test_attempts supports the regex@attempts syntax, like runs_per_test.
PiperOrigin-RevId: 186358437
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 59c5bad..3f2a463 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -384,53 +384,6 @@
}
}
- /** TODO(bazel-team): document this */
- public static class RunsPerTestConverter extends PerLabelOptions.PerLabelOptionsConverter {
- @Override
- public PerLabelOptions convert(String input) throws OptionsParsingException {
- try {
- return parseAsInteger(input);
- } catch (NumberFormatException ignored) {
- return parseAsRegex(input);
- }
- }
-
- private PerLabelOptions parseAsInteger(String input)
- throws NumberFormatException, OptionsParsingException {
- int numericValue = Integer.parseInt(input);
- if (numericValue <= 0) {
- throw new OptionsParsingException("'" + input + "' should be >= 1");
- } else {
- RegexFilter catchAll = new RegexFilter(Collections.singletonList(".*"),
- Collections.<String>emptyList());
- return new PerLabelOptions(catchAll, Collections.singletonList(input));
- }
- }
-
- private PerLabelOptions parseAsRegex(String input) throws OptionsParsingException {
- 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 {
- int numericRunsPerTest = Integer.parseInt(runsPerTest);
- if (numericRunsPerTest <= 0) {
- throw new OptionsParsingException("'" + input + "' has a value < 1");
- }
- } catch (NumberFormatException e) {
- throw new OptionsParsingException("'" + input + "' has a non-numeric value", e);
- }
- return testRegexps;
- }
-
- @Override
- public String getTypeDescription() {
- return "a positive integer or test_regex@runs. This flag may be passed more than once";
- }
- }
-
/**
* Values for the --strict_*_deps option
*/
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
index e50deed..e210a1e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java
@@ -18,7 +18,6 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
-import com.google.devtools.build.lib.analysis.config.BuildConfiguration.RunsPerTestConverter;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
@@ -30,10 +29,13 @@
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.util.RegexFilter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.TriState;
+import java.util.Collections;
import java.util.List;
/** Test-related options. */
@@ -210,4 +212,58 @@
}
return 1;
}
+
+ /**
+ * Option converter that han handle two styles of value for "--runs_per_test":
+ *
+ * <ul>
+ * <li>--runs_per_test=NUMBER: Run each test NUMBER times.
+ * <li>--runs_per_test=test_regex@NUMBER: Run each test that matches test_regex NUMBER times.
+ * This form can be repeated with multiple regexes.
+ * </ul>
+ */
+ public static class RunsPerTestConverter extends PerLabelOptions.PerLabelOptionsConverter {
+ @Override
+ public PerLabelOptions convert(String input) throws OptionsParsingException {
+ try {
+ return parseAsInteger(input);
+ } catch (NumberFormatException ignored) {
+ return parseAsRegex(input);
+ }
+ }
+
+ private PerLabelOptions parseAsInteger(String input)
+ throws NumberFormatException, OptionsParsingException {
+ int numericValue = Integer.parseInt(input);
+ if (numericValue <= 0) {
+ throw new OptionsParsingException("'" + input + "' should be >= 1");
+ } else {
+ RegexFilter catchAll =
+ new RegexFilter(Collections.singletonList(".*"), Collections.<String>emptyList());
+ return new PerLabelOptions(catchAll, Collections.singletonList(input));
+ }
+ }
+
+ private PerLabelOptions parseAsRegex(String input) throws OptionsParsingException {
+ 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 {
+ int numericRunsPerTest = Integer.parseInt(runsPerTest);
+ if (numericRunsPerTest <= 0) {
+ throw new OptionsParsingException("'" + input + "' has a value < 1");
+ }
+ } catch (NumberFormatException e) {
+ throw new OptionsParsingException("'" + input + "' has a non-numeric value", e);
+ }
+ return testRegexps;
+ }
+
+ @Override
+ public String getTypeDescription() {
+ return "a positive integer or test_regex@runs. This flag may be passed more than once";
+ }
+ }
}
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 3be1d4e..c0bae8d 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
@@ -13,18 +13,22 @@
// limitations under the License.
package com.google.devtools.build.lib.exec;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ResourceSet;
-import com.google.devtools.build.lib.exec.TestStrategy.TestOutputFormat;
-import com.google.devtools.build.lib.exec.TestStrategy.TestSummaryFormat;
+import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
import com.google.devtools.build.lib.packages.TestTimeout;
import com.google.devtools.build.lib.util.OptionsUtils;
+import com.google.devtools.build.lib.util.RegexFilter;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
import com.google.devtools.common.options.Options;
import com.google.devtools.common.options.OptionsBase;
+import com.google.devtools.common.options.OptionsParsingException;
import java.time.Duration;
+import java.util.Collections;
+import java.util.List;
import java.util.Map;
/**
@@ -132,9 +136,10 @@
@Option(
name = "flaky_test_attempts",
+ allowMultiple = true,
defaultValue = "default",
category = "testing",
- converter = TestStrategy.TestAttemptsConverter.class,
+ converter = TestAttemptsConverter.class,
documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
effectTags = {OptionEffectTag.UNKNOWN},
help =
@@ -146,7 +151,7 @@
+ "will be made for regular tests and three for tests marked explicitly as flaky by "
+ "their rule (flaky=1 attribute)."
)
- public int testAttempts;
+ public List<PerLabelOptions> testAttempts;
@Option(
name = "test_tmpdir",
@@ -173,7 +178,7 @@
+ "(this will force tests to be executed locally one at a time regardless of "
+ "--test_strategy value)."
)
- public TestOutputFormat testOutput;
+ public TestStrategy.TestOutputFormat testOutput;
@Option(
name = "test_summary",
@@ -188,7 +193,7 @@
+ "unsuccessful tests that were run, 'detailed' to print detailed information about "
+ "failed test cases, and 'none' to omit the summary."
)
- public TestSummaryFormat testSummary;
+ public TestStrategy.TestSummaryFormat testSummary;
@Option(
name = "test_timeout",
@@ -303,4 +308,62 @@
+ " aggressive RAM optimizations in some cases."
)
public boolean enableCriticalPathProfiling;
+
+ /** 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 ("default".equals(input)) {
+ return;
+ } else {
+ Integer value = Integer.parseInt(input);
+ if (value < MIN_VALUE) {
+ throw new OptionsParsingException("'" + input + "' should be >= " + MIN_VALUE);
+ } else if (value < MIN_VALUE || value > MAX_VALUE) {
+ throw new OptionsParsingException("'" + input + "' should be <= " + MAX_VALUE);
+ }
+ return;
+ }
+ }
+
+ @Override
+ public PerLabelOptions convert(String input) throws OptionsParsingException {
+ try {
+ return parseAsInteger(input);
+ } catch (NumberFormatException ignored) {
+ return parseAsRegex(input);
+ }
+ }
+
+ private PerLabelOptions parseAsInteger(String input)
+ throws NumberFormatException, OptionsParsingException {
+ validateInput(input);
+ RegexFilter catchAll =
+ new RegexFilter(Collections.singletonList(".*"), Collections.<String>emptyList());
+ return new PerLabelOptions(catchAll, Collections.singletonList(input));
+ }
+
+ private PerLabelOptions parseAsRegex(String input) throws OptionsParsingException {
+ 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);
+ }
+ return testRegexps;
+ }
+
+ @Override
+ public String getTypeDescription() {
+ return "a positive integer, the string \"default\", or test_regex@attempts. "
+ + "This flag may be passed more than once";
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
index 1114d82..b5d2350 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java
@@ -28,10 +28,12 @@
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.TestExecException;
import com.google.devtools.build.lib.actions.UserExecException;
+import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
import com.google.devtools.build.lib.analysis.test.TestActionContext;
import com.google.devtools.build.lib.analysis.test.TestResult;
import com.google.devtools.build.lib.analysis.test.TestRunnerAction;
import com.google.devtools.build.lib.analysis.test.TestTargetExecutionSettings;
+import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -44,9 +46,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.test.TestStatus.TestCase;
-import com.google.devtools.common.options.Converters.RangeConverter;
import com.google.devtools.common.options.EnumConverter;
-import com.google.devtools.common.options.OptionsParsingException;
import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
@@ -81,27 +81,6 @@
FileSystemUtils.createDirectoryAndParents(directory);
}
- /** Converter for the --flaky_test_attempts option. */
- public static class TestAttemptsConverter extends RangeConverter {
- public TestAttemptsConverter() {
- super(1, 10);
- }
-
- @Override
- public Integer convert(String input) throws OptionsParsingException {
- if ("default".equals(input)) {
- return -1;
- } else {
- return super.convert(input);
- }
- }
-
- @Override
- public String getTypeDescription() {
- return super.getTypeDescription() + " or the string \"default\"";
- }
- }
-
/** An enum for specifying different formats of test output. */
public enum TestOutputFormat {
SUMMARY, // Provide summary output only.
@@ -223,18 +202,32 @@
@VisibleForTesting /* protected */
public int getTestAttempts(TestRunnerAction action) {
return action.getTestProperties().isFlaky()
- ? getTestAttemptsForFlakyTest()
- : getTestAttempts(/*defaultTestAttempts=*/ 1);
+ ? getTestAttemptsForFlakyTest(action)
+ : getTestAttempts(action, /*defaultTestAttempts=*/ 1);
}
- public int getTestAttemptsForFlakyTest() {
- return getTestAttempts(/*defaultTestAttempts=*/ 3);
+ public int getTestAttemptsForFlakyTest(TestRunnerAction action) {
+ return getTestAttempts(action, /*defaultTestAttempts=*/ 3);
}
- private int getTestAttempts(int defaultTestAttempts) {
- return executionOptions.testAttempts == -1
- ? defaultTestAttempts
- : executionOptions.testAttempts;
+ private int getTestAttempts(TestRunnerAction action, int defaultTestAttempts) {
+ Label testLabel = action.getOwner().getLabel();
+ return getTestAttemptsPerLabel(executionOptions, testLabel, defaultTestAttempts);
+ }
+
+ private static int getTestAttemptsPerLabel(
+ ExecutionOptions options, Label label, int defaultTestAttempts) {
+ // 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;
+ }
+ return Integer.parseInt(attempts);
+ }
+ }
+ return defaultTestAttempts;
}
/**
