zulip_bots: Reify StateHandler testing.

This simplifies testing stateful bots by integrating the StateHandler
into the test library. As a side-effect, the mock bot handler gets
reused during a test, making the tests more realistic. The
StateHandler now keeps its state during a call to check_expected_responses,
forcing some stateful tests to be more verbose and explicit.

zulip_bots/zulip_bots/bots/incrementor/test_incrementor.py

@@ -23,9 +23,8 @@ class TestIncrementorBot(BotTestCase):
                 'sender_email': 'foo_sender@zulip.com',
             },
         ]
-        storage = StateHandler()
         self.assert_bot_response(dict(messages[0], content=""), {'content': "1"},
-                                 'send_reply', storage)
+                                 'send_reply')
         # Last test commented out since we don't have update_message
         # support in the test framework yet.
 

zulip_bots/zulip_bots/bots/tictactoe/test_tictactoe.py

@@ -86,11 +86,11 @@ class TestTictactoeBot(BotTestCase):
             ("1,1", msg['after_1_1']),
             ("2, 1", msg['after_2_1']),
             ("(1,3)", msg['after_1_3']),
+            ("quit", msg['successful_quit']),
             # Can't test 'after_3_2' as it's random!
         ]
         for m in messages:
-            state = StateHandler()
             for (mesg, resp) in expected_send_message:
                 self.assert_bot_response(dict(m, content=mesg),
                                          dict(private_response, content=resp),
-                                         'send_message', state)
+                                         'send_message')

zulip_bots/zulip_bots/bots/virtual_fs/test_virtual_fs.py

@@ -8,8 +8,6 @@ from zulip_bots.lib import StateHandler
 
 class TestVirtualFsBot(BotTestCase):
     bot_name = "virtual_fs"
-
-    def test_bot(self):
     help_txt = ('foo_sender@zulip.com:\n\nThis bot implements a virtual file system for a stream.\n'
                 'The locations of text are persisted for the lifetime of the bot\n'
                 'running, and if you rename a stream, you will lose the info.\n'
@@ -25,25 +23,26 @@ class TestVirtualFsBot(BotTestCase):
                 '@mention-bot rmdir: remove a directory\n'
                 '```\n'
                 'Use commands like `@mention-bot help write` for more details on specific\ncommands.\n')
-        expected = {
+
-            "cd /home": "foo_sender@zulip.com:\nERROR: invalid path",
+    def test_commands_1(self):
-            "mkdir home": "foo_sender@zulip.com:\ndirectory created",
+        expected = [
-            "pwd": "foo_sender@zulip.com:\n/",
+            ("cd /home", "foo_sender@zulip.com:\nERROR: invalid path"),
-            "help": help_txt,
+            ("mkdir home", "foo_sender@zulip.com:\ndirectory created"),
-            "help ls": "foo_sender@zulip.com:\nsyntax: ls <optional_path>",
+            ("pwd", "foo_sender@zulip.com:\n/"),
-            "": help_txt,
+            ("help", self.help_txt),
-        }
+            ("help ls", "foo_sender@zulip.com:\nsyntax: ls <optional_path>"),
+            ("", self.help_txt),
+        ]
         self.check_expected_responses(expected)
 
+    def test_commands_2(self):
         expected = [
-            ("help", help_txt),
+            ("help", self.help_txt),
             ("help ls", "foo_sender@zulip.com:\nsyntax: ls <optional_path>"),
-            ("", help_txt),
+            ("", self.help_txt),
             ("pwd", "foo_sender@zulip.com:\n/"),
             ("cd /home", "foo_sender@zulip.com:\nERROR: invalid path"),
             ("mkdir home", "foo_sender@zulip.com:\ndirectory created"),
             ("cd /home", "foo_sender@zulip.com:\nCurrent path: /home/"),
         ]
-
+        self.check_expected_responses(expected)
-        storage = StateHandler()
-        self.check_expected_responses(expected, storage = storage)

zulip_bots/zulip_bots/test_lib.py

@@ -43,6 +43,8 @@ class BotTestCase(TestCase):
         # Mocking ExternalBotHandler
         self.patcher = patch('zulip_bots.lib.ExternalBotHandler', autospec=True)
         self.MockClass = self.patcher.start()
+        self.mock_bot_handler = self.MockClass(None, None)
+        self.mock_bot_handler.storage = StateHandler()
         self.message_handler = self.get_bot_message_handler()
 
     def tearDown(self):
@@ -51,13 +53,12 @@ class BotTestCase(TestCase):
 
     def initialize_bot(self):
         # type: () -> None
