Bug 58494

Summary: commit-queue should be able to land when tree is red
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing eric: commit-queue+

Description Eric Seidel (no email) 2011-04-13 16:33:19 PDT
commit-queue should be able to land when tree is red
Comment 1 Eric Seidel (no email) 2011-04-13 16:39:28 PDT
Created attachment 89491 [details]
Patch
Comment 2 Adam Barth 2011-04-13 16:58:11 PDT
Comment on attachment 89491 [details]
Patch

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_results())

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.
Comment 3 Eric Seidel (no email) 2011-04-13 17:13:25 PDT
(In reply to comment #2)
> (From update of attachment 89491 [details])
> 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.

Done.

> > Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:217
> > +        self._expected_failures.grow_expected_failures(self._delegate.layout_test_results())
> 
> This could include some flaky failures, but maybe that's ok since we aggressively remove passing tests.

Yeah, should be fine.

> > Tools/Scripts/webkitpy/tool/bot/expectedfailures.py:32
> > +    def __init__(self):
> > +        self._failures = set()
> 
> Yeah, we should put an upper bound here.

I think I'll do that in the next patch, this one is hairy enough. :)

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

True, it is.  Going to leave it as is for now though.

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

No, supposedly it throws an IOError instead, not sure how I ended up with OSError when I wrote that code.  Leaving it as-is for now though.

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

Yup.  Bug 58370 is one way to solve that.
Comment 4 Eric Seidel (no email) 2011-04-13 17:13:46 PDT
Created attachment 89495 [details]
Patch for landing
Comment 5 Eric Seidel (no email) 2011-04-13 17:17:47 PDT
Created attachment 89497 [details]
Patch for landing
Comment 6 Eric Seidel (no email) 2011-04-13 18:08:35 PDT
Committed r83795: <http://trac.webkit.org/changeset/83795>