Bug 105534 – Let EWS not cq- patches on build/test failures for not so stable ports

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Csaba Osztrogonác]

The idea is from https://bugs.webkit.org/show_bug.cgi?id=104815

Now an EWS bot becomes red and set cq- if it can't process its task without any error. 
The type of the error ( build failure / layout test failure ) is irrelevant.

Maybe we could add "should_CQ-_if_the_build_fails" and "should_CQ-_if_test_fails" properties
(of course with better naming) to the AbstractEarlyWarningSystem class and let each ports 
determine if they are so stable to cq- patches on test failures or not. We can easily make
_post_reject_message_on_bug function cq- patches only if the build/test failed _and_ 
the property mentioned above is set.

---------------------------------------------------------------------------------------------

Otherwise to run tests on EWS bots need many hardware resources and would have bigger
latency then build only EWS bots. That's why it would be better to have very quick
build only EWS bots and many, but slow tester EWS bots.

Let's calculate a little bit: Qt EWS build only bots regularly process ~900 patches a week.
It means that they process a patch on all ~11 minutes. (A typical build is only 1-2 minutes
and a clean build is only 5 minutes thanks to the huge icecc build farm.) But run tests on
a patch takes ~30 minutes (with 1 thread, because running tests on Qt with parallel threads
are very flaky and useless on bots), so we have to have minimum 3-4 new machines to be able 
to add Qt-tester EWS bots. (Or 6-8 if we want to have WK1 and WK2 testers too) 

(I checked the EFL buildbots too, they run WK1 tests in 15 minutes with 8 
threads and WK2 tests in 20 minutes with 12 threads.)

The question is first if Qt/EFL want and have enough hardware resource to run 
tests on EWS bots. If yes, this bug report is valid and should be fixed.

[Dominik Röttsches (drott)]

I think we can have a dedicated additional EFL EWS with more cores. We can look into that next year.

[Eric Seidel (no email)]

Most EWS bots don't bother to run tests.  It sounds like Qt's shouldn't either.  Having build-only EWSes is the norm. :)

[Noam Rosenthal]

(In reply to comment #2)
> Most EWS bots don't bother to run tests.  It sounds like Qt's shouldn't either.  Having build-only EWSes is the norm. :)

I think it's broken right now that I can't tell that my change broke a bot only after landing.
I don't mind if we call it EWS, or something else, or we allow something more explicit like webkit-patch test-on-efl-bot

[Eric Seidel (no email)]

We certainly have room for other try-bot systems.  The EWS doesn't currently support such explicit requests, but something like that could be built.

I believe buildbot has support for try-bots (like you describe), and it would be possible for someone to setup a buildbot master to support such.

[Eric Seidel (no email)]

You definitely should not take any of my comments as discouraging of someone improving the systems. :)  I'm just trying to explain the status quo.

[Noam Rosenthal]

(In reply to comment #5)
> You definitely should not take any of my comments as discouraging of someone improving the systems. :)  I'm just trying to explain the status quo.

Sure thing :)
Any comments that would help with the goal of improving the infrastructure in a way that would help working with the Qt/GTK/EFL bots work smoother are highly welcome.

[Eric Seidel (no email)]

Teset failures should just slow down the EWSes, they shouldn't cause false-negative rejections.

Flaky tests can cause false-rejections.  But the EWS is designed to be able to handle up to 30 flaky tests at a time, but a single very-flaky test (30%+ flaky) can wreak havoc.

I plan to look at the flaky test situation for the EWS again in the new year.

If you're seeing false rejections, please file bugs with links to the logs, and I'm happy to look at them with you.

[Dirk Pranke]

Any reason we don't just mark flaky tests as flaky using TestExpectations?

[Eric Seidel (no email)]

(In reply to comment #8)
> Any reason we don't just mark flaky tests as flaky using TestExpectations?

We need to. :)  That would solve the current chromium and mac ews troubles.  I don't know what troubles Qt is experiancing yet.

[Csaba Osztrogonác]

We need this change to make Qt-WK2 EWS work in the future to build 
patches but not set cq- on build failure. (because Apple don't want
to block cq on WK2 build failure anymore)

[Csaba Osztrogonác]

Created attachment 181857 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #11)
> Created an attachment (id=181857) [details]
> Patch

It is a draft patch. Unfortunately I can't test it without test bugzilla and test EWS environment. But I think it should work. :)

Hmmm ... I'm thinking I should fix merging "additional_comment_text to comment_text" first in a separated patch to make this patch simpler.

[Csaba Osztrogonác]

New bug for webkitpy cleanup with the proposed fix: https://bugs.webkit.org/show_bug.cgi?id=106421

[Csaba Osztrogonác]

Created attachment 181863 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 181863 [details]
Patch

Hmmm ... webkit-patch merged the patches. :/

[Csaba Osztrogonác]

Created attachment 181868 [details]
Patch

[Adam Barth]

Comment on attachment 181868 [details]
Patch

This patch looks fine, but would you be willing to add a test?  Hopefully there's already a test that shows we reject patches from the commit-queue.  You should be able to modify it to show that this flag changes that behavior.  (I'd look in earlywarningsystem_unittest.py.)

[Adam Barth]

Comment on attachment 181868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181868&action=review

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:47
> +    _default_reject_commit_queue_on_failure = True

nit: I'd probably also use the name _reject_from_commit_queue_on_failure

[Csaba Osztrogonác]

It seems cq- -ing WebKit2 patches by Qt-WK2 EWS wasn't problem in the last
3 months. ( Because patches was reviewed and committed faster than style bot. :) ) So I think we don't need this change.