[webkit-reviews] review granted: [Bug 137353] Add --json flag to test-webkitpy to emit JSON formatted test results on stdout : [Attachment 239739] Changes to use the _print_results_as_json method, as discussed in person.

Mon Oct 13 15:27:27 PDT 2014

Daniel Bates <dbates at webkit.org> has granted Jake Nielsen
<jake.nielsen.webkit at gmail.com>'s request for review:
Bug 137353: Add --json flag to test-webkitpy to emit JSON formatted test
results on stdout
https://bugs.webkit.org/show_bug.cgi?id=137353

Attachment 239739: Changes to use the _print_results_as_json method, as
discussed in person.
https://bugs.webkit.org/attachment.cgi?id=239739&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239739&action=review

> Tools/ChangeLog:4
> +	   on stdout

Nit: on => to

> Tools/ChangeLog:11
> +	   Adds the read method to MockFile to allow the stdout of
test_webkitpy
> +	   to be redirected without causing erroneous test failures.

For you consideration, I suggest adding a remark about the test(s) that failed
without this change.

> Tools/Scripts/webkitpy/port/server_process_unittest.py:71
> +	   # This means EOF

Although it can be implied that this comment is with respect to the line above
it given the length of the body of this function, it would be better to be
explicit about this relationship and either move the comment to actually be
above the return statement (line 70) or inline the comment on the same line as
the return statement. You may also want to consider writing out End of file
instead of abbreviating it as EOF to be explicit even though I suspect our
audience is familiar with the abbreviation of EOF to mean End of file.

> Tools/Scripts/webkitpy/test/main.py:36
>  import time
>  import traceback
>  import unittest
> +import operator
> +import json

Please order the imports in this file such that they are sorted according to
the output of the UNIX sort command.

> Tools/Scripts/webkitpy/test/main.py:108
> +			     help='emit JSON formatted test results on stdout')

Nit: on => to

The word "emit" in this help message can be taken as a synonym for the verb
"output". I suggest that we use the word "write" instead of the word "emit" to
refer to the action of writing JSON output so as to be consistent with the
language suggested for the word "output" on p.114 of the Apple Style Guide
(April 2013), <https://help.apple.com/asg/mac/2013/ASG_2013.pdf>.

> Tools/Scripts/webkitpy/test/main.py:128
> +    def _print_results_as_json(self, stream, test_names, failures, errors):

It is sufficient to make this a non-member function. I don't see the need to
make this a member function that takes a parameter called self as this method
does not make use of any instance data.

> Tools/Scripts/webkitpy/test/main.py:135
> +	   results['tests'] = sorted(test_names)

:( It is OK to include a list of the names of all the tests we ran assuming we
address the inefficiency of this line (see my remark on line 176 for more
details). I wish we could avoid including such a list in the JSON output as it
guarantees that the size of the JSON file is at least proportional to the total
number of tests ran. In contrast, the JSON output for the layout tests results,
full_results.json, is proportional to the number of tests that failed
(including flaky tests). I know you mentioned in person today (10/13) that you
felt that knowing the names of all tests than ran is necessary to identify
flaky tests. It would great if we could find a way to identify flaky tests
without having to include the names of all the tests that ran in the JSON
output. Can we derived this data from the list of tests that failed (failures),
the list of the tests that had a Python error (errors) and/or other metrics?
Notice that we have logic to identify flaky tests when generating the layout
test results JSON output without necessitating inserting a list of the names of
all the tests that ran in the JSON output. You mentioned you were unhappy with
this approach, but I don't recall the reason. It would be beneficial to
understand how the shortcomings (what are they?) of the test flakiness
detection we use for layout test results and future flakiness detection for
Python unit tests (do we know of any flaky tests?) are overcome by including in
the JSON output a list of the names of all the tests that ran.

> Tools/Scripts/webkitpy/test/main.py:136
> +

Nit: I don't feel that this empty line improves the readability of this
function. I suggest removing it.

> Tools/Scripts/webkitpy/test/main.py:137
> +	   stream.write(json.dumps(results))

Notice that json.dump() (intentional omission of the 's' in the word "dumps")
takes a File-like object as output (*). I suggest we take advantage of this
convenience API and write this line as:

json.dump(results, stream)

On another note, you may want to consider specifying the keyword argument
separators=(',', ':') towards generating more compact JSON output (since we
envision the primary consumer being another computer program/web page as
opposed to a human being). This would also make the separators used in this
JSON output consistent with the separators used in the JSON output for the
layout test results, full_results.json.

(*) See <https://docs.python.org/2/library/json.html#basic-usage> for more
details on the usage of json.dump().

> Tools/Scripts/webkitpy/test/main.py:176
> +	       self._print_results_as_json(sys.stdout, parallel_tests +
serial_tests, test_runner.failures, test_runner.errors)

This is inefficient. In particular, it is space and time inefficient to
concatenate the list parallel_tests and serial_tests, which allocates a new
list, given we are going to sort this concatenated list using sorted(), which
also allocates a new list. Either we should pass an iterator (i.e.
itertools.chain(parallel_tests, serial_tests)) for the second argument to
_print_results_as_json() or we should modify _print_results_as_json() so that
it sorts tests_names in-place (i.e. use test_names.sort()). If we choose the
latter approach, then I suggest we add a docstring (**) that explains the
purpose of _print_results_as_json() and documents that it mutates the argument
test_names.

(**) See <http://legacy.python.org/dev/peps/pep-0257/> for more details on
docstrings.