[webkit-reviews] review granted: [Bug 58494] commit-queue should be able to land when tree is red : [Attachment 89491] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 58494: commit-queue should be able to land when tree is red
https://bugs.webkit.org/show_bug.cgi?id=58494

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89491&action=review

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:205
> +	   first_failing_tests = [] if not first_results else
first_results.failing_tests()
> +	   second_failing_tests = [] if not second_results else
second_results.failing_tests()
> +
>	   if first_failing_tests != second_failing_tests:

This should be some kind of comparison helper.

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:217
> +	  
self._expected_failures.grow_expected_failures(self._delegate.layout_test_resul
ts())

This could include some flaky failures, but maybe that's ok since we
aggressively remove passing tests.

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:-240
> -    _double_flaky_test_counter = 0

Who added this variable!

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:32
> +    def __init__(self):
> +	   self._failures = set()

Yeah, we should put an upper bound here.

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:37
> +	   return len(results.failing_tests()) != 0 and
len(results.failing_tests()) != results.failure_limit_count()

This part "len(results.failing_tests()) != 0" is really an assert.

> Tools/Scripts/webkitpy/tool/commands/queues.py:319
>	   except OSError, e:  # File does not exist or can't be read.

Can this exception ever occur?

> Tools/Scripts/webkitpy/tool/steps/runtests.py:35
> +    # FIXME: This knowledge really belongs in the commit-queue.
> +    NON_INTERACTIVE_FAILURE_LIMIT_COUNT = 1

This thing is not great, but you know that.