test.py: Reduce the number of times we retry commands before showing them as failed.

tools/testing/dart/test_runner.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Classes and methods for executing tests.
  *
  * This module includes:
  * - Managing parallel execution of tests, including timeout checks.
  * - Evaluating the output of each test as pass/fail/crash/timeout.
@@ skipping 12 matching lines @@
 import "http_server.dart" as http_server;
 import "status_file_parser.dart";
 import "test_progress.dart";
 import "test_suite.dart";
 import "utils.dart";
 import 'record_and_replay.dart';
 
 const int CRASHING_BROWSER_EXITCODE = -10;
 const int SLOW_TIMEOUT_MULTIPLIER = 4;
 
+const MESSAGE_CANNOT_OPEN_DISPLAY = 'Gtk-WARNING **: cannot open display';
+const MESSAGE_FAILED_TO_RUN_COMMAND = 'Failed to run command. return code=1';
+
 typedef void TestCaseEvent(TestCase testCase);
 typedef void ExitCodeEvent(int exitCode);
 typedef void EnqueueMoreWork(ProcessQueue queue);
 
 // Some IO tests use these variables and get confused if the host environment
 // variables are inherited so they are excluded.
 const List<String> EXCLUDED_ENVIRONMENT_VARIABLES =
     const ['http_proxy', 'https_proxy', 'no_proxy',
            'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY'];
 
 
 /** A command executed as a step in a test case. */
 class Command {
   /** Path to the executable of this command. */
   String executable;
 
   /** The actual command line that will be executed. */
   String commandLine;
 
   /** A descriptive name for this command. */
   String displayName;
 
   /** Command line arguments to the executable. */
   List<String> arguments;
 
   /** Environment for the command */
   Map<String, String> environmentOverrides;
 
-  /** Number of times this command can be retried */
-  int get numRetries => 0;
+  /** Number of times this command *can* be retried */
+  int get maxNumRetries => 2;
 
   // We compute the Command.hashCode lazily and cache it here, since it might
   // be expensive to compute (and hashCode is called often).
   int _cachedHashCode;
 
   Command._(this.displayName, this.executable,
             this.arguments, String configurationDir,
             [this.environmentOverrides = null]) {
     if (io.Platform.operatingSystem == 'windows') {
       // Windows can't handle the first command if it is a .bat file or the like
@@ skipping 215 matching lines @@
     builder.add(expectedOutputPath.toString());
   }
 
   bool _equal(Command other) {
     return
         other is ContentShellCommand &&
         super._equal(other) &&
         expectedOutputPath.toString() == other.expectedOutputPath.toString();
   }
 
-  // FIXME(kustermann): Remove this once we're stable
-  int get numRetries => 2;
+  int get maxNumRetries => 3;
 }
 
 class BrowserTestCommand extends Command {
   final String browser;
   final String url;
 
   BrowserTestCommand._(String _browser,
                        this.url,
                        String executable,
                        List<String> arguments,
                        String configurationDir)
       : super._(_browser, executable, arguments, configurationDir),
         browser = _browser;
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     builder.add(browser);
     builder.add(url);
   }
 
   bool _equal(Command other) {
     return
         other is BrowserTestCommand &&
         super._equal(other) &&
         browser == other.browser &&
         url == other.url;
   }
-
-  // FIXME(kustermann): Remove this once we're stable
-  int get numRetries => 2;
 }
 
 class SeleniumTestCommand extends Command {
   final String browser;
   final String url;
 
   SeleniumTestCommand._(String _browser,
                         this.url,
                         String executable,
                         List<String> arguments,
                         String configurationDir)
       : super._(_browser, executable, arguments, configurationDir),
         browser = _browser;
 
   void _buildHashCode(HashCodeBuilder builder) {
     super._buildHashCode(builder);
     builder.add(browser);
     builder.add(url);
   }
 
   bool _equal(Command other) {
     return
         other is SeleniumTestCommand &&
         super._equal(other) &&
         browser == other.browser &&
         url == other.url;
   }
-
-  // FIXME(kustermann): Remove this once we're stable
-  int get numRetries => 2;
 }
 
 class AnalysisCommand extends Command {
   final String flavor;
 
   AnalysisCommand._(this.flavor,
                     String displayName,
                     String executable,
                     List<String> arguments,
                     String configurationDir)
@@ skipping 178 matching lines @@
         return false;
       }
 
       var flags = new Set.from(expectedOutcomes);
       flags..remove(TIMEOUT)
            ..remove(SLOW);
       return flags.contains(PASS) && flags.length > 1;
   }
 
   bool get isFinished {
-    return !lastCommandOutput.successfull ||
+    return !lastCommandOutput.successful ||
            commands.length == commandOutputs.length;
   }
 }
 
 
 /**
  * BrowserTestCase has an extra compilation command that is run in a separate
  * process, before the regular test is run as in the base class [TestCase].
  * If the compilation command fails, then the rest of the test is not run.
  */
