Diff
Modified: trunk/Tools/ChangeLog (89479 => 89480)
--- trunk/Tools/ChangeLog 2011-06-22 21:23:16 UTC (rev 89479)
+++ trunk/Tools/ChangeLog 2011-06-22 21:33:35 UTC (rev 89480)
@@ -1,3 +1,25 @@
+2011-06-22 Eric Seidel <e...@webkit.org>
+
+ Reviewed by Ojan Vafai.
+
+ Make sheriff-bot rollout messages a little nicer
+ https://bugs.webkit.org/show_bug.cgi?id=63107
+
+ It annoyed me this afternoon that I had to convert sheriff-bots "r12345" revisions
+ into urls myself. So I have now fixed its "preparing" message to include a url.
+
+ I also figured that I should make the messages mention all of the responsible parties
+ so that rollouts are never surprises. If you're in the channel and were involved
+ in a patch, you will see if someone is using sheriff-bot to rollout a patch.
+
+ As part of doing this I also changed (and tested) _parse_args to fail-fast
+ when given invalid args.
+
+ * Scripts/webkitpy/tool/bot/irc_command.py:
+ * Scripts/webkitpy/tool/bot/irc_command_unittest.py:
+ * Scripts/webkitpy/tool/bot/sheriff.py:
+ * Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py:
+
2011-06-22 Dirk Pranke <dpra...@chromium.org>
Reviewed by Tony Chang.
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/irc_command.py (89479 => 89480)
--- trunk/Tools/Scripts/webkitpy/tool/bot/irc_command.py 2011-06-22 21:23:16 UTC (rev 89479)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/irc_command.py 2011-06-22 21:33:35 UTC (rev 89480)
@@ -26,6 +26,7 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+import itertools
import random
from webkitpy.common.config import irc as config_irc
@@ -56,46 +57,65 @@
class Rollout(IRCCommand):
def _parse_args(self, args):
- read_revision = True
- rollout_reason = []
+ if not args:
+ return (None, None)
+
# the first argument must be a revision number
- svn_revision_list = [args[0].lstrip("r")]
- if not svn_revision_list[0].isdigit():
- read_revision = False
+ first_revision = args[0].lstrip("r")
+ if not first_revision.isdigit():
+ return (None, None)
+ parsing_revision = True
+ svn_revision_list = [int(first_revision)]
+ rollout_reason = []
for arg in args[1:]:
- if arg.lstrip("r").isdigit() and read_revision:
- svn_revision_list.append(arg.lstrip("r"))
+ if arg.lstrip("r").isdigit() and parsing_revision:
+ svn_revision_list.append(int(arg.lstrip("r")))
else:
- read_revision = False
+ parsing_revision = False
rollout_reason.append(arg)
+ rollout_reason = " ".join(rollout_reason)
return svn_revision_list, rollout_reason
+ def _responsible_nicknames_from_revisions(self, tool, sheriff, svn_revision_list):
+ commit_infos = map(tool.checkout().commit_info_for_revision, svn_revision_list)
+ nickname_lists = map(sheriff.responsible_nicknames_from_commit_info, commit_infos)
+ return sorted(set(itertools.chain.from_iterable(nickname_lists)))
+
+ def _nicks_string(self, tool, sheriff, requester_nick, svn_revision_list):
+ # FIXME: _parse_args guarentees that our svn_revision_list is all numbers.
+ # However, it's possible our checkout will not include one of the revisions,
+ # so we may need to catch exceptions from commit_info_for_revision here.
+ target_nicks = [requester_nick] + self._responsible_nicknames_from_revisions(tool, sheriff, svn_revision_list)
+ return ", ".join(target_nicks)
+
def execute(self, nick, args, tool, sheriff):
svn_revision_list, rollout_reason = self._parse_args(args)
- if (len(svn_revision_list) == 0) or (len(rollout_reason) == 0):
- tool.irc().post("%s: Usage: SVN_REVISION [SVN_REVISIONS] REASON" % nick)
- return
+ if (not svn_revision_list or not rollout_reason):
+ # return is equivalent to an irc().post(), but makes for easier unit testing.
+ return "%s: Usage: SVN_REVISION [SVN_REVISIONS] REASON" % nick
- rollout_reason = " ".join(rollout_reason)
+ # FIXME: IRCCommand should bind to a tool and have a self._tool like Command objects do.
+ # Likewise we should probably have a self._sheriff.
+ nicks_string = self._nicks_string(tool, sheriff, nick, svn_revision_list)
- tool.irc().post("Preparing rollout for %s..." %
- join_with_separators(["r" + str(revision) for revision in svn_revision_list]))
+ revision_urls_string = join_with_separators([urls.view_revision_url(revision) for revision in svn_revision_list])
+ tool.irc().post("%s: Preparing rollout for %s..." % (nicks_string, revision_urls_string))
+
try:
complete_reason = "%s (Requested by %s on %s)." % (
rollout_reason, nick, config_irc.channel)
bug_id = sheriff.post_rollout_patch(svn_revision_list, complete_reason)
bug_url = tool.bugs.bug_url_for_bug_id(bug_id)
- tool.irc().post("%s: Created rollout: %s" % (nick, bug_url))
+ tool.irc().post("%s: Created rollout: %s" % (nicks_string, bug_url))
except ScriptError, e:
- tool.irc().post("%s: Failed to create rollout patch:" % nick)
+ tool.irc().post("%s: Failed to create rollout patch:" % nicks_string)
tool.irc().post("%s" % e)
bug_id = parse_bug_id(e.output)
if bug_id:
- tool.irc().post("Ugg... Might have created %s" %
- tool.bugs.bug_url_for_bug_id(bug_id))
+ tool.irc().post("%s: Ugg... Might have created %s" % (nicks_string, tool.bugs.bug_url_for_bug_id(bug_id)))
class Help(IRCCommand):
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py (89479 => 89480)
--- trunk/Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py 2011-06-22 21:23:16 UTC (rev 89479)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py 2011-06-22 21:33:35 UTC (rev 89480)
@@ -69,3 +69,18 @@
tool.bugs.create_bug = mock_create_bug
self.assertEquals("tom: Failed to create bug:\nException from bugzilla!",
create_bug.execute("tom", example_args, tool, None))
+
+ def test_rollout(self):
+ rollout = Rollout()
+ self.assertEquals(([1234], "testing foo"),
+ rollout._parse_args(["1234", "testing", "foo"]))
+
+ # Test invalid argument parsing:
+ self.assertEquals((None, None), rollout._parse_args([]))
+ self.assertEquals((None, None), rollout._parse_args(["--bar", "1234"]))
+
+ # Invalid arguments result in the USAGE message.
+ self.assertEquals("tom: Usage: SVN_REVISION [SVN_REVISIONS] REASON",
+ rollout.execute("tom", [], None, None))
+
+ # FIXME: We need a better way to test IRCCommands which call tool.irc().post()
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/sheriff.py (89479 => 89480)
--- trunk/Tools/Scripts/webkitpy/tool/bot/sheriff.py 2011-06-22 21:23:16 UTC (rev 89479)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/sheriff.py 2011-06-22 21:33:35 UTC (rev 89480)
@@ -38,10 +38,11 @@
self._tool = tool
self._sheriffbot = sheriffbot
+ def responsible_nicknames_from_commit_info(self, commit_info):
+ return [party.irc_nickname for party in commit_info.responsible_parties() if party.irc_nickname]
+
def post_irc_warning(self, commit_info, builders):
- irc_nicknames = sorted([party.irc_nickname for
- party in commit_info.responsible_parties()
- if party.irc_nickname])
+ irc_nicknames = sorted(self.responsible_nicknames_from_commit_info(commit_info))
irc_prefix = ": " if irc_nicknames else ""
irc_message = "%s%s%s might have broken %s" % (
", ".join(irc_nicknames),
Modified: trunk/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py (89479 => 89480)
--- trunk/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py 2011-06-22 21:23:16 UTC (rev 89479)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py 2011-06-22 21:33:35 UTC (rev 89480)
@@ -59,19 +59,19 @@
OutputCapture().assert_outputs(self, run, args=["last-green-revision"], expected_stderr=expected_stderr)
def test_rollout(self):
- expected_stderr = "MOCK: irc.post: Preparing rollout for r21654...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n"
+ expected_stderr = "MOCK: irc.post: mock_nick, abarth, darin, eseidel: Preparing rollout for http://trac.webkit.org/changeset/21654...\nMOCK: irc.post: mock_nick, abarth, darin, eseidel: Created rollout: http://example.com/36936\n"
OutputCapture().assert_outputs(self, run, args=["rollout 21654 This patch broke the world"], expected_stderr=expected_stderr)
def test_multi_rollout(self):
- expected_stderr = "MOCK: irc.post: Preparing rollout for r21654, r21655, and r21656...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n"
+ expected_stderr = "MOCK: irc.post: mock_nick, abarth, darin, eseidel: Preparing rollout for http://trac.webkit.org/changeset/21654, http://trac.webkit.org/changeset/21655, and http://trac.webkit.org/changeset/21656...\nMOCK: irc.post: mock_nick, abarth, darin, eseidel: Created rollout: http://example.com/36936\n"
OutputCapture().assert_outputs(self, run, args=["rollout 21654 21655 21656 This 21654 patch broke the world"], expected_stderr=expected_stderr)
def test_rollout_with_r_in_svn_revision(self):
- expected_stderr = "MOCK: irc.post: Preparing rollout for r21654...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n"
+ expected_stderr = "MOCK: irc.post: mock_nick, abarth, darin, eseidel: Preparing rollout for http://trac.webkit.org/changeset/21654...\nMOCK: irc.post: mock_nick, abarth, darin, eseidel: Created rollout: http://example.com/36936\n"
OutputCapture().assert_outputs(self, run, args=["rollout r21654 This patch broke the world"], expected_stderr=expected_stderr)
def test_multi_rollout_with_r_in_svn_revision(self):
- expected_stderr = "MOCK: irc.post: Preparing rollout for r21654, r21655, and r21656...\nMOCK: irc.post: mock_nick: Created rollout: http://example.com/36936\n"
+ expected_stderr = "MOCK: irc.post: mock_nick, abarth, darin, eseidel: Preparing rollout for http://trac.webkit.org/changeset/21654, http://trac.webkit.org/changeset/21655, and http://trac.webkit.org/changeset/21656...\nMOCK: irc.post: mock_nick, abarth, darin, eseidel: Created rollout: http://example.com/36936\n"
OutputCapture().assert_outputs(self, run, args=["rollout r21654 21655 r21656 This r21654 patch broke the world"], expected_stderr=expected_stderr)
def test_rollout_bananas(self):
@@ -79,31 +79,27 @@
OutputCapture().assert_outputs(self, run, args=["rollout bananas"], expected_stderr=expected_stderr)
def test_rollout_invalidate_revision(self):
- expected_stderr = ("MOCK: irc.post: Preparing rollout for r--component=Tools...\n"
- "MOCK: irc.post: mock_nick: Failed to create rollout patch:\n"
- "MOCK: irc.post: Invalid svn revision number \"--component=Tools\".\n")
+ # When folks pass junk arguments, we should just spit the usage back at them.
+ expected_stderr = "MOCK: irc.post: mock_nick: Usage: SVN_REVISION [SVN_REVISIONS] REASON\n"
OutputCapture().assert_outputs(self, run,
- args=["rollout "
- "--component=Tools 21654"],
+ args=["rollout --component=Tools 21654"],
expected_stderr=expected_stderr)
def test_rollout_invalidate_reason(self):
- expected_stderr = ("MOCK: irc.post: Preparing rollout for "
- "r21654...\nMOCK: irc.post: mock_nick: Failed to "
- "create rollout patch:\nMOCK: irc.post: The rollout"
- " reason may not begin with - (\"-bad (Requested "
- "by mock_nick on #webkit).\").\n")
+ # FIXME: I'm slightly confused as to why this doesn't return the USAGE message.
+ expected_stderr = """MOCK: irc.post: mock_nick, abarth, darin, eseidel: Preparing rollout for http://trac.webkit.org/changeset/21654...
+MOCK: irc.post: mock_nick, abarth, darin, eseidel: Failed to create rollout patch:
+MOCK: irc.post: The rollout reason may not begin with - (\"-bad (Requested by mock_nick on #webkit).\").
+"""
OutputCapture().assert_outputs(self, run,
- args=["rollout "
- "21654 -bad"],
+ args=["rollout 21654 -bad"],
expected_stderr=expected_stderr)
def test_multi_rollout_invalidate_reason(self):
- expected_stderr = ("MOCK: irc.post: Preparing rollout for "
- "r21654, r21655, and r21656...\nMOCK: irc.post: mock_nick: Failed to "
- "create rollout patch:\nMOCK: irc.post: The rollout"
- " reason may not begin with - (\"-bad (Requested "
- "by mock_nick on #webkit).\").\n")
+ expected_stderr = """MOCK: irc.post: mock_nick, abarth, darin, eseidel: Preparing rollout for http://trac.webkit.org/changeset/21654, http://trac.webkit.org/changeset/21655, and http://trac.webkit.org/changeset/21656...
+MOCK: irc.post: mock_nick, abarth, darin, eseidel: Failed to create rollout patch:
+MOCK: irc.post: The rollout reason may not begin with - (\"-bad (Requested by mock_nick on #webkit).\").
+"""
OutputCapture().assert_outputs(self, run,
args=["rollout "
"21654 21655 r21656 -bad"],
