Bug 125339 – Enable running layout tests needing http server outside LayoutTests/http/tests

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[youenn fablet]

Currently HTTP test servers are only launched for tests inside LayoutTests/http/tests.
It would be good to extend that, at least for future imported w3c tests (within LayoutTests/imported/w3c).
It would be also nice to extract the rules to trigger HTTP servers outside the scripts.

More information in the mail thread starting at https://lists.webkit.org/pipermail/webkit-dev/2013-December/025906.html

[youenn fablet]

Created attachment 218594 [details]
wip

[youenn fablet]

Uploaded patch for review.
Modified scripts to handle triggering of http, https and ws according an external config file LayoutTests/http/conf/http-test-conf.json.
Uploaded an example of LayoutTests/http/conf/http-test-conf.json that mimics current rules.
It also triggers http server for any test in LayoutTests/imported/w3c/XMLHttpRequest.
It adds aliases to enable access to LayoutTests/resources/*.js from these tests.

[Darin Adler]

Comment on attachment 218594 [details]
wip

Looks fine. I believe at least one platform is still using the old run-webkit-tests, and I don’t know what this means for that.

[youenn fablet]

Created attachment 218755 [details]
Version passing python unit tests

[youenn fablet]

Created attachment 218840 [details]
Added missing config file for unit tests

[youenn fablet]

Should be complete this time.

[youenn fablet]

ping review

[Ryosuke Niwa]

Comment on attachment 218840 [details]
Added missing config file for unit tests

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.

[Dirk Pranke]

I haven't done a detailed review of this yet. My initial reaction is that this adds more complexity to the setup than I'd really like; my initial leaning is that we should just serve *all* of LayoutTests/imported/w3c from http and call it good enough.

However, I think WebKit still only runs all of the http tests in a single thread, and even Blink only runs them in 25% of threads , so if we start running a lot of imported tests, this might be a bottle neck. It would be nice if there was some way other than manually tracking which directories did and did not actually require a web server, but I'm not sure if the w3c is going to be super excited to track that, as their basic model assumes tests are run off of a web server.

Perhaps the better thing to do is to focus on being able to run more tests in parallel off a web server without flakiness, I'm not sure.

At any rate, I'd like to do a more detailed review of this patch, but I don't want to block WebKit on this; if someone else (e.g., rniwa) wants to review/R+ it (eventually) and not wait on me, that's fine.

(I'm currently working again on the import process in Blink; where possible, I'd like to keep things as close to WebKit as possible, which is why I'm waffling on this at all).

Other things I'd note are that I think the map of which directories need or don't need the server should probably live at the top level under LayoutTests/ rather than in http/tests/conf . Also we should probably check for duplicates between LayoutTests/resources and LayoutTests/http/tests/resources (but this latter thing should be done in a separate patch).

[Ryosuke Niwa]

Another important question is whether we want to our http server setup for the imported W3C tests.  Given  W3C is going to use the new python http server written by jgraham, I'm not sure if using our existing server is a good idea or not.

[Dirk Pranke]

(In reply to comment #10)
> Another important question is whether we want to our http server setup for the imported W3C tests.  Given  W3C is going to use the new python http server written by jgraham, I'm not sure if using our existing server is a good idea or not.

True, sort of, except that I have no idea what the state of jgraham's server is, nor how widely that'll be adopted by people other than jgraham (and presumably mozilla), nor what will be done w/ the many existing tests that would need to be ported over.

(Note that I have no objections to jgraham's work, I just haven't really used it or looked at it. It surely would be nice to have a stable cross platform solution that didn't rely on LigHTTPd or cygwin apache on windows).

[youenn fablet]

(In reply to comment #9)
> I haven't done a detailed review of this yet. My initial reaction is that this adds more complexity to the setup than I'd really like; my initial leaning is that we should just serve *all* of LayoutTests/imported/w3c from http and call it good enough.

Ideally, it would be great to have one single straightforward mapping rule between test names and urls.
As a newcomer though, I find it as convenient to grasp the mapping rules from a config file than from python source files(personal taste probably).
Also, that would allow putting in that same config file the existing aliases (media-resources and js-test-resources).

> However, I think WebKit still only runs all of the http tests in a single thread, and even Blink only runs them in 25% of threads , so if we start running a lot of imported tests, this might be a bottle neck. It would be nice if there was some way other than manually tracking which directories did and did not actually require a web server, but I'm not sure if the w3c is going to be super excited to track that, as their basic model assumes tests are run off of a web server.
f> 
> Perhaps the better thing to do is to focus on being able to run more tests in parallel off a web server without flakiness, I'm not sure.
> 
> At any rate, I'd like to do a more detailed review of this patch, but I don't want to block WebKit on this; if someone else (e.g., rniwa) wants to review/R+ it (eventually) and not wait on me, that's fine.
> 
> (I'm currently working again on the import process in Blink; where possible, I'd like to keep things as close to WebKit as possible, which is why I'm waffling on this at all).

If possible, I would like to go on with this patch to start adding some w3c XHR tests.
When you finalize a better solution in Blink, I will be happy to help in porting it within webkit.

> Other things I'd note are that I think the map of which directories need or don't need the server should probably live at the top level under LayoutTests/ rather than in http/tests/conf . 

I will update the patch accordingly

[youenn fablet]

Created attachment 219419 [details]
Updated according first review

[Build Bot]

Comment on attachment 219419 [details]
Updated according first review

Attachment 219419 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/50028238

New failing tests:
http/tests/websocket/tests/hybi/workers/close-code-and-reason.html
http/tests/websocket/tests/hybi/bad-sub-protocol-non-ascii.html
http/tests/websocket/tests/hybi/workers/close.html
http/tests/websocket/tests/hybi/workers/close-in-shared-worker.html
http/tests/websocket/tests/hybi/workers/close-in-worker.html
http/tests/websocket/tests/hybi/close-code-and-reason.html
http/tests/websocket/tests/hybi/close-on-unload-and-force-gc.html
http/tests/websocket/tests/hybi/workers/receive-blob.html
http/tests/websocket/tests/hybi/close-before-open.html
http/tests/websocket/tests/hybi/broken-utf8.html
http/tests/websocket/tests/hybi/close-on-unload-reference-in-parent.html
http/tests/websocket/tests/hybi/bad-handshake-crash.html
http/tests/websocket/tests/hybi/workers/send-arraybufferview.html
http/tests/websocket/tests/hybi/bad-sub-protocol-empty.html
http/tests/websocket/tests/hybi/close-on-navigate-new-location.html
http/tests/websocket/tests/hybi/bufferedAmount-after-close.html
http/tests/websocket/tests/hybi/alert-in-event-handler.html
http/tests/websocket/tests/hybi/workers/close-in-onmessage-crash.html
http/tests/websocket/tests/hybi/client-close.html
http/tests/websocket/tests/hybi/workers/send-arraybuffer.html
http/tests/websocket/tests/hybi/binary-type.html
http/tests/websocket/tests/hybi/bad-sub-protocol-control-chars.html
http/tests/websocket/tests/hybi/workers/receive-arraybuffer.html
http/tests/websocket/tests/hybi/workers/send-blob.html
http/tests/websocket/tests/hybi/workers/multiple-subprotocols.html
http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html
http/tests/websocket/tests/hybi/workers/no-onmessage-in-sync-op.html
http/tests/websocket/tests/hybi/workers/no-subprotocol.html
http/tests/websocket/tests/hybi/workers/shared-worker-simple.html
http/tests/websocket/tests/hybi/close-event.html

[Build Bot]

Created attachment 219426 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219419 [details]
Updated according first review

Attachment 219419 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/46418019

New failing tests:
http/tests/websocket/tests/hybi/workers/close-code-and-reason.html
http/tests/websocket/tests/hybi/bad-sub-protocol-non-ascii.html
http/tests/websocket/tests/hybi/workers/close.html
http/tests/websocket/tests/hybi/workers/close-in-shared-worker.html
http/tests/websocket/tests/hybi/workers/close-in-worker.html
http/tests/websocket/tests/hybi/close-code-and-reason.html
http/tests/websocket/tests/hybi/close-on-unload-and-force-gc.html
http/tests/websocket/tests/hybi/workers/receive-blob.html
http/tests/websocket/tests/hybi/close-before-open.html
http/tests/websocket/tests/hybi/broken-utf8.html
http/tests/websocket/tests/hybi/bad-handshake-crash.html
http/tests/websocket/tests/hybi/workers/send-arraybufferview.html
http/tests/websocket/tests/hybi/bad-sub-protocol-empty.html
http/tests/websocket/tests/hybi/close-on-navigate-new-location.html
http/tests/websocket/tests/hybi/bufferedAmount-after-close.html
http/tests/websocket/tests/hybi/alert-in-event-handler.html
http/tests/websocket/tests/hybi/workers/close-in-onmessage-crash.html
http/tests/websocket/tests/hybi/client-close.html
http/tests/websocket/tests/hybi/workers/send-arraybuffer.html
http/tests/websocket/tests/hybi/binary-type.html
http/tests/websocket/tests/hybi/bad-sub-protocol-control-chars.html
http/tests/websocket/tests/hybi/workers/receive-arraybuffer.html
http/tests/websocket/tests/hybi/workers/send-blob.html
http/tests/websocket/tests/hybi/workers/multiple-subprotocols.html
http/tests/websocket/tests/hybi/bufferedAmount-after-close-in-busy.html
http/tests/websocket/tests/hybi/workers/no-onmessage-in-sync-op.html
http/tests/websocket/tests/hybi/workers/no-subprotocol.html
http/tests/websocket/tests/hybi/workers/shared-worker-simple.html
http/tests/websocket/tests/hybi/close-event.html
editing/caret/caret-color.html

[Build Bot]

Created attachment 219477 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[youenn fablet]

Created attachment 219624 [details]
Fixed config file

[Dirk Pranke]

Whoops, I just realized that I didn't review this before the holidays ... I will try to get to it this afternoon. Sorry!

[Ryosuke Niwa]

Comment on attachment 219624 [details]
Fixed config file

See my reply on webkit-dev.  W3C testing infrastructure team has already added the python server.  They're going to start converting existing tests to use the new server now.

It doesn't make sense for us to add the support for legacy tests written in PHP.

[Dirk Pranke]

I've replied to rniwa's post on webkit-dev; the PHP thing is a bit of a red herring, but there are real concerns / points about the complexity of supporting multiple directories for HTTP (as we see in this patch) and now of needing to support two different web servers as well.

Once that thread dies down (if it's not already dead), I'll summarize it the conclusion here as well.