Bug 58494 – commit-queue should be able to land when tree is red

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

commit-queue should be able to land when tree is red

[Eric Seidel (no email)]

Created attachment 89491 [details]
Patch

[Adam Barth]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 89495 [details]
Patch for landing

[Eric Seidel (no email)]

Created attachment 89497 [details]
Patch for landing

[Eric Seidel (no email)]

Committed r83795: <http://trac.webkit.org/changeset/83795>