From 8179b30873007d0a1e87d2cd4657f2446b7aee80 Mon Sep 17 00:00:00 2001 From: derAnfaenger Date: Mon, 23 Oct 2017 20:37:39 +0200 Subject: [PATCH] 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. --- .../bots/incrementor/test_incrementor.py | 3 +- .../bots/tictactoe/test_tictactoe.py | 4 +- .../bots/virtual_fs/test_virtual_fs.py | 57 ++++++++--------- zulip_bots/zulip_bots/test_lib.py | 64 +++++++++---------- 4 files changed, 60 insertions(+), 68 deletions(-) diff --git a/zulip_bots/zulip_bots/bots/incrementor/test_incrementor.py b/zulip_bots/zulip_bots/bots/incrementor/test_incrementor.py index 121388e..64f4cf1 100644 --- a/zulip_bots/zulip_bots/bots/incrementor/test_incrementor.py +++ b/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. diff --git a/zulip_bots/zulip_bots/bots/tictactoe/test_tictactoe.py b/zulip_bots/zulip_bots/bots/tictactoe/test_tictactoe.py index 6fa5a2c..e388d17 100644 --- a/zulip_bots/zulip_bots/bots/tictactoe/test_tictactoe.py +++ b/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') diff --git a/zulip_bots/zulip_bots/bots/virtual_fs/test_virtual_fs.py b/zulip_bots/zulip_bots/bots/virtual_fs/test_virtual_fs.py index aa85bbb..f87fa99 100755 --- a/zulip_bots/zulip_bots/bots/virtual_fs/test_virtual_fs.py +++ b/zulip_bots/zulip_bots/bots/virtual_fs/test_virtual_fs.py @@ -8,42 +8,41 @@ from zulip_bots.lib import StateHandler class TestVirtualFsBot(BotTestCase): bot_name = "virtual_fs" + 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' + 'Example commands:\n\n```\n' + '@mention-bot sample_conversation: sample conversation with the bot\n' + '@mention-bot mkdir: create a directory\n' + '@mention-bot ls: list a directory\n' + '@mention-bot cd: change directory\n' + '@mention-bot pwd: show current path\n' + '@mention-bot write: write text\n' + '@mention-bot read: read text\n' + '@mention-bot rm: remove a file\n' + '@mention-bot rmdir: remove a directory\n' + '```\n' + 'Use commands like `@mention-bot help write` for more details on specific\ncommands.\n') - 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' - 'Example commands:\n\n```\n' - '@mention-bot sample_conversation: sample conversation with the bot\n' - '@mention-bot mkdir: create a directory\n' - '@mention-bot ls: list a directory\n' - '@mention-bot cd: change directory\n' - '@mention-bot pwd: show current path\n' - '@mention-bot write: write text\n' - '@mention-bot read: read text\n' - '@mention-bot rm: remove a file\n' - '@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", - "mkdir home": "foo_sender@zulip.com:\ndirectory created", - "pwd": "foo_sender@zulip.com:\n/", - "help": help_txt, - "help ls": "foo_sender@zulip.com:\nsyntax: ls ", - "": help_txt, - } + def test_commands_1(self): + expected = [ + ("cd /home", "foo_sender@zulip.com:\nERROR: invalid path"), + ("mkdir home", "foo_sender@zulip.com:\ndirectory created"), + ("pwd", "foo_sender@zulip.com:\n/"), + ("help", self.help_txt), + ("help ls", "foo_sender@zulip.com:\nsyntax: ls "), + ("", 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 "), - ("", 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/"), ] - - storage = StateHandler() - self.check_expected_responses(expected, storage = storage) + self.check_expected_responses(expected) diff --git a/zulip_bots/zulip_bots/test_lib.py b/zulip_bots/zulip_bots/test_lib.py index edb5a70..78e73c4 100755 --- a/zulip_bots/zulip_bots/test_lib.py +++ b/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", - storage=None): - # type: (Union[Sequence[Tuple[str, Any]], Dict[str, Any]], str, str, str, str, int, str, str, Optional[StateHandler]) -> None + sender_id=0, sender_full_name="Foo Bar", type="all"): + # type: (Union[Sequence[Tuple[str, Any]], Dict[str, Any]], str, str, str, str, int, str, str) -> 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, - 'sender_email': email, 'sender_id': sender_id, - 'sender_full_name': sender_full_name} - stream_message_dict = {'type': "stream", 'display_recipient': recipient, - 'subject': subject, 'sender_email': email, 'sender_id': sender_id, - 'sender_full_name': sender_full_name} + private_message_template = {'type': "private", 'display_recipient': recipient, + 'sender_email': email, 'sender_id': sender_id, + 'sender_full_name': sender_full_name} + 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: - if storage is None: - current_storage = None - else: - # A new (copy of) the StateHandler is used for each trigger message. - # This avoids type="all" failing if state is created in the first iteration. - current_storage = deepcopy(storage) - - for m, r in expected: + initial_storage = deepcopy(self.mock_bot_handler.storage) + for message_template in message_templates: + # A new copy of the StateHandler is used for every new conversation with a + # different base template. This avoids type="all" failing if the created state + # of a prior conversation influences the current one. + self.mock_bot_handler.storage = deepcopy(initial_storage) + for m, r in expectations: # 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, - storage=current_storage) + expected_method=expected_method) - def call_request(self, message, expected_method, response, storage): - # type: (Dict[str, Any], str, Dict[str, Any], Optional[StateHandler]) -> None - if storage is None: - storage = StateHandler() + def call_request(self, message, expected_method, response): + # type: (Dict[str, Any], str, Dict[str, Any]) -> None # Send message to the concerned bot - mock_bot_handler = self.MockClass(None, None) - mock_bot_handler.storage = storage - self.message_handler.handle_message(message, mock_bot_handler) + self.message_handler.handle_message(message, self.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): - # type: (Dict[str, Any], Dict[str, Any], str, Optional[StateHandler]) -> None + def assert_bot_response(self, message, response, expected_method): + # 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)