Bug 137290 – EWS is too eager to say that a patch does not apply to trunk of repository

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

[Alexey Proskuryakov]

The patch in bug 137272 was processed by some queues, yet EWS claims that it was not.

[Alexey Proskuryakov]

Created attachment 239012 [details]
proposed fix

[Ryosuke Niwa]

Comment on attachment 239012 [details]
proposed fix

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

> Tools/QueueStatusServer/handlers/statusbubble.py:165
> +        bubble["had_resultative_status_other_than_failure_to_apply"] = any(map(lambda status: status.message not in progress_statuses and status.message != "Patch does not apply" and status.message != "Error: " + queue.name() + " unable to apply patch.", statuses))

Can we split this into two lines?  It's awfully long.
Alternatively, I would the variable name to did_not_fail_to_apply.

> Tools/QueueStatusServer/handlers/statusbubble.py:191
> +        had_resultative_status_other_than_failure_to_apply = any(map(lambda bubble: bubble["had_resultative_status_other_than_failure_to_apply"] == True, bubbles))

No need to compare with True (i.e. remove == True).

[Alexey Proskuryakov]

Created attachment 239029 [details]
better fix

I realized that the truth lies in between - with the previous patch, we could get all red and all popups would just say "failed to apply", which is useless.

[Ryosuke Niwa]

Comment on attachment 239029 [details]
better fix

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

> Tools/QueueStatusServer/handlers/statusbubble.py:129
> +            bubble["had_resultative_status_other_than_failure_to_apply"] = any(map(lambda status: latest_resultative_status and latest_resultative_status.message != "Error: " + queue.name() + " unable to apply patch.", statuses))

Again, can we split this into two lines?  It's really long.

[Daniel Bates]

Comment on attachment 239029 [details]
better fix

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

> Tools/QueueStatusServer/app.yaml:2
> +version: ap # Bugzilla bug ID of last major change

I take it you are using ap as placeholder. Regardless, the inline comment doesn't make sense at least since <http://trac.webkit.org/changeset/162288>. We seem to be using the SVN revision number instead of the Bugzilla bug id for the version since <http://trac.webkit.org/changeset/161062>. This is error prone and has led to a mismatch between the actual SVN revision and the revision recorded in this file at least once so far in <http://trac.webkit.org/changeset/162288>. I assume that the actual value of version matters less than changing it to be something different from the last recorded version, possibly with the invariant that its value is monotonically increasing. Regardless, we should standardize a convention for updating it and update the comment in this file to document the convention.

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 239029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239029&action=review
> 
> > Tools/QueueStatusServer/app.yaml:2
> > +version: ap # Bugzilla bug ID of last major change
> 
>... Regardless, the inline comment doesn't make sense at least since <http://trac.webkit.org/changeset/162288>....

*the inline comment doesn't make sense.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r174158>.

[Alexey Proskuryakov]

> the inline comment doesn't make sense

I guess I never read the comment! I think that we always used svn revision here, Ryosuke guided me to do so when I first made a change to the webkit-queues app.

Yes, I think that an svn revision is more useful than a bug ID, as we can bisect when something breaks (AppEngine keeps old versions available until one manually deletes them).