Added support for skipping redundant dart2js compilations.

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
diff --git a/tools/testing/dart/test_runner.dart b/tools/testing/dart/test_runner.dart
index 482b7fdd6389b6c58d0c1f3883282fc7496af6b9..a88be1a71be33afe7113e5816282ad8c8d43b561 100644
--- a/tools/testing/dart/test_runner.dart
+++ b/tools/testing/dart/test_runner.dart
@@ -13,6 +13,7 @@
 
 #import("dart:io");
 #import("dart:isolate");
+#import("dart:uri");
 #import("status_file_parser.dart");
 #import("test_progress.dart");
 #import("test_suite.dart");
@@ -46,6 +47,54 @@ class Command {
   }
 
   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;
+  }
 }
 
 /**
@@ -706,6 +755,20 @@ class RunningProcess {
   }
 
   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;
@@ -1079,6 +1142,10 @@ class ProcessQueue {
     testSuite.forEachTest(_runTest, _testCache, _testListerDone);
   }
 
+  void logSkippedCompilation() {
+    _progress.skippedCompilation();
+  }
+
   void _testListerDone() {
     _activeTestListers--;
     _checkDone();