Bug 214194 – EWS should set cq- flag when a patch fails to build or introduces layout-test failures

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

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.

[Aakash Jain]

Created attachment 403989 [details]
Patch

[Aakash Jain]

Sample run: https://ews-build.webkit-uat.org/#/builders/35/builds/2573

[Aakash Jain]

Created attachment 403993 [details]
Patch

[Aakash Jain]

Updated the patch to include the case of patch introducing layout-test failure. EWS should cq- patch in that case as well.

[Radar WebKit Bug Importer]

<rdar://problem/65852947>

[Jonathan Bedard]

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?

[Aakash Jain]

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

[Aakash Jain]

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

[Aakash Jain]

Created attachment 408249 [details]
Patch

[EWS]

Committed r266741: <https://trac.webkit.org/changeset/266741>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408249 [details].

[Darin Adler]

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.

[Aakash Jain]

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

[Darin Adler]

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