Changed: TestCase.output -> TestCase.commandOutputs[]

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 54 matching lines @@
  * the test is completed.
  */
 class TestCase {
   /**
    * A list of commands to execute. Most test cases have a single command. Frog
    * tests have two commands, one to compilate the source and another to execute
    * it. Some isolate tests might even have three, if they require compiling
    * multiple sources that are run in isolation.
    */
   List<Command> commands;
+  Map<Command,CommandOutput> commandOutputs = new Map<Command,CommandOutput>();

[ricow1, 2012/11/14 16:59:53]

space after ,

[kustermann, 2012/11/14 17:41:22]

Done.

 
   Map configuration;
   String displayName;
-  TestOutput output;
   bool isNegative;
   Set<String> expectedOutcomes;
   TestCaseEvent completedHandler;
   TestInformation info;
 
   TestCase(this.displayName,
            this.commands,
            this.configuration,
            this.completedHandler,
            this.expectedOutcomes,
@@ skipping 48 matching lines @@
         newCommands.add(newCommand);
         // If there are extra spaces inside the prefix or suffix, this fails.
         String expected =
             '$prefix ${c.executable} $suffix ${Strings.join(c.arguments, ' ')}';
         Expect.stringEquals(expected.trim(), newCommand.commandLine);
       }
       commands = newCommands;
     }
   }
 
+  CommandOutput get lastCommandOutput {
+    // Note: If commands = [cmd1, cmd2, cmd3] and cmd2 fails then
+    //       commandOutputs contains only outputs for cmd1 and cmd2.

[ricow1, 2012/11/14 16:59:53]

you could just do
return commandOutputs[commands[commandOutputs.length - 1]];
(check that it is not empty first)

[kustermann, 2012/11/14 17:41:22]

Done.

+    var i = commands.length-1;
+    while (i>=0) {

[ricow1, 2012/11/14 16:59:53]

spaces around >=

[kustermann, 2012/11/14 17:41:22]

This code is now removed!
On 2012/11/14 16:59:53, ricow1 wrote:

+      if (commandOutputs.containsKey(commands[i])) {
+          return commandOutputs[commands[i]];
+      }
+      i--;
+    }
+    throw new Exception("CommandOutputs is empty, maybe no command was run?");
+  }
+
   int get timeout {
     if (expectedOutcomes.contains(SLOW)) {
       return configuration['timeout'] * SLOW_TIMEOUT_MULTIPLIER;
     } else {
       return configuration['timeout'];
     }
   }
 
   String get configurationString {
     final compiler = configuration['compiler'];
@@ skipping 80 matching lines @@
   }
 }
 
 
 /**
  * TestOutput records the output of a completed test: the process's exit code,
  * the standard output and standard error, whether the process timed out, and
  * the time the process took to run.  It also contains a pointer to the
  * [TestCase] this is the output of.
  */
