[webkit-reviews] review denied: [Bug 46453] [NRWT] Put the http and websocket tests to end of the test list : [Attachment 69191] proposed_patch_v2

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Gabor Rapcsanyi
<rgabor at inf.u-szeged.hu>'s request for review:
Bug 46453: [NRWT] Put the http and websocket tests to end of the test list
https://bugs.webkit.org/show_bug.cgi?id=46453

Attachment 69191: proposed_patch_v2
https://bugs.webkit.org/attachment.cgi?id=69191&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
In general, this looks fine to me.  Just some minor issues with naming and
comments.  Also, is it possible to write a unit test for this method?  It could
verify that http tests are actually at the beginning of the list (good catch!).
 r- for no tests, but you might be able to convince me that it's really hard to
test.

View in context: https://bugs.webkit.org/attachment.cgi?id=69191&action=review

> WebKitTools/ChangeLog:6
> +	   [NRWT] Put the http and websocket tests to end of the test list.
> +	   https://bugs.webkit.org/show_bug.cgi?id=46453

I think this description is no longer correct.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:503
> +    def _test_requires_lock(self, test_dir):
> +	   """Return True if a test needs locking."""

Can you expand this docstring a bit?  It's not clear what locking you're
talking about.	Something like, "Return True if the test needs to be locked
when running multiple copies of NRWTs."

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:507
> +	   if 'http' in split_path or 'websocket' in split_path:
> +	       return True
> +	   return False

Maybe "return 'http' in split_path or 'websocket' in split_path" is better?

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:522
> +	   tests_to_lock = []

Nit: Maybe name this tests_to_http_lock so it's clear what lock we're talking
about.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:551
> +	       # of how long that set of tests will take to run. We can't just
use
> +	       # a PriorityQueue until we move # to Python 2.6.

Nit: Can you remove the extra # in the middle of the line?  Thanks.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:555
> +		   # TODO: Remove once tests are fixed so they can be run in
any

Nit: Can you convert this to a FIXME instead of TODO?