Added json result of test output to output debug directory.

tools/testing/dart/test_progress.dart

 // Copyright (c) 2013, 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.
 
-import 'dart:convert' show JSON;
+import 'dart:convert';
 import 'dart:io';
 
 import "package:status_file/expectation.dart";
 
 import 'command.dart';
 import 'command_output.dart';
 import 'configuration.dart';
 import 'path.dart';
 import 'summary_report.dart';
 import 'test_runner.dart';
@@ skipping 28 matching lines @@
   String failed(String message) => _color(message, _red);
 
   static String _color(String message, int color) =>
       "$_escape[${color}m$message$_escape[0m";
 }
 
 class EventListener {
   void testAdded() {}
   void done(TestCase test) {}
   void allTestsKnown() {}
-  void allDone() {}
+  void allDone(Configuration configuration) {}
 }
 
 class ExitCodeSetter extends EventListener {
   void done(TestCase test) {
     if (test.unexpectedOutput) {
       exitCode = 1;
     }
   }
 }
 
 class IgnoredTestMonitor extends EventListener {
   static final int maxIgnored = 10;
 
   int countIgnored = 0;
 
   void done(TestCase test) {
     if (test.lastCommandOutput.result(test) == Expectation.ignore) {
       countIgnored++;
       if (countIgnored > maxIgnored) {
         print("/nMore than $maxIgnored tests were ignored due to flakes in");
         print("the test infrastructure. Notify whesse@google.com.");
         print("Output of the last ignored test was:");
         print(_buildFailureOutput(test));
         exit(1);
       }
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (countIgnored > 0) {
       print("Ignored $countIgnored tests due to flaky infrastructure");
     }
   }
 }
 
 class FlakyLogWriter extends EventListener {
   void done(TestCase test) {
     if (test.isFlaky && test.result != Expectation.pass) {
       var buf = new StringBuffer();
@@ skipping 39 matching lines @@
    *           duration: 200.2,
    *         },
    *       ],
    *     }
    *  },
    */
   IOSink _sink;
 
   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();
 
     var commandResults = [];
     double totalDuration = 0.0;
     for (var command in test.commands) {
       var output = test.commandOutputs[command];
       if (output != null) {
         double duration = output.time.inMicroseconds / 1000.0;
         totalDuration += duration;
         commandResults.add({
           'name': command.displayName,
           'duration': duration,
         });
       }
     }
     _writeTestOutcomeRecord({
       'name': name,
-      'configuration': configuration,
+      'configuration': test.configuration.toSummaryMap(),
       'test_result': {
         'outcome': outcome,
         'expected_outcomes': expectations,
         'duration': totalDuration,
         'command_results': commandResults,
       },
     });
   }
 
-  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);
     }
     _sink.write("${JSON.encode(record)}\n");
   }
 }
 
 class UnexpectedCrashLogger extends EventListener {
   final archivedBinaries = <String, String>{};
@@ skipping 73 matching lines @@
 
   void done(TestCase testCase) {
     for (var commandOutput in testCase.commandOutputs.values) {
       var command = commandOutput.command;
       _commandOutputs.add(commandOutput);
       _command2testCases.putIfAbsent(command, () => <TestCase>[]);
       _command2testCases[command].add(testCase);
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     Duration d = (new DateTime.now()).difference(_startTime);
     print('\n--- Total time: ${_timeString(d)} ---');
     var outputs = _commandOutputs.toList();
     outputs.sort((a, b) {
       return b.time.inMilliseconds - a.time.inMilliseconds;
     });
     for (int i = 0; i < 20 && i < outputs.length; i++) {
       var commandOutput = outputs[i];
       var command = commandOutput.command;
       var testCases = _command2testCases[command];
@@ skipping 12 matching lines @@
 class StatusFileUpdatePrinter extends EventListener {
   var statusToConfigs = new Map<String, List<String>>();
   var _failureSummary = <String>[];
 
   void done(TestCase test) {
     if (test.unexpectedOutput) {
       _printFailureOutput(test);
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     _printFailureSummary();
   }
 
   void _printFailureOutput(TestCase test) {
     String status = '${test.displayName}: ${test.result}';
     List<String> configs =
         statusToConfigs.putIfAbsent(status, () => <String>[]);
     configs.add(test.configurationString);
     if (test.lastCommandOutput.hasTimedOut) {
       print('\n${test.displayName} timed out on ${test.configurationString}');
@@ skipping 42 matching lines @@
 
 class SkippedCompilationsPrinter extends EventListener {
   int _skippedCompilations = 0;
 
   void done(TestCase test) {
     for (var commandOutput in test.commandOutputs.values) {
       if (commandOutput.compilationSkipped) _skippedCompilations++;
     }
   }
 
-  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');
     }
   }
 }
 
 class LineProgressIndicator extends EventListener {
   void done(TestCase test) {
     var status = 'pass';
@@ skipping 20 matching lines @@
         print(line);
       }
       print('');
       if (_printSummary) {
         _failureSummary.addAll(lines);
         _failureSummary.add('');
       }
     }
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (_printSummary) {
       if (!_failureSummary.isEmpty) {
         print('\n=== Failure summary:\n');
         for (var line in _failureSummary) {
           print(line);
         }
         print('');
 
         print(_buildSummaryEnd(_failedTests));
       }
@@ skipping 46 matching lines @@
   int _foundTests = 0;
   int _passedTests = 0;
   int _failedTests = 0;
   bool _allTestsKnown = false;
   DateTime _startTime;
 }
 
 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();
     }
     print('');
   }
 
   void _printDoneProgress(TestCase test) => _printProgress();
 
   void _printProgress();
@@ skipping 47 matching lines @@
     var status = 'pass';
     if (test.unexpectedOutput) {
       status = 'fail';
     }
     var percent = ((_completedTests() / _foundTests) * 100).toInt().toString();
     print('Done ${test.configurationString} ${test.displayName}: $status');
     print('@@@STEP_CLEAR@@@');
     print('@@@STEP_TEXT@ $percent% +$_passedTests -$_failedTests @@@');
   }
 
-  void allDone() {
+  void allDone(Configuration configuration) {
     if (!_failureSummary.isEmpty) {
       print('@@@STEP_FAILURE@@@');
       if (stepName != null) {
         print('@@@BUILD_STEP $stepName failures@@@');
       }
       for (String line in _failureSummary) {
         print(line);
       }
       print('');
     }
@@ skipping 102 matching lines @@
 }
 
 String _buildSummaryEnd(int failedTests) {
   if (failedTests == 0) {
     return '\n===\n=== All tests succeeded\n===\n';
   } else {
     var pluralSuffix = failedTests != 1 ? 's' : '';
     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]);
+  }
+}