Sheriffbot rollout should be more intuitive.
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Sep 2011 00:56:48 +0000 (00:56 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Sep 2011 00:56:48 +0000 (00:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=68415

Reviewed by Adam Barth.

* Scripts/webkitpy/tool/bot/irc_command.py: Add support for revert and comma separated args.
* Scripts/webkitpy/tool/bot/irc_command_unittest.py: Add parsing tests for comma separated args
  and a few others cases.
* Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py: Verify that revert works.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@95500 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/tool/bot/irc_command.py
Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py
Tools/Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py

index a22489f..41999ba 100644 (file)
@@ -1,5 +1,17 @@
 2011-09-19  David Levin  <levin@chromium.org>
 
+        Sheriffbot rollout should be more intuitive.
+        https://bugs.webkit.org/show_bug.cgi?id=68415
+
+        Reviewed by Adam Barth.
+
+        * Scripts/webkitpy/tool/bot/irc_command.py: Add support for revert and comma separated args.
+        * Scripts/webkitpy/tool/bot/irc_command_unittest.py: Add parsing tests for comma separated args
+          and a few others cases.
+        * Scripts/webkitpy/tool/bot/sheriffircbot_unittest.py: Verify that revert works.
+
+2011-09-19  David Levin  <levin@chromium.org>
+
         check-webkit-style generates bogus warning for StructuredExceptionHandlerSupressor.h
         https://bugs.webkit.org/show_bug.cgi?id=68391
 
index dad5181..bfdb344 100644 (file)
@@ -66,26 +66,42 @@ class Restart(IRCCommand):
 
 
 class Rollout(IRCCommand):
+    def _extract_revisions(self, arg):
+
+        revision_list = []
+        possible_revisions = arg.split(",")
+        for revision in possible_revisions:
+            revision = revision.strip()
+            if not revision:
+                continue
+            revision = revision.lstrip("r")
+            # If one part of the arg isn't in the correct format,
+            # then none of the arg should be considered a revision.
+            if not revision.isdigit():
+                return None
+            revision_list.append(int(revision))
+        return revision_list
+
     def _parse_args(self, args):
         if not args:
             return (None, None)
 
-        # the first argument must be a revision number
-        first_revision = args[0].lstrip("r")
-        if not first_revision.isdigit():
+        svn_revision_list = []
+        remaining_args = args[:]
+        # First process all revisions.
+        while remaining_args:
+            new_revisions = self._extract_revisions(remaining_args[0])
+            if not new_revisions:
+                break
+            svn_revision_list += new_revisions
+            remaining_args = remaining_args[1:]
+
+        # Was there a revision number?
+        if not len(svn_revision_list):
             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 parsing_revision:
-                svn_revision_list.append(int(arg.lstrip("r")))
-            else:
-                parsing_revision = False
-                rollout_reason.append(arg)
-
-        rollout_reason = " ".join(rollout_reason)
+        # Everything left is the reason.
+        rollout_reason = " ".join(remaining_args)
         return svn_revision_list, rollout_reason
 
     def _responsible_nicknames_from_revisions(self, tool, sheriff, svn_revision_list):
@@ -161,7 +177,7 @@ class RollChromiumDEPS(IRCCommand):
 
 class Help(IRCCommand):
     def execute(self, nick, args, tool, sheriff):
-        return "%s: Available commands: %s" % (nick, ", ".join(sorted(commands.keys())))
+        return "%s: Available commands: %s" % (nick, ", ".join(sorted(visible_commands.keys())))
 
 
 class Hi(IRCCommand):
@@ -235,7 +251,7 @@ class CreateBug(IRCCommand):
 
 
 # FIXME: Lame.  We should have an auto-registering CommandCenter.
-commands = {
+visible_commands = {
     "help": Help,
     "hi": Hi,
     "last-green-revision": LastGreenRevision,
@@ -245,3 +261,10 @@ commands = {
     "create-bug": CreateBug,
     "roll-chromium-deps": RollChromiumDEPS,
 }
+
+# Add revert as an "easter egg" command. Why?
+# revert is the same as rollout and it would be confusing to list both when
+# they do the same thing. However, this command is a very natural thing for
+# people to use and it seems silly to have them hunt around for "rollout" instead.
+commands = visible_commands.copy()
+commands["revert"] = Rollout
index 5736f4e..8bb23eb 100644 (file)
@@ -98,6 +98,18 @@ class IRCCommandTest(unittest.TestCase):
         self.assertEquals(([1234], "testing foo"),
                           rollout._parse_args(["1234", "testing", "foo"]))
 
+        self.assertEquals(([554], "testing foo"),
+                          rollout._parse_args(["r554", "testing", "foo"]))
+
+        self.assertEquals(([556, 792], "testing foo"),
+                          rollout._parse_args(["r556", "792", "testing", "foo"]))
+
+        self.assertEquals(([128, 256], "testing foo"),
+                          rollout._parse_args(["r128,r256", "testing", "foo"]))
+
+        self.assertEquals(([512, 1024, 2048], "testing foo"),
+                          rollout._parse_args(["512,", "1024,2048", "testing", "foo"]))
+
         # Test invalid argument parsing:
         self.assertEquals((None, None), rollout._parse_args([]))
         self.assertEquals((None, None), rollout._parse_args(["--bar", "1234"]))
index c0bc992..d1f5f66 100644 (file)
@@ -97,6 +97,10 @@ class SheriffIRCBotTest(unittest.TestCase):
         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_revert(self):
+        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=["revert 21654 This patch broke the world"], expected_stderr=expected_stderr)
+
     def test_roll_chromium_deps(self):
         expected_stderr = "MOCK: irc.post: mock_nick: Rolling Chromium DEPS to r21654\nMOCK: irc.post: mock_nick: Created DEPS roll: http://example.com/36936\n"
         OutputCapture().assert_outputs(self, run, args=["roll-chromium-deps 21654"], expected_stderr=expected_stderr)