Bug 127094

Created attachment 221665 [details] WIP

First patch implements the support as follows: - The wpt server is started for all tests inside imported/w3c - The doc_root of the server is set to imported/w3c - A specific route is added to serve LayoutTests/resources scripts for all urls starting as /resource/*. - Running the server reuses as much as possible the web-platform-test serve.py script I needed to make some small modifications to wptserver and serve.py. Once stabilized, I hope they can be upstreamed to the respective projects

> I needed to make some small modifications to wptserver and serve.py. > Once stabilized, I hope they can be upstreamed to the respective projects It would be great if you could make a PR on github for these asap, even if they are not stable yet. That way we can make sure that this work goes in a direction that works for everyone, rather than having nasty surprises later.

(In reply to comment #3) > > I needed to make some small modifications to wptserver and serve.py. > > Once stabilized, I hope they can be upstreamed to the respective projects > > It would be great if you could make a PR on github for these asap, even if they are not stable yet. That way we can make sure that this work goes in a direction that works for everyone, rather than having nasty surprises later. Done: https://github.com/w3c/web-platform-tests/pull/540 https://github.com/w3c/wptserve/pull/3

Created attachment 222470 [details] Functional version The patch only contains webkit specific files.

Comment on attachment 222470 [details] Functional version View in context: https://bugs.webkit.org/attachment.cgi?id=222470&action=review We should probably auto-install wptserve in third_party directory. > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:77 > + self._needs_wpt = None We should probably use the full name "wptserve" or web_platform_test_server. I would prefer using the full name. > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:212 > + self._printer.write_update('Starting WPT server ...') Can we either call it "wptserve" or "Web Platform Test Server"? > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:225 > + self._printer.write_update('Stopping WPT server ...') Ditto. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:292 > + option_group_definitions.append(("Web Platform Tests Options", [ Nit: "Web Platform Test Server Options" > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:293 > + optparse.make_option("--wpt-dir", type="string", default=os.path.join("imported", "w3c"), We should probably use "--wptserve" or "--wpts" as the prefix. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:294 > + help=("set web platform server document root, relative to LayoutTests directory")), Nit: Please capitalize "set" and ditto for other help texts. > Tools/Scripts/webkitpy/layout_tests/servers/wpt/launcher.py:1 > +"""Script to set up and launch web platform test servers in the context of WebKit framework.""" We don't add these comments to new files. It should be self-explanatory from the file name. For that reason, I would prefer naming it web_platform_test_server instead of wpt.

Created attachment 222731 [details] Added auto-install and unit tests

ping review?

Created attachment 224176 [details] fixed default websocket handlers path

Created attachment 233360 [details] WIP: Removed wptserve autoinstall and direct use of imported W3C repo wptserve

Created attachment 238156 [details] some clean up

Created attachment 238712 [details] updated according bug 134763 discussions

Comment on attachment 238712 [details] updated according bug 134763 discussions View in context: https://bugs.webkit.org/attachment.cgi?id=238712&action=review > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_launcher.py:1 > +# Copyright (c) 2014, Canon Inc. All rights reserved. Isn't this copyrighted by someone else since you're copying code from serve.py? Also, what's the license of serve.py? Is that BSD? > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_launcher.py:35 > +sys.path.insert(1, ".") > +import serve as WebPlatformTestServer Can't we use imp.load_module instead since we're loading one module? > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_launcher.py:51 > + # Wait until: > + # 1. user presses enter (in case script executed in command line) > + # 2. the master process sends enter (regular stop command) > + # 3. the master process is stopped (and stdin is closed) It seems like this comment just repeats what the code below says, and it's also somewhat inaccurate. I'd prefer leaving it out instead. > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:44 > + def __init__(self, port_obj, name, output_dir, pidfile=None): > + """Args: > + output_dir: the absolute path to the layout test result directory > + """ Instead of adding a comment like that, rename output_dir to layout_test_results_dir or something. > Tools/Scripts/webkitpy/port/base.py:911 > + """Start a web server. Raise an error if it can't start or is already running. > + > + Ports can stub this out if they don't need a web server to be running.""" We don't normally add comments like this. Please remove it. > Tools/Scripts/webkitpy/port/base.py:950 > + """Shut down the web platform test server if it is running. Do nothing if it isn't.""" Ditto. > Tools/Scripts/webkitpy/port/driver.py:161 > + self.web_platform_test_dir = self._port.web_platform_test_server_doc_root() + "/" > + Why do we need to manually append '/'? > Tools/Scripts/webkitpy/port/driver.py:236 > + # Any change to WPT_BASE_URL should be synced with LayoutTests/imported/w3c/resources/config.json > + WPT_BASE_URL = "http://localhost:8800/" Can we read this value out of config.json instead? > Tools/Scripts/webkitpy/port/driver.py:402 > + elif self.is_http_test(driver_input.test_name) or self.is_web_platform_test(driver_input.test_name): There are two spaces after or. > Tools/Scripts/webkitpy/port/driver_unittest.py:89 > + Superflous change here.

(In reply to comment #13) > Comment on attachment 238712 [details] > updated according bug 134763 discussions > > View in context: > https://bugs.webkit.org/attachment.cgi?id=238712&action=review > > > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_launcher.py:1 > > +# Copyright (c) 2014, Canon Inc. All rights reserved. > > Isn't this copyrighted by someone else since you're copying code from > serve.py? > Also, what's the license of serve.py? Is that BSD? Thanks for the review. The license is BSD: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html Concerning the copyright, I am not sure what should be done. There is no copyright in the original serve.py and I read somewhere that all new source/script files added to WebKit should have copyright. I can make an explicit reference to the fact that most code is borrowed from W3C serve.py.

(In reply to comment #14) > > The license is BSD: > http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html > > Concerning the copyright, I am not sure what should be done. > There is no copyright in the original serve.py and I read somewhere that all > new source/script files added to WebKit should have copyright. I can make an > explicit reference to the fact that most code is borrowed from W3C serve.py. Cam we ask James Graham <james@hoppipolla.co.uk> about that? As I understand it, he is the primary author of WPT.

I suggest we add some text like # This Source Code Form is subject to the terms of the 3-cluase BSD License # http://www.w3.org/Consortium/Legal/2008/03-bsd-license.html to the top of the files upstream. Would that be sufficient to meet your requirements?

Comment on attachment 238712 [details] updated according bug 134763 discussions View in context: https://bugs.webkit.org/attachment.cgi?id=238712&action=review >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_launcher.py:35 >> +import serve as WebPlatformTestServer > > Can't we use imp.load_module instead since we're loading one module? WebPlatformTestServer is using other modules located in wpt tools folder. >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_launcher.py:51 >> + # 3. the master process is stopped (and stdin is closed) > > It seems like this comment just repeats what the code below says, and it's also somewhat inaccurate. > I'd prefer leaving it out instead. OK >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:44 >> + """ > > Instead of adding a comment like that, rename output_dir to layout_test_results_dir or something. OK to remove the comment, output_dir is clear as name anyway. >> Tools/Scripts/webkitpy/port/base.py:911 >> + Ports can stub this out if they don't need a web server to be running.""" > > We don't normally add comments like this. Please remove it. OK >> Tools/Scripts/webkitpy/port/base.py:950 >> + """Shut down the web platform test server if it is running. Do nothing if it isn't.""" > > Ditto. OK >> Tools/Scripts/webkitpy/port/driver.py:161 >> + > > Why do we need to manually append '/'? To be consistent with other dir variables, like HTTP_DIR and HTTP_LOCAL_DIR I will refactor the code to make the addition in port itself using TEST_PATH_SEPARATOR. >> Tools/Scripts/webkitpy/port/driver.py:236 >> + WPT_BASE_URL = "http://localhost:8800/" > > Can we read this value out of config.json instead? Will do that. >> Tools/Scripts/webkitpy/port/driver.py:402 >> + elif self.is_http_test(driver_input.test_name) or self.is_web_platform_test(driver_input.test_name): > > There are two spaces after or. OK >> Tools/Scripts/webkitpy/port/driver_unittest.py:89 >> + > > Superflous change here. OK

Created attachment 243556 [details] Updated according review

Created attachment 243671 [details] Patch

(In reply to comment #19) > Created attachment 243671 [details] > Patch Updated style and fixed unit tests

Comment on attachment 243671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243671&action=review > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:42 > + config_wk_filename = port_obj.path_from_webkit_base("LayoutTests", "imported", "w3c", "resources", "config.json") I would call this config_wk_filepath instead since this is a file path, not just a file name. > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:79 > + def _set_start_cmd(self, port_obj): > + file_path = self._filesystem.abspath(self._filesystem.split(__file__)[0]) > + self._start_cmd = ["python", self._filesystem.join(file_path, "web_platform_test_launcher.py")] > + self._cwd = port_obj.path_from_webkit_base("LayoutTests", self._doc_root) I don't think there's any point in separating this method. Also, please give _cmd a more descriptive name such as doc_root_path. > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:86 > + for f in self._resources_files_to_copy: > + webkit_filename = self._filesystem.join(self._layout_root, "resources", f) > + if self._filesystem.isfile(webkit_filename): > + self._filesystem.copyfile(webkit_filename, self._filesystem.join(self._doc_root, "resources", f)) Why do we need to do this? How can WPT tests depend on WebKit files? What happens to stale files if run-webkit-tests is killed in the middle of running tests? > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:90 > + config_wk_filename = self._filesystem.join(self._layout_root, "imported", "w3c", "resources", "config.json") > + if self._filesystem.isfile(config_wk_filename): > + self._filesystem.copyfile(config_wk_filename, self._filesystem.join(self._doc_root, "config.json")) Can we either refer to ../resources/config.json in web_platform_test_launcher.py or copy it into the directory when we import the tests? > Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:103 > + def _prepare_config(self): This function does a lot more than preparing the config.json. We should rename to reflect what it does. > Tools/Scripts/webkitpy/port/driver.py:163 > + self.web_platform_test_dir = self._port.web_platform_test_server_doc_root() We should be consistent in naming. web_platform_test_dir or web_platform_test_server_doc_root. > Tools/Scripts/webkitpy/port/driver.py:164 > + self.web_platform_test_base_url = self._port.web_platform_test_server_base_url() Ditto.

Comment on attachment 243671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243671&action=review >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:86 >> + self._filesystem.copyfile(webkit_filename, self._filesystem.join(self._doc_root, "resources", f)) > > Why do we need to do this? How can WPT tests depend on WebKit files? > What happens to stale files if run-webkit-tests is killed in the middle of running tests? WPT tests rely on testharness.js, testharnessreport.js and testharness.css. testharnessreport.js is specific to WebKit (use of window.testRunner API). Since tests are served through WPT server, we need to have these files within the server root dir. More information is available at bug 134763. If rwt gets killed, the files will remain until rwt is launched again and files will get overwritten/deleted. >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:90 >> + self._filesystem.copyfile(config_wk_filename, self._filesystem.join(self._doc_root, "config.json")) > > Can we either refer to ../resources/config.json in web_platform_test_launcher.py > or copy it into the directory when we import the tests? We can do that at import time but this would duplicate files in the repo. Copying the file is also consistent with testharness files handling.

Comment on attachment 243671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243671&action=review >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:42 >> + config_wk_filename = port_obj.path_from_webkit_base("LayoutTests", "imported", "w3c", "resources", "config.json") > > I would call this config_wk_filepath instead since this is a file path, not just a file name. OK >> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:103 >> + def _prepare_config(self): > > This function does a lot more than preparing the config.json. We should rename to reflect what it does. _prepare_config is a bit misleading as it conflicts with config.json Since it is an overriden method and called from the HttpServerBase source code, we cannot easily rename it. >> Tools/Scripts/webkitpy/port/driver.py:163 >> + self.web_platform_test_dir = self._port.web_platform_test_server_doc_root() > > We should be consistent in naming. web_platform_test_dir or web_platform_test_server_doc_root. OK >> Tools/Scripts/webkitpy/port/driver.py:164 >> + self.web_platform_test_base_url = self._port.web_platform_test_server_base_url() > > Ditto. OK

Created attachment 243806 [details] Improving variable names

(In reply to comment #24) > Created attachment 243806 [details] > Improving variable names Ping review?

Created attachment 244510 [details] Updating SSL support

Comment on attachment 244510 [details] Updating SSL support Rejecting attachment 244510 [details] from commit-queue. youennf@gmail.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 244510 [details] Updating SSL support Rejecting attachment 244510 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 244510, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Tools/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/6605731331571712

Created attachment 245361 [details] fixing change log

Comment on attachment 245361 [details] fixing change log Clearing flags on attachment: 245361 Committed r179134: <http://trac.webkit.org/changeset/179134>

All reviewed patches have been landed. Closing bug.

Comment on attachment 245361 [details] fixing change log View in context: https://bugs.webkit.org/attachment.cgi?id=245361&action=review > LayoutTests/imported/w3c/resources/config.json:3 > + "https":[8443], Doesn't this conflict with Apache https port?

(In reply to comment #32) > Comment on attachment 245361 [details] > fixing change log > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245361&action=review > > > LayoutTests/imported/w3c/resources/config.json:3 > > + "https":[8443], > > Doesn't this conflict with Apache https port? Thanks, good catch. Yes, the config should be changed. A similar issue may arise with wpt websocket server. I filed bug 141157 to capture this.