[ews-build] Add build step to validate the patch before processing it

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

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.

[Aakash Jain]

Created attachment 358317 [details]
Proposed patch

Sample runs:
Bug 193058 is already closed: http://ews-build.webkit-uat.org/#/builders/21/builds/908
Patch 358136 is obsolete:  http://ews-build.webkit-uat.org/#/builders/21/builds/906
Patch 357767 is marked r-: http://ews-build.webkit-uat.org/#/builders/21/builds/695 

Builder page: http://ews-build.webkit-uat.org/#/builders/21

[Lucas Forschler]

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 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 on attachment 358317 [details]
Proposed patch

Attachment 358317 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10628435

New failing tests:
http/wpt/css/css-animations/start-animation-001.html

[EWS Watchlist]

Created attachment 358324 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Aakash Jain]

Created attachment 358345 [details]
Updated patch

> it's a double negative.
Fixed.

[Lucas Forschler]

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]

Created attachment 358405 [details]
Patch for landing

[WebKit Commit Bot]

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: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 358405 [details]
Patch for landing

Clearing flags on attachment: 358405

Committed r239650: <https://trac.webkit.org/changeset/239650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47063037>

[Aakash Jain]

Committed r239657: <https://trac.webkit.org/changeset/239657>