Bug 60441 - Testing EWS spins on patches with a large number of failures
Summary: Testing EWS spins on patches with a large number of failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-07 15:58 PDT by Adam Barth
Modified: 2011-05-07 19:50 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.57 KB, patch)
2011-05-07 16:04 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.05 KB, patch)
2011-05-07 17:14 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (8.97 KB, patch)
2011-05-07 19:46 PDT, Adam Barth
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-05-07 15:58:57 PDT
Testing EWS spins on patches with a large number of failures
Comment 1 Adam Barth 2011-05-07 16:04:20 PDT
Created attachment 92699 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-05-07 16:41:09 PDT
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.
Comment 3 Adam Barth 2011-05-07 17:12:53 PDT
> > 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.
Comment 4 Adam Barth 2011-05-07 17:14:08 PDT
Created attachment 92701 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-05-07 19:43:01 PDT
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 6 Adam Barth 2011-05-07 19:45:28 PDT
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?
Comment 7 Adam Barth 2011-05-07 19:46:03 PDT
Created attachment 92719 [details]
Patch
Comment 8 Eric Seidel (no email) 2011-05-07 19:49:07 PDT
Comment on attachment 92719 [details]
Patch

OK.
Comment 9 Adam Barth 2011-05-07 19:50:14 PDT
Committed r86020: <http://trac.webkit.org/changeset/86020>