Bug 196599 – EWS should have a way to retry a patch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

For example, if a patch depended on another patch, and EWS ran it too early, it shouldn't be necessary to re-upload it.

Also, sometimes EWS is just wrong because of flaky trunk.

[Aakash Jain]

<rdar://problem/22918898>

[Radar WebKit Bug Importer]

<rdar://problem/49608501>

[Aakash Jain]

Created attachment 381340 [details]
Patch

[Aakash Jain]

Tested in: https://ews.webkit-uat.org/status-bubble/380828/

The button UI would need slight polishing.

[Jonathan Bedard]

Comment on attachment 381340 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:70
>          _log.info('Saved build {} in database for patch_id: {}'.format(uid, patch_id))

Do we have to use *args here? Until this false, everything was using a variable with a descriptive name. I think we either want a named variable or start using kwargs in this call.

> Tools/BuildSlaveSupport/ews-app/ews/templates/statusbubble.html:108
> +{% endif %}

Were we intending the EWS button style to be improved in this patch or a future one?

For what it's worth, the button seems pretty consistent with the existing style.

> Tools/BuildSlaveSupport/ews-app/ews/views/retrypatch.py:72
> +        return redirect('/status-bubble/{}'.format(patch_id))

Why weren't we redirecting before?

[Aakash Jain]

Comment on attachment 381340 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:70
>>          _log.info('Saved build {} in database for patch_id: {}'.format(uid, patch_id))
> 
> Do we have to use *args here? Until this false, everything was using a variable with a descriptive name. I think we either want a named variable or start using kwargs in this call.

Moved 'retried' variable declaration below complete_at in updated patch, so that we don't have to pass this optional variable while instantiating this class.

>> Tools/BuildSlaveSupport/ews-app/ews/templates/statusbubble.html:108
>> +{% endif %}
> 
> Were we intending the EWS button style to be improved in this patch or a future one?
> 
> For what it's worth, the button seems pretty consistent with the existing style.

Improved one UI aspect in updated patch, the button was rendering in new line sometimes.

>> Tools/BuildSlaveSupport/ews-app/ews/views/retrypatch.py:72
>> +        return redirect('/status-bubble/{}'.format(patch_id))
> 
> Why weren't we redirecting before?

It was work-in-progress earlier. It's a better UX to simply refresh the status-bubble on clicking the retry button. Builds which would be retried would turn blue (in-progress), (handled by the changes in statusbubble.py code).

[Aakash Jain]

Created attachment 381416 [details]
Patch

[Jonathan Bedard]

Comment on attachment 381416 [details]
Patch

I like this, do we have an example or a screenshot for the UI change?

[Aakash Jain]

(In reply to Jonathan Bedard from comment #8)
> Comment on attachment 381416 [details]
> Patch
> 
> I like this, do we have an example or a screenshot for the UI change?
https://ews.webkit-uat.org/status-bubble/380828/ the 'Retry failed builds' button was being rendered in new-line earlier., Now it's rendered in same line along-with the status-bubbles.

[Aakash Jain]

Committed r251368: <https://trac.webkit.org/changeset/251368>