[webkit-reviews] review denied: [Bug 137353] Make test-webkitpy generate a json file of its results : [Attachment 239662] Changes to the --json flag solution to the problem, as discussed in person.

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jake Nielsen
<jake.nielsen.webkit at gmail.com>'s request for review:
Bug 137353: Make test-webkitpy generate a json file of its results
https://bugs.webkit.org/show_bug.cgi?id=137353

Attachment 239662: Changes to the --json flag solution to the problem, as
discussed in person.
https://bugs.webkit.org/attachment.cgi?id=239662&action=review

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


> Tools/Scripts/webkitpy/test/main.py:120
> +	       self.printer = JSONPrinter(self._stream_override or sys.stdout,
self._options)

How did you come the decision to emit the JSON output to standard output by
default given that we emit the human readable output to standard error by
default?

> Tools/Scripts/webkitpy/test/main.py:124
> +	   self.printer.print_started_test_suite("Python Unit Tests")

How did you come to the decision to emit this text?

> Tools/Scripts/webkitpy/test/printers/human_readable_printer.py:111
> +	   elif not test_did_pass:
> +	       lines = []
> +	       suffix = ' failed'
> +	       self.num_failures += 1

We will never execute this code since we handle the cases when failures and
errors are non-empty lists (above) and "not test_did_pass" only evaluates to
true if either failures or errors is a non-empty list.

> Tools/Scripts/webkitpy/test/printers/json_printer.py:76
> +    def print_started_test_suite(self, test_suite_name):
> +	   self.stream.write("{")
> +	   self.stream.write("\"suite_name\" : \"%s\",\n" % test_suite_name)
> +	   self.stream.write("\"list_of_tests\" : [\n")
> +
> +    def write_update(self, msg):
> +	   pass
> +
> +    def print_started_test(self, source, test_name):
> +	   pass
> +
> +    def print_finished_test(self, source, test_name, test_did_pass,
test_time, failures, errors):
> +	   self._number_of_tests_run += 1
> +	   if not self._first_test_flag:
> +	       self.stream.write(",\n")
> +
> +	   test_dict = {}
> +	   test_dict["test_name"] = test_name
> +	   test_dict["test_did_pass"] = test_did_pass
> +	   test_dict["failures"] = failures
> +	   test_dict["errors"] = errors
> +
> +	   self.stream.write(json.dumps(test_dict))
> +
> +	   self._first_test_flag = False
> +
> +    def print_result(self, run_time):
> +	   self.stream.write("],\n\"test_run_time\" :
%s,\n\"number_of_tests_run\" : %s}" % (run_time, self._number_of_tests_run))

How did you come to the decision to piecewise write the JSON data to the
stream? I suggest that we build the test_dict structure in memory and then
implement print_result() to write the result of json.dump(test_dict) to stream
self.stream. The benefit of this approach is that we do not need to explicitly
emit any JSON markup ourself in print_started_test_suite() or
print_finished_test(), which is brittle and error prone.

> Tools/Scripts/webkitpy/test/runner.py:65
> +	   self.printer.print_finished_test(source, test_name, not
bool(failures + errors), delay, failures, errors)

I don't see the value of the new argument, tests_did_pass, given that the
printer is passed the list of failures and errors and can compute
tests_did_pass itself.