[webkit-reviews] review denied: [Bug 125339] Enable running layout tests needing http server outside LayoutTests/http/tests : [Attachment 218840] Added missing config file for unit tests

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 125339: Enable running layout tests needing http server outside
LayoutTests/http/tests
https://bugs.webkit.org/show_bug.cgi?id=125339

Attachment 218840: Added missing config file for unit tests
https://bugs.webkit.org/attachment.cgi?id=218840&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=218840&action=review


I think the overall structure of the code seems good but there is a lot we can
improve here.

> Tools/Scripts/webkitpy/port/base.py:916
> +    def set_http_tests_configuration_filename(self, path, filename):
> +	   self._http_tests_configuration_filename =
self._filesystem.join(path, filename)
> +	   self._http_tests_configuration = None
> +

Looks like this method is only used by the test port.
It's probably better to just directly set _http_tests_configuration_filename
there
or let __init__ take an optional argument instead.

> Tools/Scripts/webkitpy/port/driver.py:227
> +	   return self._port.get_http_tests_configuration().is_http(test_name)

I don't think is_http makes sense as the method name since we're testing
whether a given test is a http test or not.
We should probably call it is_http_test.

> Tools/Scripts/webkitpy/port/driver.py:234
> +	   if conf.is_http(test_name):
> +	       return conf.get_uri_from_test_name(test_name)
> +	   return path.abspath_to_uri(self._port.host.platform,
self._port.abspath_for_test(test_name))

Why are we inverting the condition here? Our convention is to use early returns
for uncommon code paths
so that we wouldn't have deeply nested if's.

> Tools/Scripts/webkitpy/port/http_test_conf.py:35
> +class HttpTestsConf(object):

We normally use "config" instead of "conf" as the abbreviation for
configuration.

> Tools/Scripts/webkitpy/port/http_test_conf.py:41
> +	   self.configuration = json.loads(conf_file.read())
> +	   conf_file.close()
> +	   self.tests_configuration = self.configuration['tests']
> +	   self.uris_configuration = self.configuration['uris']

Private variables should be prefixed with _ per our convention.
Also, it's not clear to me why we'd need tests_configuration and
uris_configuration in addition to configuration.
We need to have either, not both.

> Tools/Scripts/webkitpy/port/http_test_conf.py:43
> +    def get_aliases(self, root_dir, filesystem):

Our method naming convention is to use "get" prefix for methods with out
argument.
See https://www.webkit.org/coding/coding-style.html#names-out-argument

> Tools/Scripts/webkitpy/port/http_test_conf.py:51
> +	   # reverse match search based on '/', starting from full test_name

This comment seems unnecessary.

> Tools/Scripts/webkitpy/port/http_test_conf.py:56
> +	       lastPos = sub_test_name.rfind('/')

Nit: last_pos.
But really, we should be using filesystem.split here instead.

> Tools/Scripts/webkitpy/port/http_test_conf.py:62
> +	   # reverse match search based on '/', starting from full uri

Ditto.

> Tools/Scripts/webkitpy/port/http_test_conf.py:68
> +	       lastPos = sub_uri.rfind('/')
> +	       if lastPos == -1:

Ditto.

> Tools/Scripts/webkitpy/port/http_test_conf.py:95
> +    def requires_ws(self, test_name):

We should spell out web socket instead of using the abbreviation ws.