Change TestTimeout's rangeMax values so that isInRangeFuzzy will flag tests that are potentially timeout flaky and getSuggestedTestTimeout is less likely to suggest timeouts that can result in timeout flakiness.

Also modernized and refactored TestTimeout to be more understandable.

RELNOTES: Adjust the thresholds for --test_verbose_timeout_warnings so that it can recommending timeout increases and won't recommend timeouts that are too close to the actual timeout.
PiperOrigin-RevId: 159222380

2 files changed, 93 insertions, 37 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/TestTimeout.java b/src/main/java/com/google/devtools/build/lib/packages/TestTimeout.java
index b2ea42218a..a14f68ef65 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/TestTimeout.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/TestTimeout.java
@@ -15,13 +15,18 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableRangeMap;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Range;
+import com.google.common.collect.RangeMap;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.OptionsParsingException;
-
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.EnumMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -32,23 +37,79 @@ import java.util.Set;
 public enum TestTimeout {
 
   // These symbolic labels are used in the build files.
-  SHORT(0, 60, 60),
-  MODERATE(30, 300, 300),
-  LONG(300, 900, 900),
-  ETERNAL(900, 365 * 24 * 60 * 60 /* One year */, 3600);
+  SHORT(60),
+  MODERATE(300),
+  LONG(900),
+  ETERNAL(3600);
 
   /**
    * Default --test_timeout flag, used when collecting code coverage.
    */
   public static final String COVERAGE_CMD_TIMEOUT = "--test_timeout=300,600,1200,3600";
 
-  private final Integer rangeMin;
-  private final Integer rangeMax;
-  private final Integer timeout;
+  /** Map from test time to suggested TestTimeout. */
+  private static final RangeMap<Integer, TestTimeout> SUGGESTED_TIMEOUT;
+
+  /**
+   * Map from TestTimeout to fuzzy range.
+   *
+   * <p>The fuzzy range is used to check whether the actual timeout is close to the upper bound of
+   * the current timeout or much smaller than the next shorter timeout. This is used to give
+   * suggestions to developers to update their timeouts.
+   */
+  private static final Map<TestTimeout, Range<Integer>> TIMEOUT_FUZZY_RANGE;
+
+  static {
+    // For the largest timeout, cap suggested and fuzzy ranges at one year.
+    final int maxTimeout = 365 * 24 * 60 * 60 /* One year */;
+
+    ImmutableRangeMap.Builder<Integer, TestTimeout> suggestedTimeoutBuilder =
+        ImmutableRangeMap.builder();
+    ImmutableMap.Builder<TestTimeout, Range<Integer>> timeoutFuzzyRangeBuilder =
+        ImmutableMap.builder();
+
+    int previousMaxSuggested = 0;
+    int previousTimeout = 0;
+
+    Iterator<TestTimeout> timeoutIterator = Arrays.asList(values()).iterator();
+    while (timeoutIterator.hasNext()) {
+      TestTimeout timeout = timeoutIterator.next();
+
+      // Set up time ranges for suggested timeouts and fuzzy timeouts. Fuzzy timeout ranges should
+      // be looser than suggested timeout ranges in order to make sure that after a test size is
+      // adjusted, it's difficult for normal time variance to push it outside the fuzzy timeout
+      // range.
+
+      // This should be exactly the previous max because there should be exactly one suggested
+      // timeout for any given time.
+      final int minSuggested = previousMaxSuggested;
+      // Only suggest timeouts that are less than 75% of the actual timeout (unless there are no
+      // higher timeouts). This should be low enough to prevent suggested times from causing test
+      // timeout flakiness.
+      final int maxSuggested =
+          timeoutIterator.hasNext() ? (int) (timeout.timeout * 0.75) : maxTimeout;
+
+      // Set fuzzy minimum timeout to half the previous timeout. If the test is that fast, it should
+      // be safe to use the shorter timeout.
+      final int minFuzzy = previousTimeout / 2;
+      // Set fuzzy maximum timeout to 90% of the timeout. A test this close to the limit can easily
+      // become timeout flaky.
+      final int maxFuzzy = timeoutIterator.hasNext() ? (int) (timeout.timeout * 0.9) : maxTimeout;
+
+      timeoutFuzzyRangeBuilder.put(timeout, Range.closedOpen(minFuzzy, maxFuzzy));
+
+      suggestedTimeoutBuilder.put(Range.closedOpen(minSuggested, maxSuggested), timeout);
+
+      previousMaxSuggested = maxSuggested;
+      previousTimeout = timeout.timeout;
+    }
+    SUGGESTED_TIMEOUT = suggestedTimeoutBuilder.build();
+    TIMEOUT_FUZZY_RANGE = timeoutFuzzyRangeBuilder.build();
+  }
+
+  private final int timeout;
 
-  private TestTimeout(Integer rangeMin, Integer rangeMax, Integer timeout) {
-    this.rangeMin = rangeMin;
-    this.rangeMax = rangeMax;
+  private TestTimeout(int timeout) {
     this.timeout = timeout;
   }
 
@@ -79,36 +140,29 @@ public enum TestTimeout {
     return super.toString().toUpperCase();
   }
 
-  public Integer getTimeout() {
+  public int getTimeout() {
     return timeout;
   }
-  /**
-   * Returns true iff the given time in seconds is exactly in the range of valid
-   * execution times for this TestSize.
-   */
-  public boolean isInRangeExact(Integer timeInSeconds) {
-    return timeInSeconds >= rangeMin && timeInSeconds < rangeMax;
-  }
 
   /**
-   * Returns true iff the given time in seconds is approximately (+/- 75%) in the range of valid
-   * execution times for this TestSize.
+   * Returns true iff the given time is not close to the upper bound timeout and is so short that it
+   * should be assigned a different timeout.
+   *
+   * <p>This is used to give suggestions to developers to update their timeouts. If this returns
+   * true, a more reasonable timeout can be selected with {@link #getSuggestedTestTimeout(int)}
    */
-  public boolean isInRangeFuzzy(Integer timeInSeconds) {
-    return timeInSeconds >= rangeMin - (rangeMin * .75)
-        && (this == ETERNAL || timeInSeconds <= rangeMax + (rangeMax * .75));
+  public boolean isInRangeFuzzy(int timeInSeconds) {
+    return TIMEOUT_FUZZY_RANGE.get(this).contains(timeInSeconds);
   }
 
   /**
    * Returns suggested test size for the given time in seconds.
+   *
+   * <p>Will suggest times that are unlikely to result in timeout flakiness even if the test has a
+   * significant amount of time variance.
    */
-  public static TestTimeout getSuggestedTestTimeout(Integer timeInSeconds) {
-    for (TestTimeout testTimeout : values()) {
-      if (testTimeout.isInRangeExact(timeInSeconds)) {
-        return testTimeout;
-      }
-    }
-    return ETERNAL;
+  public static TestTimeout getSuggestedTestTimeout(int timeInSeconds) {
+    return SUGGESTED_TIMEOUT.get(timeInSeconds);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java
index d23b7e5b5a..a147f9ac9e 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/TestResultAnalyzer.java
@@ -299,14 +299,16 @@ public class TestResultAnalyzer {
   }
 
   /**
-   * Checks whether the specified test timeout could have been smaller and adds
-   * a warning message if verbose is true.
+   * Checks whether the specified test timeout could have been smaller or is too small and adds a
+   * warning message if verbose is true.
    *
-   * <p>Returns true if there was a test with the wrong timeout, but if was not
-   * reported.
+   * <p>Returns true if there was a test with the wrong timeout, but if was not reported.
    */
-  private static boolean shouldEmitTestSizeWarningInSummary(boolean verbose,
-      List<String> warnings, List<Long> testTimes, TransitiveInfoCollection target) {
+  private static boolean shouldEmitTestSizeWarningInSummary(
+      boolean verbose,
+      List<String> warnings,
+      List<Long> testTimes,
+      TransitiveInfoCollection target) {
 
     TestTimeout specifiedTimeout =
         target.getProvider(TestProvider.class).getTestParams().getTimeout();