[webkit-reviews] review denied: [Bug 60441] Testing EWS spins on patches with a large number of failures : [Attachment 92699] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 60441: Testing EWS spins on patches with a large number of failures
https://bugs.webkit.org/show_bug.cgi?id=60441

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92699&action=review

I think this is a great change, but I would like to see anothe rround.

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:33
> +	   self._failures_bounded = True

_failures_are_bounded would be better.	We need to clarify that this means that
we can't use the _failures() set.  That we're treating the set of failures as
infinite.  That was not clear in my first reading of this patch.

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:36
> +    def _has_results(self, results):
> +	   return bool(results and len(results.failing_tests()) != 0)

I think you should call this _has_failures instead of _has_results

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:39
> +	   return bool(results.failure_limit_count() and
len(results.failing_tests()) < results.failure_limit_count())

maybe assert(results)?

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:41
> +    def _has_trustworthy_results(self, results):

I would have left this as _can_trust_results (since I don't think the other two
methods are really _results methods, but rather _failures methods.)

> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:45
> +	   if not self._has_trustworthy_results(results):

You should note here why failures_were_expected does not need to check
_failures_are_bounded.	I believe it's because we'll "incorrectly" return false
negatives in the unbounded failures case, but our callers won't care.