Added json result of test output to output debug directory.

Added json result of test output to output debug directory.

R=whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/70e3e8aac11187ad454fce903c7d87a3ce62d30b

[mkroghj, 2017-09-01 09:36:44]

mkroghj@google.com changed reviewers:
+ whesse@google.com

[mkroghj, 2017-09-01 09:36:45]

ptal

[Bill Hesse, 2017-09-01 11:56:14]

Some comments.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
File tools/testing/dart/configuration.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:55: this.writeResultJsonLog,
Just writeResultLog, I think.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:78: this.debugOutputDirectory,
I wouldn't call it debugOutput, just output.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:428: /// of a test. Flags and properties
used for output is not included.
are not included

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
File tools/testing/dart/options.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:237: 'The name of the output directory for
storing debug and log files.',
for storing log files.  Does any other output actually go there? screenshots?
core dumps?

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:238: defaultsTo: "debug"),
default "logs"?

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:249: 'Write test results to a
"${TestUtils.testResultFileName}" json file '
.resultLogFileName

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_con...
File tools/testing/dart/test_configurations.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_con...
tools/testing/dart/test_configurations.dart:237: eventListener.add(new
TestResultLogWriter());
ResultLogWriter

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:676: var thisConf =
test.configuration.toSummaryMap();
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'];

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:687: 'commands':
test.commands.map((command) {
You could move the computation of this list outside and before the _results.add
call.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:697: if (test.unexpectedOutput) {
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.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:709: return {'name': command.displayName};
Put this up after "if (output == null)", then you don't need an else clause.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:718: var path = new
Path(configuration.debugOutputDirectory);
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.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:727: bool areSummaryMapsEqual(Map map1,
Map map2) {
Shouldn't be needed.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_run...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_run...
tools/testing/dart/test_runner.dart:1652:
listener.allDone(_globalConfiguration);
I think we can avoid this.  The one thing we need (the output log directory) can
be set in the when the listener is registered.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/utils.dart
File tools/testing/dart/utils.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/utils.da...
tools/testing/dart/utils.dart:449: static const testResultFileName =
"result.log";
resultLogName

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

I just returned with some comments/questions. The rest will be marked as done
when I can make the changes.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:676: var thisConf =
test.configuration.toSummaryMap();
On 2017/09/01 11:56:13, Bill Hesse wrote:
> 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'];
> 
>   

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?

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:697: if (test.unexpectedOutput) {
On 2017/09/01 11:56:13, Bill Hesse wrote:
> 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.

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

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:709: return {'name': command.displayName};
On 2017/09/01 11:56:13, Bill Hesse wrote:
> Put this up after "if (output == null)", then you don't need an else clause.

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

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:718: var path = new
Path(configuration.debugOutputDirectory);
On 2017/09/01 11:56:13, Bill Hesse wrote:
> 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.

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.

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

LGTM with the proposed changes.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:676: var thisConf =
test.configuration.toSummaryMap();
On 2017/09/02 09:40:22, mkroghj wrote:
> On 2017/09/01 11:56:13, Bill Hesse wrote:
> > 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'];
> > 
> >   
> 
> 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?

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.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:709: return {'name': command.displayName};
On 2017/09/02 09:40:22, mkroghj wrote:
> On 2017/09/01 11:56:13, Bill Hesse wrote:
> > Put this up after "if (output == null)", then you don't need an else clause.
> 
> I don't think I have an output == null test somewhere.. :)

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

[mkroghj, 2017-09-04 08:13:37]

Description was changed from

==========
Added json result of test output to output debug directory.
==========

to

==========
Added json result of test output to output debug directory.

R=whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/70e3e8aac11187ad454fce903c7d87a3ce62d30b
==========

[mkroghj, 2017-09-04 08:13:38]

Committed patchset #2 (id:20001) manually as
70e3e8aac11187ad454fce903c7d87a3ce62d30b (presubmit successful).

[mkroghj, 2017-09-04 10:43:35]

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
File tools/testing/dart/configuration.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:55: this.writeResultJsonLog,
On 2017/09/01 11:56:13, Bill Hesse wrote:
> Just writeResultLog, I think.

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:78: this.debugOutputDirectory,
On 2017/09/01 11:56:13, Bill Hesse wrote:
> I wouldn't call it debugOutput, just output.

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:428: /// of a test. Flags and properties
used for output is not included.
On 2017/09/01 11:56:13, Bill Hesse wrote:
> are not included

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
File tools/testing/dart/options.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:237: 'The name of the output directory for
storing debug and log files.',
On 2017/09/01 11:56:13, Bill Hesse wrote:
> for storing log files.  Does any other output actually go there? screenshots?
> core dumps?

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:238: defaultsTo: "debug"),
On 2017/09/01 11:56:13, Bill Hesse wrote:
> default "logs"?

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:249: 'Write test results to a
"${TestUtils.testResultFileName}" json file '
On 2017/09/01 11:56:13, Bill Hesse wrote:
> .resultLogFileName

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_con...
File tools/testing/dart/test_configurations.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_con...
tools/testing/dart/test_configurations.dart:237: eventListener.add(new
TestResultLogWriter());
On 2017/09/01 11:56:13, Bill Hesse wrote:
> ResultLogWriter

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:687: 'commands':
test.commands.map((command) {
On 2017/09/01 11:56:13, Bill Hesse wrote:
> You could move the computation of this list outside and before the
_results.add
> call.

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_run...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/test_run...
tools/testing/dart/test_runner.dart:1652:
listener.allDone(_globalConfiguration);
On 2017/09/01 11:56:14, Bill Hesse wrote:
> I think we can avoid this.  The one thing we need (the output log directory)
can
> be set in the when the listener is registered.

Done.

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/utils.dart
File tools/testing/dart/utils.dart (right):

https://codereview.chromium.org/3005013002/diff/1/tools/testing/dart/utils.da...
tools/testing/dart/utils.dart:449: static const testResultFileName =
"result.log";
On 2017/09/01 11:56:14, Bill Hesse wrote:
> resultLogName

Acknowledged.