Bug 193140

Summary: [ews-build] Add build step to validate the patch before processing it
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, ews-watchlist, lforschler, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews112 for mac-sierra
none
Updated patch
lforschler: review+
Patch for landing none

Aakash Jain
Reported 2019-01-04 07:25:37 PST
EWS should perform some basic validation of the patch before processing it. For e.g.: if the bug is already closed or the patch is obsolete, processing the patch would be just waste of resources. We should add a validate-patch build step.
Attachments
Proposed patch (7.51 KB, patch)
2019-01-04 07:54 PST, Aakash Jain 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews112 for mac-sierra (2.00 MB, application/zip)
2019-01-04 09:46 PST, EWS Watchlist 		no flags
Details
Updated patch (7.51 KB, patch)
2019-01-04 12:10 PST, Aakash Jain 		lforschler: review+
DetailsFormatted DiffDiff
Patch for landing (7.43 KB, patch)
2019-01-04 17:41 PST, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Lucas Forschler
Comment 2 2019-01-04 09:25:29 PST
Comment on attachment 358317 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=358317&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:247 > + return bugs_json[0] it it possible we'd ever need to return something other than the '0' index? > Tools/BuildSlaveSupport/ews-build/steps.py:262 > + if not patch_json.get('id') != self.getProperty('patch_id', ''): this feels wrong... if not != ? it's a double negative. I would think you'd want if patch_json.get('id') != self.getProperty('patch_id...) > Tools/BuildSlaveSupport/ews-build/steps.py:305 > + if bug_closed == 1: can we simply say if bug_closed: ? > Tools/BuildSlaveSupport/ews-build/steps.py:310 > + if obsolete == 1: ditto here > Tools/BuildSlaveSupport/ews-build/steps.py:315 > + if review_denied == 1: ditto
Aakash Jain
Comment 3 2019-01-04 09:43:19 PST
Comment on attachment 358317 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=358317&action=review >> Tools/BuildSlaveSupport/ews-build/steps.py:247 >> + return bugs_json[0] > > it it possible we'd ever need to return something other than the '0' index? I don't think so. It just how the bugzilla REST is. Even on fetching a single bug, it returns a list containing single entry. e.g.: https://bugs.webkit.org/rest/bug/193135 >> Tools/BuildSlaveSupport/ews-build/steps.py:262 >> + if not patch_json.get('id') != self.getProperty('patch_id', ''): > > this feels wrong... if not != ? > it's a double negative. > > > I would think you'd want > if patch_json.get('id') != self.getProperty('patch_id...) good catch >> Tools/BuildSlaveSupport/ews-build/steps.py:305 >> + if bug_closed == 1: > > can we simply say if bug_closed: ? This is intentional, since bug_closed can also be -1, in which case we don't want to enter this if condition. -1 would indicate some infrastructure error (e.g.: unable to fetch from bugzilla) which prevented validation. >> Tools/BuildSlaveSupport/ews-build/steps.py:310 >> + if obsolete == 1: > > ditto here This is intentional, since obsolete can also be -1, in which case we don't want to enter this if condition.
EWS Watchlist
Comment 4 2019-01-04 09:46:35 PST Comment hidden (obsolete)
EWS Watchlist
Comment 5 2019-01-04 09:46:37 PST Comment hidden (obsolete)
Aakash Jain
Comment 6 2019-01-04 12:10:56 PST
Created attachment 358345 [details] Updated patch > it's a double negative. Fixed.
Lucas Forschler
Comment 7 2019-01-04 12:41:17 PST
Comment on attachment 358345 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=358345&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:263 > + self._addToLog('stdio', 'Fetched Patch id {} does not match with requested patch id {}. Unable to validate.\n'.format(patch_json.get('id'), self.getProperty('patch_id', ''))) nit: lowercase 'patch'
Aakash Jain
Comment 8 2019-01-04 17:41:04 PST
Created attachment 358405 [details] Patch for landing
WebKit Commit Bot
Comment 9 2019-01-04 18:29:28 PST
The commit-queue encountered the following flaky tests while processing attachment 358405 [details]: http/wpt/css/css-animations/start-animation-001.html bug 190903 (authors: dino@apple.com, fred.wang@free.fr, and graouts@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 10 2019-01-04 18:30:13 PST
Comment on attachment 358405 [details] Patch for landing Clearing flags on attachment: 358405 Committed r239650: <https://trac.webkit.org/changeset/239650>
WebKit Commit Bot
Comment 11 2019-01-04 18:30:15 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12 2019-01-04 18:35:22 PST
<rdar://problem/47063037>
Aakash Jain
Comment 13 2019-01-05 04:02:32 PST
Committed r239657: <https://trac.webkit.org/changeset/239657>
Note You need to log in before you can comment on or make changes to this bug.