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 / Tests | Assignee: | Alexey Proskuryakov <ap> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dbates, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2014-10-01 00:05:57 PDT
Created attachment 239012 [details]
proposed fix
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). 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 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 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. (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. Committed <http://trac.webkit.org/r174158>. > 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).
|