Bug 107092 – Sheriffbot should explain rollout failures in a human readable form.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[WebKit Review Bot]

Sheriffbot should explain rollout failures in a human readable form.
Requested by alancutter on #webkit.

[Alan Cutter]

IRC Log:

> benjaminp  | sheriffbot: rollout 139947 139935 It causes 600 crashes on the WebKit2 bots
> sheriffbot | benjaminp: Preparing rollout for http://trac.webkit.org/changeset/139947 and http://trac.webkit.org/changeset/139935 ...
> sheriffbot | benjaminp, bradee-oh, mitzpettel, weinig: Failed to create rollout patch:
> sheriffbot | Failed to run "['/mnt/git/webkit-sheriff-bot/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--..." exit_code: 2 cwd: /mnt/git/webkit-sheriff-bot


Sheriffbot Log:

> Running: webkit-patch --status-host=queues.webkit.org --bot-id=gce-feeder-01 create-rollout --force-clean --parent-command=sheriff-bot 139947 139935 It causes 600 crashes on the WebKit2 bots (Requested by benjaminp on #webkit).
> Failed to run "['/mnt/git/webkit-sheriff-bot/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--..." exit_code: 2 cwd: /mnt/git/webkit-sheriff-bot
> 
> Preparing rollout for bug 106826.
> Updating working directory
> Failed to apply reverse diff for revision 139935 because of the following conflicts:
> Source/WebCore/WebCore.vcproj/WebCore.vcproj
> Failed to apply reverse diff for revision 139935 because of the following conflicts:
> Source/WebCore/WebCore.vcproj/WebCore.vcproj
> Updating OpenSource
> Current branch master is up to date.
> 
> No work item. Sleeping until 2013-01-17 04:48:54 (2 mins).
> No work item. Sleeping until 2013-01-17 04:49:24 (2 mins).

[Eric Seidel (no email)]

Agreed!

[Alan Cutter]

Created attachment 194224 [details]
Patch

[Alan Cutter]

(In reply to comment #3)
> Created an attachment (id=194224) [details]
> Patch

Sheriffbot will now respond like so:

<alancutter> sheriffbot: rollout 112776 Rollout failure testing.
<sheriffbot> alancutter: Preparing rollout for http://trac.webkit.org/changeset/112776 ...
<sheriffbot> alancutter, dpranke, rniwa: Failed to create rollout patch:
<sheriffbot> alancutter, dpranke, rniwa: Failed to apply reverse diff for file(s): Source/WebCore/rendering/RenderText.cpp, Source/WebCore/rendering/RenderText.h

[Alan Cutter]

Created attachment 194231 [details]
Patch

[Alan Cutter]

(In reply to comment #5)
> Created an attachment (id=194231) [details]
> Patch

 - Fixed issue where the new error would not be displayed if sheriffbot's working directory was not the repository.

[Eric Seidel (no email)]

Comment on attachment 194231 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194231&action=review

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:249
> +            if os.path.exists(tool.scm().absolute_path(line)):

Filesystem.exists

[Alan Cutter]

Created attachment 194365 [details]
Patch

[Alan Cutter]

(In reply to comment #7)
> (From update of attachment 194231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194231&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:249
> > +            if os.path.exists(tool.scm().absolute_path(line)):
> 
> Filesystem.exists

For me this was one of those things you realise in the middle of the night that you should have done differently. Fixed in new patch.

[Ryosuke Niwa]

Comment on attachment 194365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194365&action=review

Can't we add any test for this?

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:240
> +        revert_failure_message = "Failed to apply reverse diff for revision"
> +        index = error_log.find(revert_failure_message)

I would not have defined revert_failure_message since the semantics of the message is pretty self-evident from the code.
On the other hand, I don't like the variable name "index" as it's such a generic name.
What kind of index is it?

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:251
> +        for line in lines:
> +            if tool.filesystem.exists(tool.scm().absolute_path(line)):
> +                files.append(line)
> +            else:
> +                break

Why don't we just use filter?

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:254
> +        return

I'd prefer explicitly returning None or "" here.

[Alan Cutter]

(In reply to comment #10)
> (From update of attachment 194365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194365&action=review
> 
> Can't we add any test for this?
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:240
> > +        revert_failure_message = "Failed to apply reverse diff for revision"
> > +        index = error_log.find(revert_failure_message)
> 
> I would not have defined revert_failure_message since the semantics of the message is pretty self-evident from the code.
> On the other hand, I don't like the variable name "index" as it's such a generic name.
> What kind of index is it?
> 

I'll take the variable revert_failure_message out, it's only used once anyway.
Will rename index to revert_failure_message_start as suggested in channel.

> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:251
> > +        for line in lines:
> > +            if tool.filesystem.exists(tool.scm().absolute_path(line)):
> > +                files.append(line)
> > +            else:
> > +                break
> 
> Why don't we just use filter?

Filter will grab all paths in the list, I need this to stop once it hits. I could use itertools.takewhile instead to reduce the lines of code.

> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:254
> > +        return
> 
> I'd prefer explicitly returning None or "" here.

Will return None instead.

[Alan Cutter]

Created attachment 195017 [details]
Patch

[Alan Cutter]

(In reply to comment #12)
> Created an attachment (id=195017) [details]
> Patch

Made changes based on the review.

Tested this change on Linux and Mac.
Sheriffbot's rollout command doesn't seem to work on Windows and I don't anticipate sheriffbot to be hosted on Windows.

[Ryosuke Niwa]

Comment on attachment 195017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195017&action=review

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:243
> +        # Pull out file paths after the error message.

I don't think this comment is helpful. Please remove.

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:248
> +        if files:
> +            return "Failed to apply reverse diff for file(s): %s" % ", ".join(files)
> +        return None

I would have used tertiary here as in: return "~" if files else None but it's not a big deal.

[Alan Cutter]

Created attachment 195021 [details]
Patch

[Alan Cutter]

(In reply to comment #15)
> Created an attachment (id=195021) [details]
> Patch

(In reply to comment #14)
> (From update of attachment 195017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195017&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:243
> > +        # Pull out file paths after the error message.
> 
> I don't think this comment is helpful. Please remove.
> 

Removed comment.


Added a couple of unit tests for the function.

[WebKit Review Bot]

Comment on attachment 195021 [details]
Patch

Clearing flags on attachment: 195021

Committed r146861: <http://trac.webkit.org/changeset/146861>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.