diff options
4 files changed, 150 insertions, 85 deletions
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 59c5badf37..3f2a463c43 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 @@ public class BuildConfiguration implements BuildConfigurationInterface { } } - /** 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 e50deeded7..e210a1effd 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.ImmutableSet; 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.Event; 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 @@ public class TestConfiguration extends Fragment { } 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 3be1d4eac6..c0bae8dca9 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 @@ public class ExecutionOptions extends OptionsBase { @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 @@ public class ExecutionOptions extends OptionsBase { + "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 @@ public class ExecutionOptions extends OptionsBase { + "(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 @@ public class ExecutionOptions extends OptionsBase { + "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 @@ public class ExecutionOptions extends OptionsBase { + " 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 1114d821ad..b5d2350208 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.ExecException; 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.FileSystemUtils; 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 @@ public abstract class TestStrategy implements TestActionContext { 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 @@ public abstract class TestStrategy implements TestActionContext { @VisibleForTesting /* protected */ public int getTestAttempts(TestRunnerAction action) { return action.getTestProperties().isFlaky() - ? getTestAttemptsForFlakyTest() - : getTestAttempts(/*defaultTestAttempts=*/ 1); + ? getTestAttemptsForFlakyTest(action) + : getTestAttempts(action, /*defaultTestAttempts=*/ 1); + } + + public int getTestAttemptsForFlakyTest(TestRunnerAction action) { + return getTestAttempts(action, /*defaultTestAttempts=*/ 3); } - public int getTestAttemptsForFlakyTest() { - return getTestAttempts(/*defaultTestAttempts=*/ 3); + private int getTestAttempts(TestRunnerAction action, int defaultTestAttempts) { + Label testLabel = action.getOwner().getLabel(); + return getTestAttemptsPerLabel(executionOptions, testLabel, defaultTestAttempts); } - private int getTestAttempts(int defaultTestAttempts) { - return executionOptions.testAttempts == -1 - ? defaultTestAttempts - : executionOptions.testAttempts; + 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; } /** |