[webkit-reviews] review denied: [Bug 64564] [NRWT] Add support for --no-http : [Attachment 101021] Proposed fix 2: Also don't run websocket tests and take Adam's comments into account.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 15 14:25:45 PDT 2011


Dirk Pranke <dpranke at chromium.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 64564: [NRWT] Add support for --no-http
https://bugs.webkit.org/show_bug.cgi?id=64564

Attachment 101021: Proposed fix 2: Also don't run websocket tests and take
Adam's comments into account.
https://bugs.webkit.org/attachment.cgi?id=101021&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
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.


More information about the webkit-reviews mailing list