Added support for skipping redundant dart2js compilations.

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.
  */
 #library("test_runner");
 
 #import("dart:io");
 #import("dart:isolate");
+#import("dart:uri");
 #import("status_file_parser.dart");
 #import("test_progress.dart");
 #import("test_suite.dart");
 
 const int NO_TIMEOUT = 0;
 const int SLOW_TIMEOUT_MULTIPLIER = 4;
 
 typedef void TestCaseEvent(TestCase testCase);
 typedef void ExitCodeEvent(int exitCode);
 typedef void EnqueueMoreWork(ProcessQueue queue);
@@ skipping 13 matching lines @@
     if (Platform.operatingSystem == 'windows') {
       // Windows can't handle the first command if it is a .bat file or the like
       // with the slashes going the other direction.
       // TODO(efortuna): Remove this when fixed (Issue 1306).
       executable = executable.replaceAll('/', '\\');
     }
     commandLine = "$executable ${Strings.join(arguments, ' ')}";
   }
 
   String toString() => commandLine;
+
+  bool get outputIsUpToDate => false;
+}
+
+class Dart2JsCommand extends Command {
+  String _jsOutFile;

[ahe, 2012/11/14 18:46:16]

We try to avoid abbreviating.  So this should be _jsOutputFile.

[kustermann, 2012/11/16 14:58:42]

Done.

+  bool _neverSkipCompilation;
+  List<Uri> _bootstrapDeps;

[ahe, 2012/11/14 18:46:16]

_bootstrapDependencies

[kustermann, 2012/11/16 14:58:42]

Done.

+
+  Dart2JsCommand(String this._jsOutFile, bool this._neverSkipCompilation, 

[ricow1, 2012/11/14 08:53:28]

I know that not all of our code is actually strictly adhering to this - but I
really like to either have all parameters on one line, or if that don't fit, one
line per parameter (indented under the parameter from the main line)

[ahe, 2012/11/14 18:46:16]

Remove types from this.field parameters.  They are spec'ed to be inferred from
the corresponding fields.

[kustermann, 2012/11/16 14:58:42]

Done.

[kustermann, 2012/11/16 14:58:42]

Done.

+      List<Uri> this._bootstrapDeps, String executable, List<String> arguments)
+      : super(executable, arguments);
+
+  bool get outputIsUpToDate {
+    if (_neverSkipCompilation) return false;
+
+    List<Uri> readDepsFile(String path) {

[ricow1, 2012/11/14 08:53:28]

I think we should make this asynchronious

[ahe, 2012/11/14 18:46:16]

Using the word "deps" file is fine in this case, because it actually reads a
.deps file.

[kustermann, 2012/11/16 14:58:42]

Done.

+      var file = new File(path);
+      if (!file.existsSync()) {
+        return null;
+      }
+      var deps = new List<Uri>();

[ahe, 2012/11/14 18:46:16]

But these are dependencies :-)

[kustermann, 2012/11/16 14:58:42]

Done.

+      for (var line in file.readAsLinesSync()) {
+        line = line.trim();
+        if (line.length > 0) {
+          deps.add(new Uri(line));
+        }
+      }
+      return deps;
+    }
+
+    var deps = readDepsFile("$_jsOutFile.deps");

[ahe, 2012/11/14 18:46:16]

dependencies

[kustermann, 2012/11/16 14:58:42]

Done.

+    if (deps != null) {
+      deps.addAll(_bootstrapDeps);
+      var jsOutTimestamp = TestUtils.timestampCache.getTimeStamp(

[ahe, 2012/11/14 18:46:16]

jsOutput...

[kustermann, 2012/11/16 14:58:42]

Done.

+          new Uri("file://$_jsOutFile"));

[ahe, 2012/11/14 18:46:16]

How do you ensure this is a valid file URI?

See: http://blogs.msdn.com/b/ie/archive/2006/12/06/file-uris-in-windows.aspx

[kustermann, 2012/11/16 14:58:42]

"StandardTestSuite.{makeCommands,_compileCommand}" call the Dart2JsCommand
constructor. These two methods build the  jsOutputFilename based on the output
of "createOutputDirectory". "createOutputDirectory" in return,  replaces '\\' by
'/'.
This is obviously bad code and should be fixed. But I don't think this CL should
include these cleanups.

On 2012/11/14 18:46:16, ahe wrote:

+      if (jsOutTimestamp != null) {
+        for (var dep in deps) {
+          var depTs = TestUtils.timestampCache.getTimeStamp(dep);

[ahe, 2012/11/14 18:46:16]

In this case, abbreviation makes it really hard to read this code.  So I suggest
that you use dependencyTimestamp or lastModified.

[kustermann, 2012/11/16 14:58:42]

Done. But:
Very often, longer names result in lines with more than 80 characters. Meaning
we need to break the line into multiple lines. This in return often makes the
code less readable than the original one.
On 2012/11/14 18:46:16, ahe wrote:

+          if (depTs == null || depTs > jsOutTimestamp) {
+            return false;
+          }
+        }
+        return true;
+      }
+    }
+    return false;
+  }
 }
 
 /**
  * TestCase contains all the information needed to run a test and evaluate
  * its output.  Running a test involves starting a separate process, with
  * the executable and arguments given by the TestCase, and recording its
  * stdout and stderr output streams, and its exit code.  TestCase only
  * contains static information about the test; actually running the test is
  * performed by [ProcessQueue] using a [RunningProcess] object.
  *
@@ skipping 640 matching lines @@
   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);
   }
 
   void runCommand(Command command, void exitHandler(int exitCode)) {
+    if (command.outputIsUpToDate) {
+      // NOTE: we need to have the same async + timeout handler behaviour as below

[ricow1, 2012/11/14 08:53:28]

long line

[kustermann, 2012/11/16 14:58:42]

Done.

+      //       otherwise we risk breaking code.

[ahe, 2012/11/14 18:46:16]

Only one space after //.

[kustermann, 2012/11/16 14:58:42]

Done.

+      timeoutTimer = new Timer(1000 * testCase.timeout, timeoutHandler);
+      new Timer(0, (ignored) {

[ahe, 2012/11/14 18:46:16]

You could avoid this if command.outputIsUpToDate returned a future.

[kustermann, 2012/11/16 14:58:42]

Done.

+        stdout.add("Skipped dart2js compilation because the old output is still up to date!");

[ricow1, 2012/11/14 08:53:28]

long line

[kustermann, 2012/11/16 14:58:42]

Done.

+        if (processQueue != null) {

[Bill Hesse, 2012/11/14 09:37:34]

We don't ever get to this point without a processQueue, do we?  What runs the
tests, if not a processQueue?

[kustermann, 2012/11/16 14:58:42]

Actually the test
 tests/standalone/io/test_runner_test.dart
as well as the one included in this CL do run RunningProcess without a
ProcessQueue.
On 2012/11/14 09:37:34, Bill Hesse wrote:

+          processQueue.logSkippedCompilation();

[Bill Hesse, 2012/11/14 09:37:34]

Other information gets to the progress indicator by being put in the testOutput
class.  Could we put this as a flag into testOutput, rather than tunneling
through processQueue to the progress indicator?

[kustermann, 2012/11/16 14:58:42]

Done.

+        }
+        exitHandler(0);
+      });
+      return;
+    }
+
     Future processFuture = Process.start(command.executable, command.arguments);
     processFuture.then((Process p) {
       process = p;
       process.onExit = exitHandler;
       var stdoutStringStream = new StringInputStream(process.stdout);
       var stderrStringStream = new StringInputStream(process.stderr);
       stdoutStringStream.onLine =
           makeReadHandler(stdoutStringStream, stdout);
       stderrStringStream.onLine =
           makeReadHandler(stderrStringStream, stderr);
@@ skipping 353 matching lines @@
   }
 
   /**
    * Registers a TestSuite so that all of its tests will be run.
    */
   void addTestSuite(TestSuite testSuite) {
     _activeTestListers++;
     testSuite.forEachTest(_runTest, _testCache, _testListerDone);
   }
 
+  void logSkippedCompilation() {
+    _progress.skippedCompilation();
+  }
+
   void _testListerDone() {
     _activeTestListers--;
     _checkDone();
   }
 
   /**
    * Perform any cleanup needed once all tests in a TestSuite have completed
    * and notify our progress indicator that we are done.
    */
   void _cleanupAndMarkDone() {
@@ skipping 238 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++;
     }
   }
 }