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.
This commit is contained in:
		
							parent
							
								
									894a816618
								
							
						
					
					
						commit
						8f2fc6069d
					
				
					 2 changed files with 46 additions and 61 deletions
				
			
		|  | @ -51,11 +51,10 @@ class TestGiphyBot(BotTestCase): | ||||||
|         # This message calls `send_reply` function of BotHandlerApi |         # This message calls `send_reply` function of BotHandlerApi | ||||||
|         keyword = "Hello" |         keyword = "Hello" | ||||||
|         gif_url = "https://media4.giphy.com/media/3o6ZtpxSZbQRRnwCKQ/giphy.gif" |         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_request=get_http_request(keyword), | ||||||
|             http_response=get_http_response_json(gif_url) |             http_response=get_http_response_json(gif_url) | ||||||
|         ) |         ) | ||||||
|  |  | ||||||
|  | @ -29,38 +29,25 @@ class BotTestCase(TestCase): | ||||||
| 
 | 
 | ||||||
|     def check_expected_responses(self, expectations, expected_method='send_reply', |     def check_expected_responses(self, expectations, expected_method='send_reply', | ||||||
|                                  email="foo_sender@zulip.com", recipient="foo", subject="foo", |                                  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_message, Any would be a Dict type, | ||||||
|         # to test send_reply, Any would be a str type. |         # to test send_reply, Any would be a str type. | ||||||
|         if type not in ["private", "stream", "all"]: |         if type not in ["private", "stream", "all"]: | ||||||
|             logging.exception("check_expected_response expects type to be 'private', 'stream' or 'all'") |             logging.exception("check_expected_response expects type to be 'private', 'stream' or 'all'") | ||||||
|         for m, r in expectations.items(): |         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": |             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": |             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): |     def get_bot_message_handler(self): | ||||||
|         # type: () -> Any |         # type: () -> Any | ||||||
|  | @ -75,7 +62,7 @@ class BotTestCase(TestCase): | ||||||
| 
 | 
 | ||||||
|     def call_request(self, message_handler, message, expected_method, |     def call_request(self, message_handler, message, expected_method, | ||||||
|                      MockClass, response): |                      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 |         # Send message to the concerned bot | ||||||
|         message_handler.handle_message(message, MockClass(), StateHandler()) |         message_handler.handle_message(message, MockClass(), StateHandler()) | ||||||
| 
 | 
 | ||||||
|  | @ -87,40 +74,39 @@ class BotTestCase(TestCase): | ||||||
|         else: |         else: | ||||||
|             instance.send_reply.assert_called_with(message, response['content']) |             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() |         message_handler = self.get_bot_message_handler() | ||||||
|         # Mocking BotHandlerApi |         # Mocking BotHandlerApi | ||||||
|         with patch('bots_api.bot_lib.BotHandlerApi') as MockClass: |         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): |     def bot_to_run(self, bot_module): | ||||||
|         # Returning Any, same argument as in get_bot_message_handler function. |         # Returning Any, same argument as in get_bot_message_handler function. | ||||||
|  |  | ||||||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue
	
	 derAnfaenger
						derAnfaenger