Bug 137290

Summary: EWS is too eager to say that a patch does not apply to trunk of repository
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Tools / TestsAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed fix
rniwa: review+
better fix rniwa: review+

Description Alexey Proskuryakov 2014-10-01 00:05:57 PDT
The patch in bug 137272 was processed by some queues, yet EWS claims that it was not.
Comment 1 Alexey Proskuryakov 2014-10-01 00:08:28 PDT
Created attachment 239012 [details]
proposed fix
Comment 2 Ryosuke Niwa 2014-10-01 06:40:52 PDT
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).
Comment 3 Alexey Proskuryakov 2014-10-01 09:07:10 PDT
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.
Comment 4 Ryosuke Niwa 2014-10-01 09:58:08 PDT
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.
Comment 5 Daniel Bates 2014-10-01 10:08:21 PDT
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.
Comment 6 Daniel Bates 2014-10-01 10:09:47 PDT
(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.
Comment 7 Alexey Proskuryakov 2014-10-01 10:18:29 PDT
Committed <http://trac.webkit.org/r174158>.
Comment 8 Alexey Proskuryakov 2014-10-01 10:24:16 PDT
> 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).