Bug 64564

Created attachment 100872 [details] Proposed implementation

Comment on attachment 100872 [details] Proposed implementation View in context: https://bugs.webkit.org/attachment.cgi?id=100872&action=review This looks great except for the web socket server issue! > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:356 > + httpTests = set() httpTests => http_tests (Python PEP8 style uses unix_hacke style names) > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:358 > + if test.find(self.HTTP_SUBDIR, 0) is not -1: This looks like a really elaborate "test.startswith(self.HTTP_SUBDIR)" > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:394 > + # Remove HTTP test if option set up. This comment can be removed. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:956 > + self._printer.print_update('Starting WebSocket server ...') > + self._port.start_websocket_server() Presumably we should still start the web socket server even if we're not going to run HTTP tests.

> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:358 > > + if test.find(self.HTTP_SUBDIR, 0) is not -1: > > This looks like a really elaborate "test.startswith(self.HTTP_SUBDIR)" It is not. It started as such but if you look at the test case, we make sure to ignore any test in platform/test/http/ (chromium has some of those). > > > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:956 > > + self._printer.print_update('Starting WebSocket server ...') > > + self._port.start_websocket_server() > > Presumably we should still start the web socket server even if we're not going to run HTTP tests. Good catch, the problem with that is that you would have to also grab the HTTP lock too to serialize 2 instances of NRWT running at the same time. ORWT did not make any difference between websockets and HTTP tests as they would live inside the http/ directory. Also we would not run the websockets tests in http/ with this change anyway so better IMHO to just add those tests to the skipped list too and not start the server. Any objections?

(In reply to comment #3) > > > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:358 > > > + if test.find(self.HTTP_SUBDIR, 0) is not -1: > > > > This looks like a really elaborate "test.startswith(self.HTTP_SUBDIR)" > > It is not. It started as such but if you look at the test case, we make sure to ignore any test in platform/test/http/ (chromium has some of those). There is also always re.search(self.http_subdir, test) of course. Not sure if that helps.

Created attachment 101021 [details] Proposed fix 2: Also don't run websocket tests and take Adam's comments into account.

Comment on attachment 101021 [details] Proposed fix 2: Also don't run websocket tests and take Adam's comments into account. View in context: https://bugs.webkit.org/attachment.cgi?id=101021&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:283 > + self.HTTP_SUBDIR = 'http' + port.TEST_PATH_SEPARATOR > + self.WEBSOCKET_SUBDIR = 'websocket' + port.TEST_PATH_SEPARATOR I've never understood why these need separartors in their names at all. I'm not sure why the original author added them that way. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:359 > + def _get_http_tests(self): We don't use _get_ in names (like our c++ style). Some of the other code from this file is inherited from chromium and doesn't match our python style correctly. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:361 > + """ Returns all the HTTP tests of HTTP. Returns a set containing > + them. """ This doens't actually add anything and should be removed. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:367 > + http_tests = set() > + for test in self._test_files: > + if self.HTTP_SUBDIR in test or self.WEBSOCKET_SUBDIR in test: > + http_tests.add(test) > + > + return http_tests This can easily be written as: return set([test for test in self._test_files if self.HTTP_SUBDIR in test or self.WEBSOCKET_SUBDIR in test]) You could even use filter/lambda if you wanted to shorten the line: is_http_test = lamdba test: self.WEBSOCKET_SUBDIR in test or self.WEBSOCKET_SUBDIR in test return set(filter(is_http_test, self._test_files)) > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:401 > + skipped = self._get_http_tests() Seems you should add/union here for future-proofness of your code. > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:405 > + skipped = skipped.union(self._expectations.get_tests_with_result_type( > + test_expectations.SKIP)) Like webkit C++ code, our python doesn't follow any wrapping rule. We only wrap to aid in readability. :) Which for better or worse 80c doen'st count as doing. :) > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:412 > + if len(skipped): > + self._test_files -= skipped This can also be if skipped: since bool([]) is false. But it doesn't really matter. I'm also surprised you even need to test. Why doesn't self._test_files -= set() just work? > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:955 > + if self._options.http: Better to early-return instead of indenting the whole function. > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:201 > + port = layout_tests.port.get(port_name=options.platform, options=options) You should be sure to pass a MockFileSystem and MockExectuive to the get() method. It's sadly confusing, but otherwise you'll end up talking to the real filesystem which is bad. :) Maybe we need a make_port() helper for this class? > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:204 > + printer = printing.Printer(port, options, StringIO.StringIO(), StringIO.StringIO(), > + configure_logging=True) No need to wrap. > Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:240 > + manager.clean_up_run() > + printer.cleanup() I guess manager/printer don't understand __entered__, __exited__ so can't we used with 'with' for auto-cleanup? > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:158 > + if not options.http and options.force: > + warnings.append("--no-http is ignored since --force is also provided") > + options.http = True What does --force do? I'm confused by this.

Comment on attachment 101021 [details] Proposed fix 2: Also don't run websocket tests and take Adam's comments into account. View in context: https://bugs.webkit.org/attachment.cgi?id=101021&action=review >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:283 >> + self.WEBSOCKET_SUBDIR = 'websocket' + port.TEST_PATH_SEPARATOR > > I've never understood why these need separartors in their names at all. I'm not sure why the original author added them that way. This code as originally written was looking for "LayoutTests/http/". I removed the "LayoutTests" part when I converted to "test_names". I thought I had deleted the first separator as well, but I must've missed that. >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:367 >> + return http_tests > > This can easily be written as: > > return set([test for test in self._test_files if self.HTTP_SUBDIR in test or self.WEBSOCKET_SUBDIR in test]) > > You could even use filter/lambda if you wanted to shorten the line: > > is_http_test = lamdba test: self.WEBSOCKET_SUBDIR in test or self.WEBSOCKET_SUBDIR in test > return set(filter(is_http_test, self._test_files)) see the code on line 541, which tests the string 'http' instead of the constants. Can you update that code in this patch as well, to be consistent? It might be best to rewrite test_requires_http_lock() as is_http_test() or is_websocket_test(). >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:955 >> + if self._options.http: > > Better to early-return instead of indenting the whole function. You shouldn't need this check at all. start_servers_with_lock() shouldn't be called if you don't have any http or websocket tests. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:655 > + self.assertTrue(passing_run(['--http'])) Look at test_dryrun or another test in this file that calls get_tests_run(). Personally, I would write tests here that actually test whether the http/* tests are run, and skip the unit tests in manager_unittest.py . The reason is that tests here are integration/functional tests, and it's much clearer what the test is doing and what the correct response would be.

Created attachment 101060 [details] Proposed fix 3: With more python-ness thanks to Eric & Dirk.

Comment on attachment 101060 [details] Proposed fix 3: With more python-ness thanks to Eric & Dirk. View in context: https://bugs.webkit.org/attachment.cgi?id=101060&action=review Looks great; just a few nits. Thanks for fixing this! > Tools/Scripts/webkitpy/layout_tests/port/test.py:181 > + # For --no-http tests. Nit: I might be a bit more verbose and note that we test to make sure platform-specific http tests are skipped as well. > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:654 > + self.assertTrue(passing_run(['--no-http'])) Nit: I wouldn't bother with this since you have test_no_http(), below.

Created attachment 101066 [details] Patch for landing

Comment on attachment 101066 [details] Patch for landing Clearing flags on attachment: 101066 Committed r91136: <http://trac.webkit.org/changeset/91136>

All reviewed patches have been landed. Closing bug.

Thank you!