@@ skipping 23 matching lines @@
   bool get hasCrashed;
 
   bool get hasTimedOut;
 
   bool didFail(testcase);
 
   bool hasFailed(TestCase testCase);
 
   bool get canRunDependendCommands;
 
-  bool get successfull; // otherwise we might to retry running
+  bool get successful; // otherwise we might to retry running
 
   Duration get time;
 
   int get exitCode;
 
   List<int> get stdout;
 
   List<int> get stderr;
 
   List<String> get diagnostics;
@@ skipping 62 matching lines @@
 
   bool didFail(TestCase testCase) {
     return (exitCode != 0 && !hasCrashed);
   }
 
   bool get canRunDependendCommands {
     // FIXME(kustermann): We may need to change this
     return !hasTimedOut && exitCode == 0;
   }
 
-  bool get successfull {
+  bool get successful {
     // FIXME(kustermann): We may need to change this
     return !hasTimedOut && exitCode == 0;
   }
 
   // Reverse result of a negative test.
   bool hasFailed(TestCase testCase) {
     // FIXME(kustermann): this is a hack, remove it
     bool isCompilationCommand = testCase.commands.first == command
         && testCase.commands.length > 1;
     if (isCompilationCommand &&
@@ skipping 23 matching lines @@
           time,
           compilationSkipped) {
     _failedBecauseOfMissingXDisplay = _didFailBecauseOfMissingXDisplay();
     if (_failedBecauseOfMissingXDisplay) {
       DebugLogger.warning("Warning: Test failure because of missing XDisplay");
       // If we get the X server error, or DRT crashes with a core dump, retry
       // the test.
     }
   }
 
+  bool get successful => canRunDependendCommands;
+
   bool get canRunDependendCommands {
     // We cannot rely on the exit code of content_shell as a method to determine
     // if we were successful or not.
     return super.canRunDependendCommands && !didFail(null);
   }
 
   bool didFail(TestCase _) {
     if (_failedBecauseOfMissingXDisplay) {
       return true;
     }
 
     if (command.expectedOutputFile != null) {
       // We are either doing a pixel test or a layout test with content shell
       return _failedBecauseOfUnexpectedDRTOutput;
     }
     return _browserTestFailure;
   }
 
   bool _didFailBecauseOfMissingXDisplay() {
     // Browser case:
     // If the browser test failed, it may have been because content shell
     // and the virtual framebuffer X server didn't hook up, or it crashed with
     // a core dump. Sometimes content shell crashes after it has set the stdout
     // to PASS, so we have to do this check first.
     var stderrLines = decodeUtf8(super.stderr).split("\n");
     for (String line in stderrLines) {
       // TODO(kustermann,ricow): Issue: 7564
       // This seems to happen quite frequently, we need to figure out why.
-      if (line.contains('Gtk-WARNING **: cannot open display') ||
-          line.contains('Failed to run command. return code=1')) {
+      if (line.contains(MESSAGE_CANNOT_OPEN_DISPLAY) ||
+          line.contains(MESSAGE_FAILED_TO_RUN_COMMAND)) {
         return true;
       }
     }
     return false;
   }
 
   bool get _failedBecauseOfUnexpectedDRTOutput {
     /*
      * The output of content shell is different for pixel tests than for
      * layout tests.
@@ skipping 996 matching lines @@
     }
 
     return Future.wait([_terminateBatchRunners(), _terminateBrowserRunners()]);
   }
 
   Future<CommandOutput> runCommand(node, Command command, int timeout) {
     assert(!_finishing);
 
     Future<CommandOutput> runCommand(int retriesLeft) {
       return _runCommand(command, timeout).then((CommandOutput output) {
-        if (!output.canRunDependendCommands && retriesLeft > 0) {
+        if (retriesLeft > 0 && shouldRetryCommand(output)) {
           DebugLogger.warning("Rerunning Command: ($retriesLeft "
                               "attempt(s) remains) [cmd: $command]");
           return runCommand(retriesLeft - 1);
         } else {
           return new Future.value(output);
         }
       });
     }
-    return runCommand(command.numRetries);
+    return runCommand(command.maxNumRetries);
   }
 
   Future<CommandOutput> _runCommand(Command command, int timeout) {
     var batchMode = !globalConfiguration['noBatch'];
 
     if (command is BrowserTestCommand) {
       return _startBrowserControllerTest(command, timeout);
     } else if (command is SeleniumTestCommand && batchMode) {
       var arguments = ['--force-refresh', '--browser=${command.browser}',
                        '--timeout=${timeout}', '--out', '${command.url}'];
@@ skipping 107 matching lines @@
   }
 
   Future cleanup() => new Future.value();
 
   Future<CommandOutput> runCommand(node, Command command, int timeout) {
     assert(node.dependencies.length == 0);
     return new Future.value(_archive.outputOf(command));
   }
 }
 
+bool shouldRetryCommand(CommandOutput output) {
+  var command = output.command;
+
+  if (!output.successful) {
+    List<String> stdout, stderr;
+
+    decodeOutput() {
+      if (stdout == null && stderr == null) {
+        stdout = decodeUtf8(output.stderr).split("\n");
+        stderr = decodeUtf8(output.stderr).split("\n");
+      }
+    }
+
+    if (io.Platform.operatingSystem == 'linux') {
+      decodeOutput();
+      // No matter which command we ran: If we get failures due to the
+      // "xvfb-run" issue 7564, try re-running the test.
+      bool containsFailureMsg(String line) {
+        return line.contains(MESSAGE_CANNOT_OPEN_DISPLAY) ||
+               line.contains(MESSAGE_FAILED_TO_RUN_COMMAND);
+      }
+      if (stdout.any(containsFailureMsg) || stderr.any(containsFailureMsg)) {
+        return true;
+      }
+    }
+
+    if (command is BrowserTestCommand) {
+      // We do not re-run tests on the new browser controller, since it should
+      // not be as flaky as selenium.
+      return false;
+    } else if (command is SeleniumTestCommand) {
+      // Selenium tests can be flaky. Try re-running.
+      return true;
+    } else if (command is ContentShellCommand) {
+      // FIXME(kustermann): Remove this condition once we figured out why
+      // content_shell is sometimes not able to fetch resources from the
+      // HttpServer on windows.
+      // TODO(kustermann): Don't blindly re-run DRT tests on windows but rather
+      // check if the stderr/stdout indicates that we actually have this issue.
+      return io.Platform.operatingSystem == 'windows';
+    }
+  }
+  return false;
+}
 
 /*
  * [TestCaseCompleter] will listen for
  * NodeState.Processing -> NodeState.{Successfull,Failed} state changes and
  * will complete a TestCase if it is finished.
  *
  * It provides a stream [finishedTestCases], which will stream all TestCases
  * once they're finished. After all TestCases are done, the stream will be
  * closed.
  */
@@ skipping 176 matching lines @@
     }
   }
 
   void eventAllTestsDone() {
     for (var listener in _eventListener) {
       listener.allDone();
     }
     _allDone();
   }
 }