-abstract class TestOutput {
-  factory TestOutput.fromCase(TestCase testCase,
-                              int exitCode,
-                              bool incomplete,
-                              bool timedOut,
-                              List<String> stdout,
-                              List<String> stderr,
-                              Duration time) {
-    return new TestOutputImpl.fromCase(
-        testCase, exitCode, incomplete, timedOut, stdout, stderr, time);
+abstract class CommandOutput {
+  factory CommandOutput.fromCase(TestCase testCase,
+                                 Command command,
+                                 int exitCode,
+                                 bool incomplete,
+                                 bool timedOut,
+                                 List<String> stdout,
+                                 List<String> stderr,
+                                 Duration time) {
+    return new CommandOutputImpl.fromCase(testCase, 
+        command, exitCode, incomplete, timedOut, stdout, stderr, time);

[ricow1, 2012/11/14 16:59:53]

one argument per line

[kustermann, 2012/11/14 17:41:22]

Done.

   }
 
   bool get incomplete;
 
   String get result;
 
   bool get unexpectedOutput;
 
   bool get hasCrashed;
 
   bool get hasTimedOut;
 
   bool get didFail;
 
   bool requestRetry;
 
   Duration get time;
 
   int get exitCode;
 
   List<String> get stdout;
 
   List<String> get stderr;
 
   List<String> get diagnostics;
 }
 
-class TestOutputImpl implements TestOutput {
+class CommandOutputImpl implements CommandOutput {
   TestCase testCase;
   int exitCode;
 
   /// Records if all commands were run, true if they weren't.
   final bool incomplete;
 
   bool timedOut;
   bool failed = false;
   List<String> stdout;
   List<String> stderr;
   Duration time;
   List<String> diagnostics;
 
   /**
    * A flag to indicate we have already printed a warning about ignoring the VM
    * crash, to limit the amount of output produced per test.
    */
   bool alreadyPrintedWarning = false;
 
   /**
    * Set to true if we encounter a condition in the output that indicates we
    * need to rerun this test.
    */
   bool requestRetry = false;
 
   // Don't call this constructor, call TestOutput.fromCase() to
   // get a new TestOutput instance.
-  TestOutputImpl(TestCase this.testCase,
+  CommandOutputImpl(TestCase this.testCase,
+                 Command command,

[ricow1, 2012/11/14 16:59:53]

indentation

[kustermann, 2012/11/14 17:41:22]

Done.

                  int this.exitCode,
                  bool this.incomplete,
                  bool this.timedOut,
                  List<String> this.stdout,
                  List<String> this.stderr,
                  Duration this.time) {
-    testCase.output = this;
+    testCase.commandOutputs[command] = this;
     diagnostics = [];
   }
-  factory TestOutputImpl.fromCase(TestCase testCase,
+  factory CommandOutputImpl.fromCase(TestCase testCase,
+                                  Command command,

[ricow1, 2012/11/14 16:59:53]

indentation

[kustermann, 2012/11/14 17:41:22]

Done.

                                   int exitCode,
                                   bool incomplete,
                                   bool timedOut,
                                   List<String> stdout,
                                   List<String> stderr,
                                   Duration time) {
     if (testCase is BrowserTestCase) {
-      return new BrowserTestOutputImpl(testCase, exitCode, incomplete,
+      return new BrowserCommandOutputImpl(testCase, command, exitCode, 
+          incomplete, timedOut, stdout, stderr, time);

[ricow1, 2012/11/14 16:59:53]

all or one per line

[Bill Hesse, 2012/11/14 17:07:34]

These arguments should be all on one line or one on one line.  Sorry.

[kustermann, 2012/11/14 17:41:22]

Done.

[kustermann, 2012/11/14 17:41:22]

Done.

+    } else if (testCase.configuration['compiler'] == 'dartc') {
+      return new AnalysisCommandOutputImpl(testCase, command, exitCode, 

[ricow1, 2012/11/14 16:59:53]

all or one per line

[kustermann, 2012/11/14 17:41:22]

Done.

           timedOut, stdout, stderr, time);
-    } else if (testCase.configuration['compiler'] == 'dartc') {
-      return new AnalysisTestOutputImpl(testCase, exitCode, timedOut,
-          stdout, stderr, time);
     }
-    return new TestOutputImpl(testCase, exitCode, incomplete, timedOut,
-        stdout, stderr, time);
+    return new CommandOutputImpl(testCase, command, exitCode, incomplete, 

[ricow1, 2012/11/14 16:59:53]

all or one per line

[kustermann, 2012/11/14 17:41:22]

Done.

+        timedOut, stdout, stderr, time);
   }
 
   String get result =>
       hasCrashed ? CRASH : (hasTimedOut ? TIMEOUT : (hasFailed ? FAIL : PASS));
 
   bool get unexpectedOutput => !testCase.expectedOutcomes.contains(result);
 
   bool get hasCrashed {
     // The Java dartc runner and dart2js exits with code 253 in case
     // of unhandled exceptions.
@@ skipping 19 matching lines @@
   bool get hasFailed {
     // Always fail if a runtime-error is expected and compilation failed.
     if (testCase.info != null && testCase.info.hasRuntimeError && incomplete) {
       return true;
     }
     return testCase.isNegative ? !didFail : didFail;
   }
 
 }
 
-class BrowserTestOutputImpl extends TestOutputImpl {
-  BrowserTestOutputImpl(testCase, exitCode, incomplete,
+class BrowserCommandOutputImpl extends CommandOutputImpl {
+  BrowserCommandOutputImpl(testCase, command, exitCode, incomplete,
                         timedOut, stdout, stderr, time) :

[ricow1, 2012/11/14 16:59:53]

indentation

[kustermann, 2012/11/14 17:41:22]

Done.

-    super(testCase, exitCode, incomplete, timedOut, stdout, stderr, time);
+    super(testCase, command, exitCode, incomplete, timedOut, stdout, stderr,
+          time);
 
   bool get didFail {
     // Browser case:
     // If the browser test failed, it may have been because DumpRenderTree
     // and the virtual framebuffer X server didn't hook up, or DRT crashed with
     // a core dump. Sometimes DRT crashes after it has set the stdout to PASS,
     // so we have to do this check first.
     for (String line in super.stderr) {
       if (line.contains('Gtk-WARNING **: cannot open display: :99') ||
         line.contains('Failed to run command. return code=1')) {
@@ skipping 22 matching lines @@
           break;
       }
     }
     return true;
   }
 }
 
 // The static analyzer does not actually execute code, so
 // the criteria for success now depend on the text sent
 // to stderr.
-class AnalysisTestOutputImpl extends TestOutputImpl {
+class AnalysisCommandOutputImpl extends CommandOutputImpl {
   // An error line has 8 fields that look like:
   // ERROR|COMPILER|MISSING_SOURCE|file:/tmp/t.dart|15|1|24|Missing source.
   final int ERROR_LEVEL = 0;
   final int ERROR_TYPE = 1;
   final int FORMATTED_ERROR = 7;
 
   bool alreadyComputed = false;
   bool failResult;
-  AnalysisTestOutputImpl(testCase, exitCode, timedOut, stdout, stderr, time) :
-    super(testCase, exitCode, false, timedOut, stdout, stderr, time);
+  AnalysisCommandOutputImpl(testCase, command, exitCode, timedOut, stdout, 
+                            stderr, time) :
+    super(testCase, command, exitCode, false, timedOut, stdout, stderr, time);
 
   bool get didFail {
     if (!alreadyComputed) {
       failResult = _didFail();
       alreadyComputed = true;
     }
     return failResult;
   }
 
   bool _didFail() {
@@ skipping 156 matching lines @@
   List<String> stderr;
   bool allowRetries;
 
   /** Which command of [testCase.commands] is currently being executed. */
   int currentStep;
 
   RunningProcess(TestCase this.testCase,
       [this.allowRetries = false, this.processQueue]);
 
   /**
    * Called when all commands are executed. [exitCode] is 0 if all command

[Bill Hesse, 2012/11/14 17:07:34]

Is this comment still correct?  Do we reach here after only some commands are
executed, if one fails?

[kustermann, 2012/11/14 17:41:22]

I think so. In the commandComplete function below, testComplete is called if
"exitCode != 0".
On 2012/11/14 17:07:34, Bill Hesse wrote:

    * succeded, otherwise it will have the exit code of the first failing
    * command.
    */
-  void testComplete(int exitCode, bool incomplete) {
-    new TestOutput.fromCase(testCase, exitCode, incomplete, timedOut, stdout,
-                            stderr, new Date.now().difference(startTime));
+  void testComplete(Command lastCommand, int exitCode, bool incomplete) {
+    var lastCmdOut = new CommandOutput.fromCase(testCase, lastCommand, 

[Bill Hesse, 2012/11/14 17:07:34]

lastCommandOutput, not lastCmdOut.  Never abbreviate words, except Impl.

[kustermann, 2012/11/14 17:41:22]

Done.

+        exitCode, incomplete, timedOut, stdout, stderr, 
+        new Date.now().difference(startTime));
     timeoutTimer.cancel();
-    if (testCase.output.unexpectedOutput
+    if (lastCmdOut.unexpectedOutput
         && testCase.configuration['verbose'] != null
         && testCase.configuration['verbose']) {
       print(testCase.displayName);
-      for (var line in testCase.output.stderr) print(line);
-      for (var line in testCase.output.stdout) print(line);
+      for (var line in lastCmdOut.stderr) print(line);
+      for (var line in lastCmdOut.stdout) print(line);
     }
     if (allowRetries && testCase.usesWebDriver
-        && testCase.output.unexpectedOutput
+        && lastCmdOut.unexpectedOutput
         && (testCase as BrowserTestCase).numRetries > 0) {
       // Selenium tests can be flaky. Try rerunning.
-      testCase.output.requestRetry = true;
+      lastCmdOut.requestRetry = true;
     }
-    if (testCase.output.requestRetry) {
-      testCase.output.requestRetry = false;
+    if (lastCmdOut.requestRetry) {
+      lastCmdOut.requestRetry = false;
       this.timedOut = false;
       (testCase as BrowserTestCase).numRetries--;
       print("Potential flake. Re-running ${testCase.displayName} "
           "(${(testCase as BrowserTestCase).numRetries} attempt(s) remains)");
       // When retrying we need to reset the timeout as well.
       // Otherwise there will be no timeout handling for the retry.
       timeoutTimer = null;
       this.start();
     } else {
       testCase.completed();
     }
   }
 
   /**
    * Process exit handler called at the end of every command. It internally
    * treats all but the last command as compilation steps. The last command is
    * the actual test and its output is analyzed in [testComplete].
    */
-  void stepExitHandler(int exitCode) {
+  void commandComplete(Command cmd, int exitCode) {

[Bill Hesse, 2012/11/14 17:07:34]

command

[kustermann, 2012/11/14 17:41:22]

Done.

     process = null;
     int totalSteps = testCase.commands.length;
     String suffix =' (step $currentStep of $totalSteps)';
     if (timedOut) {
       // Non-webdriver test timed out before it could complete. Webdriver tests
       // run their own timeouts by timing from the launch of the browser (which
       // could be delayed).
-      testComplete(0, true);
+      testComplete(cmd, 0, true);
     } else if (currentStep == totalSteps) {
       // Done with all test commands.
-      testComplete(exitCode, false);
+      testComplete(cmd, exitCode, false);
     } else if (exitCode != 0) {
       // One of the steps failed.
       stderr.add('test.dart: Compilation failed$suffix, exit code $exitCode\n');
-      testComplete(exitCode, true);
+      testComplete(cmd, exitCode, true);
     } else {
       // One compilation step successfully completed, move on to the
       // next step.
       stderr.add('test.dart: Compilation finished $suffix\n');
       stdout.add('test.dart: Compilation finished $suffix\n');
       if (currentStep == totalSteps - 1 && testCase.usesWebDriver &&
           !testCase.configuration['noBatch']) {
         // Note: processQueue will always be non-null for runtime == ie9, ie10,
         // ff, safari, chrome, opera. (It is only null for runtime == vm)
         // This RunningProcess object is done, and hands over control to
         // BatchRunner.startTest(), which handles reporting, etc.
         timeoutTimer.cancel();
         processQueue._getBatchRunner(testCase).startTest(testCase);
       } else {
-        runCommand(testCase.commands[currentStep++], stepExitHandler);
+        runCommand(testCase.commands[currentStep++], commandComplete);
       }
     }
   }
 
   VoidFunction makeReadHandler(StringInputStream source,
                                List<String> destination) {
     void handler () {
       if (source.closed) return;  // TODO(whesse): Remove when bug is fixed.
       var line = source.readLine();
       while (null != line) {
         destination.add(line);
         line = source.readLine();
       }
     }
     return handler;
   }
 
   void start() {
     Expect.isFalse(testCase.expectedOutcomes.contains(SKIP));
     stdout = new List<String>();
     stderr = new List<String>();
     currentStep = 0;
     startTime = new Date.now();
-    runCommand(testCase.commands[currentStep++], stepExitHandler);
+    runCommand(testCase.commands[currentStep++], commandComplete);
   }
 
-  void runCommand(Command command, void exitHandler(int exitCode)) {
+  void runCommand(Command command, void cmdCompleteHandler(Command cmd, 

[Bill Hesse, 2012/11/14 17:07:34]

Can we leave the parameter names out of the argument types on the handler?

I would rather break the line at the first comma.

[Bill Hesse, 2012/11/14 17:07:34]

commandCompleteHandler

[kustermann, 2012/11/14 17:41:22]

Done.

[kustermann, 2012/11/14 17:41:22]

Done.

+                                                           int exitCode)) {
+    void processExitHandler(int returnCode) {
+      cmdCompleteHandler(command, returnCode);
+    }
+    
     Future processFuture = Process.start(command.executable, command.arguments);
     processFuture.then((Process p) {
       process = p;
-      process.onExit = exitHandler;
+      process.onExit = processExitHandler;
       var stdoutStringStream = new StringInputStream(process.stdout);
       var stderrStringStream = new StringInputStream(process.stderr);
       stdoutStringStream.onLine =
           makeReadHandler(stdoutStringStream, stdout);
       stderrStringStream.onLine =
           makeReadHandler(stderrStringStream, stderr);
       if (timeoutTimer == null) {
         // Create one timeout timer when starting test case, remove it at end.
         timeoutTimer = new Timer(1000 * testCase.timeout, timeoutHandler);
       }
       // If the timeout fired in between two commands, kill the just
       // started process immediately.
       if (timedOut) safeKill(process);
     });
     processFuture.handleException((e) {
       print("Process error:");
       print("  Command: $command");
       print("  Error: $e");
-      testComplete(-1, false);
+      testComplete(command, -1, false);
       return true;
     });
   }
 
   void timeoutHandler(Timer unusedTimer) {
     timedOut = true;
     safeKill(process);
   }
 
   void safeKill(Process p) {
@@ skipping 10 matching lines @@
 /**
  * This class holds a value, that can be changed.  It is used when
  * closures need a shared value, that they can all change and read.
  */
 class MutableValue<T> {
   MutableValue(T this.value);
   T value;
 }
 
 class BatchRunnerProcess {
+  Command _command;
   String _executable;
   List<String> _batchArguments;
 
   Process _process;
   StringInputStream _stdoutStream;
   StringInputStream _stderrStream;
 
   TestCase _currentTest;
   List<String> _testStdout;
   List<String> _testStderr;
   String _status;
   bool _stdoutDrained = false;
   bool _stderrDrained = false;
   MutableValue<bool> _ignoreStreams;
   Date _startTime;
   Timer _timer;
 
   bool _isWebDriver;
 
   BatchRunnerProcess(TestCase testCase) {
+    _command = testCase.commands.last;
     _executable = testCase.commands.last.executable;
     _batchArguments = testCase.batchRunnerArguments;
     _isWebDriver = testCase.usesWebDriver;
   }
 
   bool get active => _currentTest != null;
 
   void startTest(TestCase testCase) {
     Expect.isNull(_currentTest);
     _currentTest = testCase;
+    _command = testCase.commands.last;
     if (_process === null) {
       // Start process if not yet started.
       _executable = testCase.commands.last.executable;
       _startProcess(() {
         doStartTest(testCase);
       });
     } else if (testCase.commands.last.executable != _executable) {
       // Restart this runner with the right executable for this test
       // if needed.
       _executable = testCase.commands.last.executable;
@@ skipping 60 matching lines @@
   }
 
   void _reportResult() {
     if (!active) return;
     // _status == '>>> TEST {PASS, FAIL, OK, CRASH, FAIL, TIMEOUT}'
 
     var outcome = _status.split(" ")[2];
     var exitCode = 0;
     if (outcome == "CRASH") exitCode = -10;
     if (outcome == "FAIL" || outcome == "TIMEOUT") exitCode = 1;
-    new TestOutput.fromCase(_currentTest, exitCode, false,
+    new CommandOutput.fromCase(_currentTest, _command, exitCode, false,
                             (outcome == "TIMEOUT"),
                             _testStdout, _testStderr,
                             new Date.now().difference(_startTime));
     var test = _currentTest;
     _currentTest = null;
     test.completed();
   }
 
   void _stderrDone() {
     _stderrDrained = true;
@@ skipping 447 matching lines @@
         // the developer doesn't waste his or her time trying to fix a bunch of
         // tests that appear to be broken but were actually just flakes that
         // didn't get retried because there had already been one failure.
         bool allowRetry = _MAX_FAILED_NO_RETRY > _progress.numFailedTests;
         new RunningProcess(test, allowRetry, this).start();
       }
       _numProcesses++;
     }
   }
 }