[webkit-reviews] review denied: [Bug 63112] nrwt: make sharding tests needing locks less hard-coded : [Attachment 98108] fix ChangeLog

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 63112: nrwt: make sharding tests needing locks less hard-coded
https://bugs.webkit.org/show_bug.cgi?id=63112

Attachment 98108: fix ChangeLog
https://bugs.webkit.org/attachment.cgi?id=98108&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=98108&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:269
> +	   self._locked_shards = []
> +	   self._unlocked_shards = []

It's not clear from these names that we're talking about the HTTP lock.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:-580
> -	   # Put the http tests first. There are only a couple hundred of them,

> -	   # but each http test takes a very long time to run, so sorting by
the
> -	   # number of tests doesn't accurately capture how long they take to
run.

While this comment is outdated, some comment saying why we put the HTTP tests
first seems useful to me.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1362
> +	       i = 0
> +	       while i < len(test_lists):
> +		   if test_lists[i][0] == name:
> +		       return i
> +		   i += 1
> +
> +	       return -1

I think you can write this as the following:
for i in range(len(test_lists)):
    if test_lists[i][0] == name:
	return i
return -1

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1372
> +	   index = find(list_name, self._unlocked_shards)
> +	   self._unlocked_shards.pop(index)

Is there a reason to remove items from the unlocked_shards list?