Cache output of dart2js compilations that went wrong on disk.

tools/testing/dart/test_runner.dart

Index: tools/testing/dart/test_runner.dart
===================================================================
--- tools/testing/dart/test_runner.dart	(revision 35513)
+++ tools/testing/dart/test_runner.dart	(working copy)
@@ -92,9 +92,21 @@
 
   String toString() => reproductionCommand;
 
-  Future<bool> get outputIsUpToDate => new Future.value(false);
+  Future<CachedState> get outputIsUpToDate => 
+      new Future.value(const CachedState(false, false));

[kustermann, 2014/04/29 21:20:44]

It's not so nice that the Command class itself has a property of type
CacheState, which contains compilation logic (compilation logic should only live
in the CompilationCommand subclass).

The only reason why you need this CachedState class at the moment, is because
you only write the output files if the 'exitCode' was nonzero. You could write
stdout/stderr/extitCode/... always out to a file (even for successful
compilations) eliminating this problem. The cost would be one more file, with a
bit of content, but looking at what dart2js compilation currently outputs for
some tests (e.g. a polymer test):
-rw-r----- ... 1.1M ...  /tmp/foobar.js
-rw-r----- ... 23K   ...  /tmp/foobar.js.deps
-rw-r----- ... 367K ...  /tmp/foobar.js.map
-rw-r----- ... 1.6M ...  /tmp/foobar.precompiled.js

I don't think < 1k of stdout/stderr would matter much (just the deps file would
usually be 5-10x bigger than stdout/stderr).

Additional benefit: If a test fails at runtime and the compilation was skipped,
we could still show stdout/stderr from dart2js in the failure output (which we
currently don't have).

[ricow1, 2014/04/30 06:51:01]

This is actually no different then the fact that it already had a
outputIsUpToDate function which is only relevant for compilation commands!
There is a tradeoff here, do you want to to split using is XCommand in specific
code or do you want additional stuff in your main interface. Look below why this
is no longer a problem.
This is probably not the general case, I would expect simple tests to have a
much lower difference. That said, I am not concerned about disk space here.
I actually started out by doing this, but decided against it  since it
complicated the general logic in the RunningProcess class. It just occurred to
me now that there is no reason whatsoever that I should still be looking at the
timestamp of the dart2js generated file - I should just be looking at the cached
CommandOutput file - I don't know why I had in my mind that I needed to look at
both.

I changed now the logic to only look at the new file, much better and smaller
code - removed CachedState

+  
+  Future<String> get cachedOutput => new Future.value(null);
+  Future writeCachedOutput(CommandOutput output) => new Future.value(null);
 }
 
+class CachedState {
+  const CachedState(this.upToDateCompilation,
+                 this.upToDateCommandOutput);
+  final bool upToDateCompilation;
+  final bool upToDateCommandOutput;
+}
+
+
 class ProcessCommand extends Command {
   /** Path to the executable of this command. */
   String executable;
@@ -164,7 +176,8 @@
     return command;
   }
 
