Bug 60441 – Testing EWS spins on patches with a large number of failures

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Testing EWS spins on patches with a large number of failures

[Adam Barth]

Created attachment 92699 [details]
Patch

[Eric Seidel (no email)]

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.

[Adam Barth]

> > 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.

[Adam Barth]

Created attachment 92701 [details]
Patch

[Eric Seidel (no email)]

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?

[Adam Barth]

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?

[Adam Barth]

Created attachment 92719 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 92719 [details]
Patch

OK.

[Adam Barth]

Committed r86020: <http://trac.webkit.org/changeset/86020>