Added json result of test output to output debug directory.

tools/testing/dart/test_progress.dart

Index: tools/testing/dart/test_progress.dart
diff --git a/tools/testing/dart/test_progress.dart b/tools/testing/dart/test_progress.dart
index ee5deaf86d32293e693440ae4033cb5b966330cc..a5e3a4ab842bfc16d3404287d8690880e38b499b 100644
--- a/tools/testing/dart/test_progress.dart
+++ b/tools/testing/dart/test_progress.dart
@@ -2,7 +2,7 @@
 // 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.
 
-import 'dart:convert' show JSON;
+import 'dart:convert';
 import 'dart:io';
 
 import "package:status_file/expectation.dart";
@@ -51,7 +51,7 @@ class EventListener {
   void testAdded() {}
   void done(TestCase test) {}
   void allTestsKnown() {}
-  void allDone() {}
+  void allDone(Configuration configuration) {}
 }
 
 class ExitCodeSetter extends EventListener {
@@ -80,7 +80,7 @@ class IgnoredTestMonitor extends EventListener {
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (countIgnored > 0) {
       print("Ignored $countIgnored tests due to flaky infrastructure");
     }
@@ -140,22 +140,6 @@ class TestOutcomeLogWriter extends EventListener {
 
   void done(TestCase test) {
     var name = test.displayName;
-    var configuration = {
-      'mode': test.configuration.mode.name,
-      'arch': test.configuration.architecture.name,
-      'compiler': test.configuration.compiler.name,
-      'runtime': test.configuration.runtime.name,
-      'checked': test.configuration.isChecked,
-      'strong': test.configuration.isStrong,
-      'host_checked': test.configuration.isHostChecked,
-      'minified': test.configuration.isMinified,
-      'csp': test.configuration.isCsp,
-      'system': test.configuration.system.name,
-      'vm_options': test.configuration.vmOptions,
-      'use_sdk': test.configuration.useSdk,
-      'builder_tag': test.configuration.builderTag
-    };
-
     var outcome = '${test.lastCommandOutput.result(test)}';
     var expectations =
         test.expectedOutcomes.map((expectation) => "$expectation").toList();
@@ -175,7 +159,7 @@ class TestOutcomeLogWriter extends EventListener {
     }
     _writeTestOutcomeRecord({
       'name': name,
-      'configuration': configuration,
+      'configuration': test.configuration.toSummaryMap(),
       'test_result': {
         'outcome': outcome,
         'expected_outcomes': expectations,
@@ -185,11 +169,14 @@ class TestOutcomeLogWriter extends EventListener {
     });
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (_sink != null) _sink.close();
   }
 
   void _writeTestOutcomeRecord(Map record) {
+    // TODO(mkroghj) change the location of this file
+    // to be in the debug_output_directory
+    // if the current location is not used.
     if (_sink == null) {
       _sink = new File(TestUtils.testOutcomeFileName)
           .openWrite(mode: FileMode.APPEND);
@@ -283,7 +270,7 @@ class TimingPrinter extends EventListener {
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     Duration d = (new DateTime.now()).difference(_startTime);
     print('\n--- Total time: ${_timeString(d)} ---');
     var outputs = _commandOutputs.toList();
@@ -316,7 +303,7 @@ class StatusFileUpdatePrinter extends EventListener {
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     _printFailureSummary();
   }
 
@@ -379,7 +366,7 @@ class SkippedCompilationsPrinter extends EventListener {
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (_skippedCompilations > 0) {
       print('\n$_skippedCompilations compilations were skipped because '
           'the previous output was already up to date.\n');
@@ -420,7 +407,7 @@ class TestFailurePrinter extends EventListener {
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (_printSummary) {
       if (!_failureSummary.isEmpty) {
         print('\n=== Failure summary:\n');
@@ -487,7 +474,7 @@ class ProgressIndicator extends EventListener {
 abstract class CompactIndicator extends ProgressIndicator {
   CompactIndicator(DateTime startTime) : super(startTime);
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (_failedTests > 0) {
       // We may have printed many failure logs, so reprint the summary data.
       _printProgress();
@@ -555,7 +542,7 @@ class BuildbotProgressIndicator extends ProgressIndicator {
     print('@@@STEP_TEXT@ $percent% +$_passedTests -$_failedTests @@@');
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (!_failureSummary.isEmpty) {
       print('@@@STEP_FAILURE@@@');
       if (stepName != null) {
@@ -678,3 +665,67 @@ String _buildSummaryEnd(int failedTests) {
     return '\n===\n=== ${failedTests} test$pluralSuffix failed\n===\n';
   }
 }
+
+class TestResultLogWriter extends EventListener {
+  Map<String, Map> _configurations = {};
+  List<Map> _results = [];
+
+  void done(TestCase test) {
+    // We try to find an existing configuration, so as to not duplicate this
+    // for each test.
+    var thisConf = test.configuration.toSummaryMap();

[Bill Hesse, 2017/09/01 11:56:13]

Make toSummaryMap return a cached object, that only is created once.
You can even make _summaryMap in the configuration constructor, so it is final.
Then you can compare with isIdentical.

You could even assign the key in the _summaryMap, the first time you see it
here, and just store the maps in a list.  You don't ever actually need to look
up a summaryMap by key for anything, you just need to get the key from a map.

So, here you would write
(Can't use .putIfAbsent easily because we also want to add it to a list)
if !thisConf.contains('key') {
  thisConf['key'] = _getNewKey();
  _configurations.add(thisConf);
}
var key = thisConf['key'];

  

[mkroghj, 2017/09/02 09:40:22]

The reason why I chose it was because that is the way I generally would like the
Json to look like.. As:
{
  ...
  configurations : { "key1": ..., "key2": ... } 

I rather not have it as a list, with a property on each object for the key. This
would make it more difficult for consumers to use it.

But I think generating the summaryMap once for each configuration if it re-uses
the object (which I was unsure about) is a good idea, I can still test for
isIdentical.

Would this be satisfactory?

[Bill Hesse, 2017/09/04 07:49:59]

Yes.  It is tricky to make each configuration return the same summaryMap each
time without explicit caching, and testing if the cache is there yet.  Static
members do caching and initialization on first use automatically, but instance
members should be set in the constructor unless you want to check each time if
they are there yet.

+    String key = _configurations.keys.firstWhere(
+        (key) => areSummaryMapsEqual(_configurations[key], thisConf),
+        orElse: () {
+      var newKey = "conf${_configurations.length + 1}";
+      _configurations[newKey] = thisConf;
+      return newKey;
+    });
+    _results.add({
+      'configuration': key,
+      'name': test.displayName,
+      'commands': test.commands.map((command) {

[Bill Hesse, 2017/09/01 11:56:13]

You could move the computation of this list outside and before the _results.add
call.

[mkroghj, 2017/09/04 10:43:34]

Done.

+        var output = test.commandOutputs[command];
+        if (output != null) {
+          var outputMap = {
+            'name': command.displayName,
+            'exitCode': output.exitCode,
+            'compilationSkipped': output.compilationSkipped,
+            'timeout': output.hasTimedOut,
+            'duration': output.time.inMilliseconds
+          };
+          if (test.unexpectedOutput) {

[Bill Hesse, 2017/09/01 11:56:13]

I thought the whole point here was that we wouldn't depend on test expectations.
 But I think unexpectedOutput depends on the status files.

I think maybe we should start by including no stdout or stderr data. Those
should be there in the regular logs anyway. Later we could include a stack trace
if one is in stdout.  Having BASE64 in the Json would also be really ugly.

These outputs are already going to stdout, we don't need them in the results
log.

[mkroghj, 2017/09/02 09:40:22]

I think unexpectedOutput checks for isCrash or isFail, which I think implicitly
check the status file. But it is the only time when a stack-trace is attempted
to be written out in the other logs.

But if we are to not keep any logs, I will just remove those lines.
That is how it was written for the

+            if (!output.stdout.isEmpty) {
+              outputMap["stdout"] = encodeStringForJson(
+                  _linesWithoutCarriageReturn(output.stdout).join('\n'));
+            }
+            if (!output.stderr.isEmpty) {
+              outputMap["stderr"] = encodeStringForJson(
+                  _linesWithoutCarriageReturn(output.stderr).join('\n'));
+            }
+          }
+          return outputMap;
+        } else {
+          return {'name': command.displayName};

[Bill Hesse, 2017/09/01 11:56:13]

Put this up after "if (output == null)", then you don't need an else clause.

[mkroghj, 2017/09/02 09:40:22]

I don't think I have an output == null test somewhere.. :)

[Bill Hesse, 2017/09/04 07:49:59]

I meant that you should reverse the test above, put this short return there, and
then current "then" block is not nested.

+        }
+      }).toList()
+    });
+  }
+
+  String encodeStringForJson(String str) => BASE64.encode(UTF8.encode(str));
+
+  void allDone(Configuration configuration) {
+    var path = new Path(configuration.debugOutputDirectory);

[Bill Hesse, 2017/09/01 11:56:13]

Can you use file URLs here?  We don't want new code to use the Path class.

We can set the outputDirectory from any configuration we receive during the test
runs.  Or set it in the constructor of this handler, when the options say to add
it.  Then we don't need to add it to the interface of all "allDone" handlers.

[mkroghj, 2017/09/02 09:40:22]

The test-runner owns a Configuration _globalConfiguration object, which is never
passed explicitly to any handler. I just found it as a prettier design than
keeping state around if it is not needed. But I will change it.

+    path = path.append(TestUtils.testResultFileName);
+    String fullPath = path.toNativePath();
+    var file = new File(fullPath);
+    file.createSync(recursive: true);
+    file.writeAsStringSync(
+        JSON.encode({'configurations': _configurations, 'results': _results}));
+  }
+
+  bool areSummaryMapsEqual(Map map1, Map map2) {

[Bill Hesse, 2017/09/01 11:56:13]

Shouldn't be needed.

+    return map1.keys
+        .every((key) => map2.containsKey(key) && map1[key] == map2[key]);
+  }
+}