Bug 208920 – [ews] Resubmit patches to commit-queue which were cq- by commit-queue and later cq+

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

[Aakash Jain]

EWS django app should resubmit patches to commit-queue which were cq- by commit-queue (e.g.: due to flaky test failure) and later were cq+. Currently ews django app does not re-submit any patch, it sends the patch to buildbot only once. Because of this is a patch which was rejected by commit-queue, and later cq+ again by patch author, would not be picked up again.

[Aakash Jain]

Created attachment 393247 [details]
WIP

[Aakash Jain]

Created attachment 393778 [details]
Patch

[Jonathan Bedard]

Comment on attachment 393778 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-app/ews/fetcher.py:106
>      def patches_to_send_to_buildbot(self, patch_ids, commit_queue=False):

If we are adding a new 'patches_to_send_to_commit_queue' function, what's the point of this flag?

> Tools/BuildSlaveSupport/ews-app/ews/common/bugzilla.py:67
> +    def convert_iso_timestamp(cls, iso_timestamp):

Nit: Not convinced that this should be a separate function, I would have put the logic in the above function.

> Tools/BuildSlaveSupport/ews-app/ews/common/buildbot.py:-102
> -        return Buildbot.icons_for_queues_mapping

I think this deletion was unintentional.

> Tools/BuildSlaveSupport/ews-app/ews/common/buildbot.py:104
> +    def fetch_builders(cls):

Do we ever intend to call this outside of update_builder_name_to_id_mapping? Seems like we should consider putting the contents inside that function

[Aakash Jain]

Created attachment 393788 [details]
Patch

[Aakash Jain]

(In reply to Jonathan Bedard from comment #3)
> If we are adding a new 'patches_to_send_to_commit_queue' function, what's the point of this flag?
Removed in updated patch

> Nit: Not convinced that this should be a separate function, I would have put the logic in the above function.
Moved inside same function.

> > -        return Buildbot.icons_for_queues_mapping
> I think this deletion was unintentional.
It was actually intentional. The return value is never used and the function name also doesn't indicate that the function will return anything.

> Do we ever intend to call this outside of update_builder_name_to_id_mapping? Seems like we should consider putting the contents inside that function
Done in updated patch.

[EWS]

Committed r258611: <https://trac.webkit.org/changeset/258611>

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

[Radar WebKit Bug Importer]

<rdar://problem/60561892>