-        self.message_handler.initialize(self.MockClass(None, None))
+        self.message_handler.initialize(self.mock_bot_handler)
 
     def check_expected_responses(self, expectations, expected_method='send_reply',
                                  email="foo_sender@zulip.com", recipient="foo", subject="foo",
-                                 sender_id=0, sender_full_name="Foo Bar", type="all",
+                                 sender_id=0, sender_full_name="Foo Bar", type="all"):
-                                 storage=None):
+        # type: (Union[Sequence[Tuple[str, Any]], Dict[str, Any]], str, str, str, str, int, str, str) -> None
-        # type: (Union[Sequence[Tuple[str, Any]], Dict[str, Any]], str, str, str, str, int, str, str, Optional[StateHandler]) -> None
         # To test send_message, Any would be a Dict type,
         # to test send_reply, Any would be a str type.
         if isinstance(expectations, dict):
@@ -68,45 +69,38 @@ class BotTestCase(TestCase):
         if type not in ["private", "stream", "all"]:
             logging.exception("check_expected_response expects type to be 'private', 'stream' or 'all'")
 
-        private_message_dict = {'type': "private", 'display_recipient': recipient,
+        private_message_template = {'type': "private", 'display_recipient': recipient,
                                     'sender_email': email, 'sender_id': sender_id,
                                     'sender_full_name': sender_full_name}
-        stream_message_dict = {'type': "stream", 'display_recipient': recipient,
+        stream_message_template = {'type': "stream", 'display_recipient': recipient,
                                    'subject': subject, 'sender_email': email, 'sender_id': sender_id,
                                    'sender_full_name': sender_full_name}
 
-        trigger_messages = []
+        message_templates = []
         if type in ["private", "all"]:
-            trigger_messages.append(private_message_dict)
+            message_templates.append(private_message_template)
         if type in ["stream", "all"]:
-            trigger_messages.append(stream_message_dict)
+            message_templates.append(stream_message_template)
 
-        for trigger_message in trigger_messages:
+        initial_storage = deepcopy(self.mock_bot_handler.storage)
-            if storage is None:
+        for message_template in message_templates:
-                current_storage = None
+            # A new copy of the StateHandler is used for every new conversation with a
-            else:
+            # different base template. This avoids type="all" failing if the created state
-                # A new (copy of) the StateHandler is used for each trigger message.
+            # of a prior conversation influences the current one.
-                # This avoids type="all" failing if state is created in the first iteration.
+            self.mock_bot_handler.storage = deepcopy(initial_storage)
-                current_storage = deepcopy(storage)
+            for m, r in expectations:
-
-            for m, r in expected:
                 # For calls with send_reply, r is a string (the content of a message),
                 # so we need to add it to a Dict as the value of 'content'.
                 # For calls with send_message, r is already a Dict.
-                message = dict(trigger_message, content = m)
+                message = dict(message_template, content = m)
                 response = {'content': r} if expected_method == 'send_reply' else r
                 self.assert_bot_response(message=message, response=response,
-                                         expected_method=expected_method,
+                                         expected_method=expected_method)
-                                         storage=current_storage)
 
-    def call_request(self, message, expected_method, response, storage):
+    def call_request(self, message, expected_method, response):
-        # type: (Dict[str, Any], str, Dict[str, Any], Optional[StateHandler]) -> None
+        # type: (Dict[str, Any], str, Dict[str, Any]) -> None
-        if storage is None:
-            storage = StateHandler()
         # Send message to the concerned bot
-        mock_bot_handler = self.MockClass(None, None)
+        self.message_handler.handle_message(message, self.mock_bot_handler)
-        mock_bot_handler.storage = storage
-        self.message_handler.handle_message(message, mock_bot_handler)
 
         # Check if the bot is sending a message via `send_message` function.
         # Where response is a dictionary here.
@@ -159,8 +153,8 @@ class BotTestCase(TestCase):
                 else:
                     mock_get.assert_called_with(http_request['api_url'])
 
-    def assert_bot_response(self, message, response, expected_method, storage = None):
+    def assert_bot_response(self, message, response, expected_method):
-        # type: (Dict[str, Any], Dict[str, Any], str, Optional[StateHandler]) -> None
+        # type: (Dict[str, Any], Dict[str, Any], str) -> None
         # Strictly speaking, this function is not needed anymore,
         # kept for now for legacy reasons.
-        self.call_request(message, expected_method, response, storage)
+        self.call_request(message, expected_method, response)