Bug 33496 – webkit-commit-queue status page is confusing

• Component: Tools / Tests
• Status: REOPENED

[Eric Seidel (no email)]

webkit-commit-queue status page is confusing

<WildFox> eseidel: wondering about this commit-queue status page, when it says "patch 46256... from bug...: Fail" - what does that mean

Most of those "Fail" messages are temporary.  The page does not express that very well.

[Eric Seidel (no email)]

This is actually partially teh commit-queue bots fault.  It shouldn't post "FAIL" in those cases, it shoudl post "Error" when the failure is temporary.

[Eric Seidel (no email)]

I've had two more complaints since about FAIL being confusing.  Need to improve status reporting here. :(

[Eric Seidel (no email)]

Created attachment 46964 [details]
Patch

[Eric Seidel (no email)]

*** Bug 33871 has been marked as a duplicate of this bug. ***

[Adam Barth]

Comment on attachment 46964 [details]
Patch

         if is_svn_apply:
-            QueueEngine.exit_after_handled_error(script_error)
+            return # No need to update the bug for svn-apply failures.
[...]
-        exit(1)

How is the master process supposed to distinguish these cases if we don't return a different exit code?

[Eric Seidel (no email)]

Why does the master process need to care?  We've updated the server, the master process will notice next time it queries the server.

Exiting 1 will cause the master process to also update the server, because it thinks things failed.  The master process doesnt' need to do anything in that case.

This is the only place that handle_script_error is called:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/stepsequence.py#L76

[Adam Barth]

(In reply to comment #6)
> Why does the master process need to care?

The master process decides whether to log a "FAIL" message to the server depending on the return code used by the child process.  If we land this change, the purple bubbles will become red because the master will not know that the child was able to handle svn-apply failures.

[Eric Seidel (no email)]

exit code 1 should cause FAIL.  2 should cause nothing because the child handled it.  You're seeing something I"m not, or you're reading the code backwards. :)

[Eric Seidel (no email)]

Created attachment 46995 [details]
Patch

[Eric Seidel (no email)]

Created attachment 46997 [details]
Patch

[Adam Barth]

Comment on attachment 46997 [details]
Patch

This is much better

[Eric Seidel (no email)]

Committed r53537: <http://trac.webkit.org/changeset/53537>

[Eric Seidel (no email)]

Reverted r53537 for reason:

Added a failure condition to the commit-queue and looks to have broken the EWS bots

Committed r53570: <http://trac.webkit.org/changeset/53570>

[WebKit Commit Bot]

Comment on attachment 46997 [details]
Patch

Clearing flags on attachment: 46997

Committed r53593: <http://trac.webkit.org/changeset/53593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Bah!  This is why we clear flags.  The commit-queeu just re-landed what I had rolled out.  I guess I'll just fix it this time...

[Eric Seidel (no email)]

About to roll this out again.  Filed a bug about webkit-patch rollout needing to clear flag when re-opening bugs.

[Eric Seidel (no email)]

Reverted r53593 for reason:

Re-rollout this patch, the commit-queue should not have landed it again, but it did due to land-diff and rollout both not clearing flags.

Committed r53609: <http://trac.webkit.org/changeset/53609>