[webkit-reviews] review denied: [Bug 137353] Make test-webkitpy generate a json file of its results : [Attachment 239345] Adds Config object logic, and changes runtest.py to make sure it doesn't accidentally circumvent mock injection

[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 239345: Adds Config object logic, and changes runtest.py to make
sure it doesn't accidentally circumvent mock injection
https://bugs.webkit.org/attachment.cgi?id=239345&action=review

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


> Tools/ChangeLog:8
> +	   The webkit buildbot page's results.html file should contain results

Nit: webkit => WebKit

> Tools/ChangeLog:9
> +	   for the python unit tests (and soon to be others), as should the EWS


Nit: python => Python

> Tools/ChangeLog:31
> +	   in an equivelant TestSuiteResults object.

Nit: equivelant => equivalent

> Tools/Scripts/webkitpy/test/main.py:78
> +	   self._filesystem = filesystem or FileSystem()

Notice that Finder stores the passed FileSystem object in the public instance
variable, Finder.filesystem. It seems sufficient to go through Finder to get
the FileSystem object instead of explicitly storing a reference to it in a
private instance variable (i.e. self.finder.filesystem).

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

Nit: " => '
(double quote) to (single quote) for the empty string literal defined as the
default value of parameter default.

> Tools/Scripts/webkitpy/test/main.py:170
> +	      
self._filesystem.write_text_file(self._filesystem.join(self._options.results_di
rectory, "results.json"),
test_runner.test_suite_results.test_suite_results_to_json_string())

Nit: " => '

(double quote) to (single quotes) for the string literal 'results.json'.

> Tools/Scripts/webkitpy/test/results.py:26
> +class TestSuiteResults(object):

This class is OK as-is. Having said that it seems sufficient to have non-member
functions that deserialize and serialize test result data as opposed to
defining a class with such functionality.

> Tools/Scripts/webkitpy/test/results.py:44
> +	       test_result_dict['did_pass'] = test_result_tuple[1]

I take it that did_pass cannot be inferred from the values of failure_messages
and error_messages?

> Tools/Scripts/webkitpy/test/results.py:54
> +	       self.list_of_test_results = initial_list_of_test_results

Would it make sense to make self.list_of_test_results a private instance
variable? Or remove list_of_failed_test_results() and
list_of_errored_test_results()? I mean, it seems unnecessary to both expose all
the tests and provide convenience methods to filter the tests as a client can
perform the filtering themselves.

> Tools/Scripts/webkitpy/test/runner.py:59
> +	   self.test_suite_results.add_test_result(test_name=test_name,
did_pass=not bool(failures + errors), failure_messages=failures,
error_messages=errors)

Nit: I suggest removing the parameter name for the first argument (test_name)
as its purpose is clear from the name of its argument (the local variable named
test_name):

self.test_suite_results.add_test_result(test_name, did_pass=not bool(failures +
errors), failure_messages=failures, error_messages=errors)

See my question about did_pass in
TestSuiteResults.test_suite_results_to_json_string(). If did_pass can be
inferred from the values of failure_messages and error_messages then we can
further simplify this line of code.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:73
> +	       filesystem = self._tool.filesystem
> +	       port_name = self._tool.deprecated_port().name()
> +
> +	       if not type(port_name) is str:
> +		   port_name = port_name.__name__
> +
> +	       config = Config(self._tool.executive, filesystem, port_name)
> +
> +	       python_unittests_results_dir =
filesystem.join(config.build_directory(config.default_configuration()),
"python-unittest-results")
> +	       filesystem.maybe_make_directory(python_unittests_results_dir)
>  
>	       python_unittests_command =
self._tool.deprecated_port().run_python_unittests_command()
> +	       python_unittests_command.append("--results-directory")
> +	       python_unittests_command.append(python_unittests_results_dir)
> +

I envisioned calling Config.build_directory() through an instance of class
Port. In particular, I envisioned adding a method to class Port similar to
Port.default_results_directory(), say Port.python_unittest_results_directory(),
that returned the path to the Python unit tests results directory:

def python_unittest_results_directory(self):
    return self._build_path('python-unittest-results')

Then we can write this code to have the form:

...
if not self._options.non_interactive:
    filesystem = self._tool.filesystem
    python_unittest_results_directory =
self._tool.port_factory.get().python_unittest_results_directory()
    filesystem.maybe_make_directory(python_unittests_results_dir)

    # FIXME: We should teach the commit-queue and the EWS how to run these
tests.

    python_unittests_command =
self._tool.deprecated_port().run_python_unittests_command()
    python_unittests_command.extend(['--results-directory',
python_unittest_results_directory])
...

If we have Tools/Scripts/webkitpy/test/main.py create the specified results
directory (if it does not already exist) then we can remove the call to
FileSystem.maybe_make_directory() and simplify this code to have the form:

...
if not self._options.non_interactive:
    # FIXME: We should teach the commit-queue and the EWS how to run these
tests.

    python_unittests_command =
self._tool.deprecated_port().run_python_unittests_command()
    python_unittests_command.extend(['--results-directory',
self._tool.port_factory.get().python_unittest_results_directory()])
...