The ews error message is "Error: * did not process patch." is confusing/misleading. It gives no information about why the queue did not process patch, no information about location of the logs. What actually happens, is that the patch fails validation. We should improve the error message to make it more readable/meaningful.. something like “Patch fails validation”. Also, if possible we should add more information to this error message indicating why the patch failed validation. See <rdar://problem/27410788>
Created attachment 283950 [details] Proposed patch Tested on a staging server. Also test-webkitpy passes. Logs from staging server: Patch 283778 from bug 159824: 0 minutes ago (testbot-aakash) Error: Patch fails validation. Bug is already closed.
Created attachment 283953 [details] Logs from staging server
Comment on attachment 283950 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=283950&action=review I think that this will break reporting, because it matches log lines by name: $ grep -r "did not process patch" Tools/ Tools//QueueStatusServer/handlers/processingtimesjson.py: elif status_message == "Error: " + queue_name + " did not process patch.": Tools//QueueStatusServer/handlers/statusbubble.py: elif statuses[0].message == "Error: " + queue.name() + " did not process patch.": ... > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:91 > + self._did_error(patch, "Patch fails validation. %s" % error.failure_message) I think that "fails validation" text may also be confusing to people looking at the bubbles in Bugzilla.
Perhaps something like "Bug was already closed when EWS attempted to process it."?
Created attachment 284750 [details] Updated patch Previous patch did not modify the messages shown in bubbles in bugzilla. This patch will add some more information in the message shown in bubbles. The exact messages are in QueueStatusServer/handlers/statusbubble.py
Comment on attachment 284750 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=284750&action=review > Tools/QueueStatusServer/handlers/processingtimesjson.py:43 > - elif status_message == "Error: " + queue_name + " did not process patch.": > + elif "Error: Patch fails validation" in status_message: We should preserve support for the old style too, as there are patches with old style statuses in the database. > Tools/QueueStatusServer/handlers/statusbubble.py:143 > - elif statuses[0].message == "Error: " + queue.name() + " did not process patch.": > + elif "Error: Patch fails validation" in statuses[0].message: Ditto. > Tools/QueueStatusServer/handlers/statusbubble.py:154 > + bubble["details_message"] += " EWS could not determine the committer information." This one seems unclear. What does this mean? Is this only for commit queue when the person who marked the patch cq+ is not a committer? > Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:91 > + self._did_error(patch, "Patch fails validation. %s" % error.failure_message) Why "fails" and not "failed"? Looks like other statuses use past tense.
Created attachment 284803 [details] Updated patch Reverted the error message to "did not process patch." as it is easier to maintain backward compatibility and it's anyways not shown on the bubbles. The additional explanation about why the queue did not process patch, should make it easier to debug/understand. Made the missing committer error message similar to the perfalizer.py:run()
Comment on attachment 284803 [details] Updated patch Clearing flags on attachment: 284803 Committed r203830: <http://trac.webkit.org/changeset/203830>
All reviewed patches have been landed. Closing bug.