-  Future<bool> get outputIsUpToDate => new Future.value(false);
+  Future<CachedState> get outputIsUpToDate => 
+      new Future.value(const CachedState(false, false));
 }
 
 class CompilationCommand extends ProcessCommand {
@@ -172,6 +185,20 @@
   bool _neverSkipCompilation;
   List<Uri> _bootstrapDependencies;
 
+  Future writeCachedOutput(CommandOutput output) {
+    var file = new io.File(TestUtils.cachedOutputFile(_outputFile));
+    return file.writeAsString(output.json);
+  }
+
+  Future<CommandOutput> get cachedOutput {
+    var file = new io.File(TestUtils.cachedOutputFile(_outputFile));
+    return file.exists().then((exists) {
+      if (exists) return file.readAsString().then((content) {
+        return new CompilationCommandOutputImpl.fromJson(this, content);
+      });;
+    });
+  }
+
   CompilationCommand._(String displayName,
                        this._outputFile,
                        this._neverSkipCompilation,
@@ -185,9 +212,8 @@
     _bootstrapDependencies.sort();
   }
 
-  Future<bool> get outputIsUpToDate {
+  Future<CachedState> get outputIsUpToDate {
     if (_neverSkipCompilation) return new Future.value(false);

[Bill Hesse, 2014/04/29 17:51:55]

Shouldn't these return CachedState objects?

[kustermann, 2014/04/29 21:20:44]

BTW: I hope we did not disable '--checked' mode when running test.dart :)

[ricow1, 2014/04/30 06:51:01]

We did not

[ricow1, 2014/04/30 06:51:01]

Done.

-
     Future<List<Uri>> readDepsFile(String path) {
       var file = new io.File(new Path(path).toNativePath());
       if (!file.existsSync()) {
@@ -205,24 +231,39 @@
       });
     }
 
+    bool isUpToDate(lastModified, dependencies) {
+      if (lastModified == null) return false;
+      for (var dependency in dependencies) {
+        var dependencyLastModified =
+          TestUtils.lastModifiedCache.getLastModified(dependency);
+        if (dependencyLastModified == null ||
+            dependencyLastModified.isAfter(lastModified)) {
+          return false;
+        }
+      }
+      return true;
+    }
+
     return readDepsFile("$_outputFile.deps").then((dependencies) {
       if (dependencies != null) {
         dependencies.addAll(_bootstrapDependencies);
-        var jsOutputLastModified = TestUtils.lastModifiedCache.getLastModified(
+        var lastModified = TestUtils.lastModifiedCache.getLastModified(
             new Uri(scheme: 'file', path: _outputFile));
-        if (jsOutputLastModified != null) {
-          for (var dependency in dependencies) {
-            var dependencyLastModified =
-                TestUtils.lastModifiedCache.getLastModified(dependency);
-            if (dependencyLastModified == null ||
-                dependencyLastModified.isAfter(jsOutputLastModified)) {
-              return false;
-            }
+        if (isUpToDate(lastModified, dependencies)) {
+          return const CachedState(true, false);

[Bill Hesse, 2014/04/29 17:51:55]

Is CachedState(true, false) really meaning CachedState(true, ignored), since
having the right compiled output means we don't need the output?  If so, the
class should document that.  Why is true, true not a better choice here?

[ricow1, 2014/04/30 06:51:01]

Because it would not be true - if we have up to date compilation output it means
that compilation succeeded, i.e., we would not have saved the command output -
it is not up to date.
That said, I removed CachedState class again

+        } else {
+          // We did not have real output, check if we have stored the 
+          // output of running the compiler on this test for a failed run.
+          var cachedOutputFile = TestUtils.cachedOutputFile(_outputFile);
+          var cachedOutputUri = new Uri(scheme: 'file', path: cachedOutputFile);
+          var cachedOutputLastModified = 
+              TestUtils.lastModifiedCache.getLastModified(cachedOutputUri);
+          if (isUpToDate(cachedOutputLastModified, dependencies)) {
+            return const CachedState(false, true);
           }
-          return true;
         }
       }
-      return false;
+      return const CachedState(false, false);
     });
   }
 
@@ -919,6 +960,8 @@
   List<String> get diagnostics;
 
   bool get compilationSkipped;
+
+  String get json;
 }
 
 class CommandOutputImpl extends UniqueObject implements CommandOutput {
@@ -1008,6 +1051,9 @@
     }
     return Expectation.FAIL;
   }
+
+  String get json => null;
+
 }
 
 class BrowserCommandOutputImpl extends CommandOutputImpl {
@@ -1605,6 +1651,32 @@
         exitCode == 0 ? Expectation.PASS : Expectation.COMPILETIME_ERROR;
     return _negateOutcomeIfNegativeTest(outcome, testCase.isNegative);
   }
+
+  String get json {
+    var map = {
+    'stdout': stdout,
+    'stderr': stderr,
+    'exitCode': exitCode,
+    'timedOut': timedOut,

[kustermann, 2014/04/29 21:20:44]

It may make sense to also add 'time' here, not sure though.

[ricow1, 2014/04/30 06:51:01]

I did so at first, but removed it, since it would give counter intuitive numbers
in the reporting (with the --time flag). I would be surprised to see compilation
taking the top 20 spots when I did not do any compilations :-)

+    };
+    return JSON.encode(map);
+  }
+
+  factory CompilationCommandOutputImpl.fromJson(Command command,
+                                                String json) {
+    var obj = JSON.decode(json);
+    for (var v in ['stdout', 'stderr', 'exitCode', 'timedOut']) {
+      assert(obj.containsKey(v));
+    }
+    return new CompilationCommandOutputImpl(command,
+                                            obj['exitCode'],
+                                            obj['timedOut'],
+                                            obj['stdout'],
+                                            obj['stderr'],
+                                            const Duration(seconds: 0),
+                                            true);
+                                            
+  }
 }
 
 class JsCommandlineOutputImpl extends CommandOutputImpl
@@ -1802,10 +1874,15 @@
   }
 
   void _runCommand() {
-    command.outputIsUpToDate.then((bool isUpToDate) {
-      if (isUpToDate) {
+    command.outputIsUpToDate.then((CachedState cachedState) {
+      if (cachedState.upToDateCompilation) {
         compilationSkipped = true;
         _commandComplete(0);
+      } else if (cachedState.upToDateCommandOutput) {
+        compilationSkipped = true;
+        command.cachedOutput.then((cached) {
+          _commandComplete(cached.exitCode, cachedOutput: cached);
+        });
       } else {
         var processEnvironment = _createProcessEnvironment();
         Future processFuture =
@@ -1898,12 +1975,19 @@
     });
   }
 
-  void _commandComplete(int exitCode) {
+  void _commandComplete(int exitCode, {cachedOutput: null}) {
     if (timeoutTimer != null) {
       timeoutTimer.cancel();
     }
-    var commandOutput = _createCommandOutput(command, exitCode);
-    completer.complete(commandOutput);
+    if (cachedOutput != null) {
+      completer.complete(cachedOutput);
+    } else {
+      var commandOutput = _createCommandOutput(command, exitCode);
+      if (exitCode != 0) {
+        command.writeCachedOutput(commandOutput);
+      }

[Bill Hesse, 2014/04/29 17:51:55]

Should we put a try-catch around this?  Or make writeCachedOutput never throw? 
Or do we want to crash in this case?

[ricow1, 2014/04/30 06:51:01]

Changed writeCachedOutput to not ever throw

+      completer.complete(commandOutput);
+    }
   }
 
   CommandOutput _createCommandOutput(ProcessCommand command, int exitCode) {