Bug 159903 – EWS error message "Error: * did not process patch" should include explanation

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

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>

[Aakash Jain]

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.

[Aakash Jain]

Created attachment 283953 [details]
Logs from staging server

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Perhaps something like "Bug was already closed when EWS attempted to process it."?

[Aakash Jain]

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

[Alexey Proskuryakov]

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.

[Aakash Jain]

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()

[WebKit Commit Bot]

Comment on attachment 284803 [details]
Updated patch

Clearing flags on attachment: 284803

Committed r203830: <http://trac.webkit.org/changeset/203830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.