bots: Refactor and simplify bot_test_lib.py.

This commit integrates the mock_test function in
check_expected_responses and makes this function usable
for simple tests only by not allowing mock http conversations.

assert_bot_response() is now used for single message-response pairs,
resolving potential issues with multiple messages and a single http
request-response pair.

The giphy bot test file is updated accordingly.

api/bots/giphy/test_giphy.py

@@ -51,11 +51,10 @@ class TestGiphyBot(BotTestCase):
         # This message calls `send_reply` function of BotHandlerApi
         keyword = "Hello"
         gif_url = "https://media4.giphy.com/media/3o6ZtpxSZbQRRnwCKQ/giphy.gif"
-        expectations = {
+        self.assert_bot_response(
-            keyword: get_bot_response(gif_url)
+            message = {'content': keyword},
-        }
+            response = {'content': get_bot_response(gif_url)},
-        self.check_expected_responses(
+            expected_method='send_reply',
-            expectations=expectations,
             http_request=get_http_request(keyword),
             http_response=get_http_response_json(gif_url)
         )

api/bots_api/bots_test_lib.py

@@ -29,38 +29,25 @@ class BotTestCase(TestCase):
 
     def check_expected_responses(self, expectations, expected_method='send_reply',
                                  email="foo_sender@zulip.com", recipient="foo", subject="foo",
-                                 type="all", http_request=None, http_response=None):
+                                 type="all"):
-        # type: (Dict[str, Any], str, str, str, str, str, Dict[str, Any], Dict[str, Any]) -> None
+        # type: (Dict[str, Any], str, str, str, str, str) -> None
         # To test send_message, Any would be a Dict type,
         # to test send_reply, Any would be a str type.
         if type not in ["private", "stream", "all"]:
             logging.exception("check_expected_response expects type to be 'private', 'stream' or 'all'")
         for m, r in expectations.items():
+            # 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.
+            response = {'content': r} if expected_method == 'send_reply' else r
             if type != "stream":
-                self.mock_test(
+                message = {'content': m, 'type': "private", 'display_recipient': recipient,
-                    messages={'content': m, 'type': "private", 'display_recipient': recipient,
+                           'sender_email': email}
-                              'sender_email': email}, bot_response=r, expected_method=expected_method,
+                self.assert_bot_response(message=message, response=response, expected_method=expected_method)
-                    http_request=http_request, http_response=http_response)
             if type != "private":
-                self.mock_test(
+                message = {'content': m, 'type': "stream", 'display_recipient': recipient,
-                    messages={'content': m, 'type': "stream", 'display_recipient': recipient,
+                           'subject': subject, 'sender_email': email}
-                              'subject': subject, 'sender_email': email}, bot_response=r,
+                self.assert_bot_response(message=message, response=response, expected_method=expected_method)
-                    expected_method=expected_method, http_request=http_request, http_response=http_response)
-
-    def mock_test(self, messages, bot_response, expected_method,
-                  http_request=None, http_response=None):
-        # type: (Dict[str, str], Any, str, Dict[str, Any], Dict[str, Any]) -> None
-        if expected_method == "send_message":
-            # Since send_message function uses bot_response of type Dict, no
-            # further changes required.
-            self.assert_bot_output(messages=[messages], bot_response=[bot_response], expected_method=expected_method,
-                                   http_request=http_request, http_response=http_response)
-        else:
-            # Since send_reply function uses bot_response of type str, we
-            # do convert the str type to a Dict type to have the same assert_bot_output function.
-            bot_response_type_dict = {'content': bot_response}
-            self.assert_bot_output(messages=[messages], bot_response=[bot_response_type_dict], expected_method=expected_method,
-                                   http_request=http_request, http_response=http_response)
 
     def get_bot_message_handler(self):
         # type: () -> Any
@@ -75,7 +62,7 @@ class BotTestCase(TestCase):
 
     def call_request(self, message_handler, message, expected_method,
                      MockClass, response):
-        # type: (Any, Dict[str, Any], str, Any, Optional[Dict[str, Any]]) -> None
+        # type: (Any, Dict[str, Any], str, Any, Dict[str, Any]) -> None
         # Send message to the concerned bot
         message_handler.handle_message(message, MockClass(), StateHandler())
 
@@ -87,40 +74,39 @@ class BotTestCase(TestCase):
         else:
             instance.send_reply.assert_called_with(message, response['content'])
 
-    def assert_bot_output(self, messages, bot_response, expected_method,
+    def assert_bot_response(self, message, response, expected_method,
-                          http_request=None, http_response=None):
+                            http_request=None, http_response=None):
-        # type: (List[Dict[str, Any]], List[Dict[str, str]], str, Optional[Dict[str, Any]], Optional[Dict[str, Any]]) -> None
+        # type: (Dict[str, Any], Dict[str, Any], str, Optional[Dict[str, Any]], Optional[Dict[str, Any]]) -> None
         message_handler = self.get_bot_message_handler()
         # Mocking BotHandlerApi
         with patch('bots_api.bot_lib.BotHandlerApi') as MockClass:
-            for (message, response) in zip(messages, bot_response):
+            # If not mock http_request/http_response are provided,
-                # If not mock http_request/http_response are provided,
+            # just call the request normally (potentially using
-                # just call the request normally (potentially using
+            # the Internet)
-                # the Internet)
+            if http_response is None:
-                if http_response is None:
+                assert http_request is None
-                    assert http_request is None
+                self.call_request(message_handler, message, expected_method,
-                    self.call_request(message_handler, message, expected_method,
+                                  MockClass, response)
-                                      MockClass, response)
+                return
-                    continue
 
-                # Otherwise, we mock requests, and verify that the bot
+            # Otherwise, we mock requests, and verify that the bot
-                # made the correct HTTP request to the third-party API
+            # made the correct HTTP request to the third-party API
-                # (and provide the correct third-party API response.
+            # (and provide the correct third-party API response.
-                # This allows us to test things that would require the
+            # This allows us to test things that would require the
-                # Internet without it).
+            # Internet without it).
-                assert http_request is not None
+            assert http_request is not None
-                with patch('requests.get') as mock_get:
+            with patch('requests.get') as mock_get:
-                    mock_result = mock.MagicMock()
+                mock_result = mock.MagicMock()
-                    mock_result.json.return_value = http_response
+                mock_result.json.return_value = http_response
-                    mock_result.ok.return_value = True
+                mock_result.ok.return_value = True
-                    mock_get.return_value = mock_result
+                mock_get.return_value = mock_result
-                    self.call_request(message_handler, message, expected_method,
+                self.call_request(message_handler, message, expected_method,
-                                      MockClass, response)
+                                  MockClass, response)
-                    # Check if the bot is sending the correct http_request corresponding
+                # Check if the bot is sending the correct http_request corresponding
-                    # to the given http_response.
+                # to the given http_response.
-                    if http_request is not None:
+                if http_request is not None:
-                        mock_get.assert_called_with(http_request['api_url'],
+                    mock_get.assert_called_with(http_request['api_url'],
-                                                    params=http_request['params'])
+                                                params=http_request['params'])
 
     def bot_to_run(self, bot_module):
         # Returning Any, same argument as in get_bot_message_handler function.