[webkit-reviews] review denied: [Bug 63116] nrwt: allow for multiple http shards : [Attachment 98302] clean up logic, add tests

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 63116: nrwt: allow for multiple http shards
https://bugs.webkit.org/show_bug.cgi?id=63116

Attachment 98302: clean up logic, add tests
https://bugs.webkit.org/attachment.cgi?id=98302&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98302&action=review

> Tools/ChangeLog:20
> +	   nrwt: make sharding tests needing locks less hard-coded
> +	   https://bugs.webkit.org/show_bug.cgi?id=63112

Is this ChangeLog entry supposed to be here?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:543
>	   Return:
>	       Two lists of lists of TestInput objects. The first list should
>	       only be run under the server lock, the second can be run
whenever.

This patch might be the right time to make this return type into a small class.


> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:629
> +    def _resize_shards(self, shards, max_shards):

What is |shards|?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:633
> +	   i = 1
> +	   j = 1

Please use more descriptive variable names.  It looks like i is current
directory count and j is number of shards.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:637
> +	       tests.extend(shard[1])

It's not clear to me what shard[1].  I assume it's part of a tuple (so it's
always valid), but I can't tell.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:640
> +		   new_shards.append(('locked_shard_%d' % j, tests))

Can we compute j based on the size of new_shards?