[Webkit-unassigned] [Bug 125339] Enable running layout tests needing http server outside LayoutTests/http/tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=125339


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #218840|review?                     |review-
               Flag|                            |




--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org>  2013-12-16 13:08:30 PST ---
(From update of attachment 218840)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.