Bug 214194

Summary: EWS should set cq- flag when a patch fails to build or introduces layout-test failures
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, darin, jbedard, rniwa, ryanhaddad, tsavell, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=208941
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Aakash Jain 2020-07-10 12:34:37 PDT 
EWS should set cq- flag on the patch when that patch fails to build. This would help in two ways:

#1: making sure commit-queue doesn't commit a patch if it fails to build webkit on any configuration (e.g.: ios, win, wpe, gtk, watch, tv etc.)

#2: Bugzilla would send an email notification when patch fails to build (notification about patch being marked as cq-).

Also, if a patch is already marked cq-, subsequent attempt to mark it cq- would be ignored by Bugzilla. So, if a patch fails to build on multiple configuration, bugzilla will set cq- only once and generate only one email notification.
Comment 1 Aakash Jain 2020-07-10 12:36:43 PDT 
Created attachment 403989 [details]
Patch
Comment 2 Aakash Jain 2020-07-10 12:38:55 PDT 
Sample run: https://ews-build.webkit-uat.org/#/builders/35/builds/2573
Comment 3 Aakash Jain 2020-07-10 13:08:56 PDT 
Created attachment 403993 [details]
Patch
Comment 4 Aakash Jain 2020-07-10 13:11:44 PDT 
Updated the patch to include the case of patch introducing layout-test failure. EWS should cq- patch in that case as well.
Comment 5 Radar WebKit Bug Importer 2020-07-20 16:03:20 PDT 
<rdar://problem/65852947>
Comment 6 Jonathan Bedard 2020-07-20 16:38:29 PDT 
Comment on attachment 403993 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/steps.py:1941
> +            self.build.addStepsAfterCurrentStep([SetCommitQueueMinusFlagOnPatch()])

To clarify, this only does anything if a patch is in commit queue and tests fail before it lands, right?
Comment 7 Aakash Jain 2020-08-14 12:26:46 PDT 
(In reply to Aakash Jain from comment #4)
> Updated the patch to include the case of patch introducing layout-test failure. EWS should cq- patch in that case as well.

Since we have various flaky layout tests, they can causing false positives as well (causing cq- flag on the bug unnecessarily).
Comment 8 Aakash Jain 2020-09-08 11:37:33 PDT 
(In reply to Jonathan Bedard from comment #6)
> To clarify, this only does anything if a patch is in commit queue and tests fail before it lands, right?
This marks the patch cq- irrespective of whether the patch is in commit-queue or not. If patch is in commit-queue, and patch is marked cq-, commit-queue would not land it.

Engineers can always override it by setting cq+ flag again.
Comment 9 Aakash Jain 2020-09-08 11:50:09 PDT 
Created attachment 408249 [details]
Patch
Comment 10 EWS 2020-09-08 12:29:31 PDT 
Committed r266741: <https://trac.webkit.org/changeset/266741>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408249 [details].
Comment 11 Darin Adler 2020-09-09 14:22:58 PDT 
Would be better if this was refined so that it will not add a "commit-queue-" to an obsolete patch. Doing that generates notification mail that's not valuable and has no other practical effect.

I’ve been getting that mail.
Comment 12 Aakash Jain 2020-09-09 14:39:14 PDT 
(In reply to Darin Adler from comment #11)
> Would be better if this was refined so that it will not add a "commit-queue-" to an obsolete patch.
That's a good point. I will make that change.

Also, what should be the behavior in these two scenarios (should the cq- flag be set by ews or not):
1) when the patch is already marked r-

2) when corresponding bug is already resolved (patch already landed either through commit-queue or manually).
Comment 13 Darin Adler 2020-09-09 14:52:26 PDT 
(In reply to Aakash Jain from comment #12)
> Also, what should be the behavior in these two scenarios (should the cq-
> flag be set by ews or not):
> 1) when the patch is already marked r-
> 
> 2) when corresponding bug is already resolved (patch already landed either
> through commit-queue or manually).

In both these cases I don’t see a lot of value adding commit-queue-.

I suppose that if commit-queue? or commit-queue+ was set, I might think it’s OK to *change* it to commit-queue-. But it’s not great to add a “commit-queue-“ when nothing indicates we intend to commit the patch and no one was saying anything about the commit queue at all.