[webkit-reviews] review denied: [Bug 137353] Make test-webkitpy generate a json file of its results : [Attachment 239227] Removes the TestResult(s) class.

[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 239227: Removes the TestResult(s) class.
https://bugs.webkit.org/attachment.cgi?id=239227&action=review

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


> Tools/ChangeLog:7
> +

We should add a description to this change log entry that describers the
reasoning behind this change based on your remark in comment #5 because the bug
title doesn't explain the reasoning for this change.

> Tools/Scripts/webkitpy/common/webkit_finder.py:39
> +    def webkit_results_dir(self):
> +	   path_to_build_dir = self._filesystem.join(self.webkit_base(),
"WebKitBuild")
> +	   configuration_dir =
self._filesystem.read_text_file(self._filesystem.join(path_to_build_dir,
"Configuration"))
> +	   return self._filesystem.join(path_to_build_dir, configuration_dir)
> +

This will not work for people that specify a custom build directory path using
the environment variable WEBKIT_OUTPUTDIR (see second paragraph on
<http://www.webkit.org/building/build.html>). Notice we have existing logic to
compute the correct build directory in Config.build_directory(). Can we make
use of this functionality?

> Tools/Scripts/webkitpy/test/main.py:108
> +	   parser.add_option('-r', '--results-dir', action='store', default="",


I don't see the value in abbreviating the word "directory" as "dir". Moreover,
--results-dir is inconsistent with the naming of the command line option
--results-directory used in existing tools for a similar purpose, including
run-webkit-tests.

> Tools/Scripts/webkitpy/test/main.py:109
> +			     help='specifies a directory in which to generate a
json file that contains the results of the tests')

Nit: json => JSON

(JSON is an acronym for JavaScript Object Notation).