| Summary: | Enable running layout tests needing http server outside LayoutTests/http/tests | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||||||||
| Severity: | Normal | CC: | ap, aroben, buildbot, commit-queue, dpranke, glenn, rniwa, sergio | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
youenn fablet
2013-12-06 00:16:20 PST
Created attachment 218594 [details]
wip
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. 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.
Created attachment 218755 [details]
Version passing python unit tests
Created attachment 218840 [details]
Added missing config file for unit tests
Should be complete this time. ping review 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. 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). 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. (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). (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 Created attachment 219419 [details]
Updated according first review
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 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
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 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
Created attachment 219624 [details]
Fixed config file
Whoops, I just realized that I didn't review this before the holidays ... I will try to get to it this afternoon. Sorry! 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.
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. |