Testing EWS spins on patches with a large number of failures
Created attachment 92699 [details] Patch
Comment on attachment 92699 [details] Patch 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.
> > 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. I added the check. I don't think it matters much either way.
Created attachment 92701 [details] Patch
Comment on attachment 92701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92701&action=review I think we need more testing here. Again, the concept looks good. It's a bit odd that we're storing the failure_limit outside this class, but kinda necessary. > Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:40 > + def _has_bounded_results(self, results): I would name this has_bounded_failures since it deals with failures. But it's true, that failures is the way we limit results these days. > Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:48 > + if not self._can_trust_results(results) or not self._failures_are_bounded: Is this intentional? Seems we want to test this. > Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:66 > + self._failures_are_bounded = True _failures_are_bounded = False if we can't trust resutls here?
Comment on attachment 92701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92701&action=review >> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:40 >> + def _has_bounded_results(self, results): > > I would name this has_bounded_failures since it deals with failures. But it's true, that failures is the way we limit results these days. Done. >> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:48 >> - if not self._can_trust_results(results): >> + if not self._can_trust_results(results) or not self._failures_are_bounded: > > Is this intentional? Seems we want to test this. You asked me to add this in the previous iteration. I can remove it again if you want. >> Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:66 >> + self._failures_are_bounded = True > > _failures_are_bounded = False if we can't trust resutls here? That wouldn't shrink the expected failures, right?
Created attachment 92719 [details] Patch
Comment on attachment 92719 [details] Patch OK.
Committed r86020: <http://trac.webkit.org/changeset/86020>