From e6974d3013e27aadf472ccb625012a4ac1c4a519 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 16 Jul 2020 15:06:39 +0000 Subject: [PATCH] reaction tests: Clean up optional parameters. This fixes up some complex helpers that may have had some value before f-strings come along, but they mostly obscured the logic for people reading the tests. We still keep really simple helpers for the common cases, but there are no optional parameters for them. One goal of this fix is to remove the short_name concept, and we just explicitly set senders everywhere we need them. We also now have each test just explicitly set its reaction_type. For cases where we have custom message ids or senders, we just inline the simple call to api_post. --- zerver/tests/test_reactions.py | 123 +++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 50 deletions(-) diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index ff349a3fcc..b528047fea 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -9,7 +9,7 @@ from zerver.lib.emoji import emoji_name_to_emoji_code from zerver.lib.message import extract_message_dict from zerver.lib.request import JsonableError from zerver.lib.test_classes import ZulipTestCase -from zerver.lib.test_helpers import tornado_redirected_to_list +from zerver.lib.test_helpers import tornado_redirected_to_list, zulip_reaction_info from zerver.models import Message, Reaction, RealmEmoji, UserMessage, get_realm @@ -480,44 +480,36 @@ class EmojiReactionBase(ZulipTestCase): changing this: It's used in test_retention.py as well.""" def __init__(self, *args: Any, **kwargs: Any) -> None: - self.reaction_type = 'realm_emoji' super().__init__(*args, **kwargs) - def post_reaction(self, reaction_info: Dict[str, str], message_id: int=1, - sender: str='hamlet') -> HttpResponse: - if 'reaction_type' not in reaction_info: - reaction_info['reaction_type'] = self.reaction_type - sender = self.example_user(sender) - result = self.api_post(sender, f'/api/v1/messages/{message_id}/reactions', - reaction_info) + def post_reaction(self, reaction_info: Dict[str, str]) -> HttpResponse: + message_id = 1 + + result = self.api_post( + self.example_user('hamlet'), + f'/api/v1/messages/{message_id}/reactions', + reaction_info + ) return result - def post_zulip_reaction(self, message_id: int=1, sender: str='hamlet') -> HttpResponse: - reaction_info = { - 'emoji_name': 'zulip', - 'emoji_code': 'zulip', - 'reaction_type': 'zulip_extra_emoji', - } - result = self.post_reaction(reaction_info, message_id, sender) + def post_other_reaction(self, reaction_info: Dict[str, str]) -> HttpResponse: + message_id = 1 + result = self.api_post( + self.example_user('AARON'), + f'/api/v1/messages/{message_id}/reactions', + reaction_info + ) return result - def delete_reaction(self, reaction_info: Dict[str, str], message_id: int=1, - sender: str='hamlet') -> HttpResponse: - if 'reaction_type' not in reaction_info: - reaction_info['reaction_type'] = self.reaction_type - sender = self.example_user(sender) - result = self.api_delete(sender, f'/api/v1/messages/{message_id}/reactions', - reaction_info) - return result + def delete_reaction(self, reaction_info: Dict[str, str]) -> HttpResponse: + message_id = 1 - def delete_zulip_reaction(self, message_id: int=1, sender: str='hamlet') -> HttpResponse: - reaction_info = { - 'emoji_name': 'zulip', - 'emoji_code': 'zulip', - 'reaction_type': 'zulip_extra_emoji', - } - result = self.delete_reaction(reaction_info, message_id, sender) + result = self.api_delete( + self.example_user('hamlet'), + f'/api/v1/messages/{message_id}/reactions', + reaction_info + ) return result def get_message_reactions(self, message_id: int, emoji_code: str, @@ -531,8 +523,8 @@ class EmojiReactionBase(ZulipTestCase): class DefaultEmojiReactionTests(EmojiReactionBase): def setUp(self) -> None: super().setUp() - self.reaction_type = 'unicode_emoji' reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'hamburger', 'emoji_code': '1f354', } @@ -541,6 +533,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_add_default_emoji_reaction(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'thumbs_up', 'emoji_code': '1f44d', } @@ -549,6 +542,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_add_default_emoji_invalid_code(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'hamburger', 'emoji_code': 'TBD', } @@ -557,6 +551,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_add_default_emoji_invalid_name(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'non-existent', 'emoji_code': '1f44d', } @@ -573,10 +568,11 @@ class DefaultEmojiReactionTests(EmojiReactionBase): reaction_type='unicode_emoji', ) reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'smiley', 'emoji_code': '1f603', } - result = self.post_reaction(reaction_info, sender='AARON') + result = self.post_other_reaction(reaction_info) self.assert_json_success(result) reactions = self.get_message_reactions(1, '1f603', 'unicode_emoji') @@ -585,6 +581,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_add_duplicate_reaction(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'non-existent', 'emoji_code': '1f354', } @@ -593,6 +590,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_add_reaction_by_name(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': '+1', } result = self.post_reaction(reaction_info) @@ -609,6 +607,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_preserve_non_canonical_name(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': '+1', 'emoji_code': '1f44d', } @@ -621,6 +620,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_reaction_name_collapse(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': '+1', 'emoji_code': '1f44d', } @@ -628,7 +628,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): self.assert_json_success(result) reaction_info['emoji_name'] = 'thumbs_up' - result = self.post_reaction(reaction_info, sender='AARON') + result = self.post_other_reaction(reaction_info) self.assert_json_success(result) reactions = self.get_message_reactions(1, '1f44d', 'unicode_emoji') @@ -637,6 +637,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_delete_default_emoji_reaction(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'hamburger', 'emoji_code': '1f354', } @@ -651,6 +652,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): def test_delete_non_existing_emoji_reaction(self) -> None: reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'thumbs_up', 'emoji_code': '1f44d', } @@ -668,6 +670,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): ) reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'new_name', 'emoji_code': '1f44f', } @@ -685,6 +688,7 @@ class DefaultEmojiReactionTests(EmojiReactionBase): ) reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': '+1', } result = self.delete_reaction(reaction_info) @@ -713,10 +717,16 @@ class DefaultEmojiReactionTests(EmojiReactionBase): # Have hamlet react to the message reaction_info = { + 'reaction_type': 'unicode_emoji', 'emoji_name': 'hamburger', 'emoji_code': '1f354', } - result = self.post_reaction(reaction_info, message_id=message_id) + + result = self.api_post( + user_profile, + f'/api/v1/messages/{message_id}/reactions', + reaction_info + ) self.assert_json_success(result) # Fetch the now-created UserMessage object to confirm it exists and is historical @@ -727,14 +737,14 @@ class DefaultEmojiReactionTests(EmojiReactionBase): class ZulipExtraEmojiReactionTest(EmojiReactionBase): def test_add_zulip_emoji_reaction(self) -> None: - result = self.post_zulip_reaction() + result = self.post_reaction(zulip_reaction_info()) self.assert_json_success(result) def test_add_duplicate_zulip_reaction(self) -> None: - result = self.post_zulip_reaction() + result = self.post_reaction(zulip_reaction_info()) self.assert_json_success(result) - result = self.post_zulip_reaction() + result = self.post_reaction(zulip_reaction_info()) self.assert_json_error(result, 'Reaction already exists.') def test_add_invalid_extra_emoji(self) -> None: @@ -756,14 +766,14 @@ class ZulipExtraEmojiReactionTest(EmojiReactionBase): self.assert_json_error(result, 'Invalid emoji name.') def test_delete_zulip_emoji(self) -> None: - result = self.post_zulip_reaction() + result = self.post_reaction(zulip_reaction_info()) self.assert_json_success(result) - result = self.delete_zulip_reaction() + result = self.delete_reaction(zulip_reaction_info()) self.assert_json_success(result) def test_delete_non_existent_zulip_reaction(self) -> None: - result = self.delete_zulip_reaction() + result = self.delete_reaction(zulip_reaction_info()) self.assert_json_error(result, "Reaction doesn't exist.") class RealmEmojiReactionTests(EmojiReactionBase): @@ -771,6 +781,7 @@ class RealmEmojiReactionTests(EmojiReactionBase): super().setUp() green_tick_emoji = RealmEmoji.objects.get(name="green_tick") self.default_reaction_info = { + 'reaction_type': 'realm_emoji', 'emoji_name': 'green_tick', 'emoji_code': str(green_tick_emoji.id), } @@ -781,6 +792,7 @@ class RealmEmojiReactionTests(EmojiReactionBase): def test_add_realm_emoji_invalid_code(self) -> None: reaction_info = { + 'reaction_type': 'realm_emoji', 'emoji_name': 'green_tick', 'emoji_code': '9999', } @@ -790,6 +802,7 @@ class RealmEmojiReactionTests(EmojiReactionBase): def test_add_realm_emoji_invalid_name(self) -> None: green_tick_emoji = RealmEmoji.objects.get(name="green_tick") reaction_info = { + 'reaction_type': 'realm_emoji', 'emoji_name': 'bogus_name', 'emoji_code': str(green_tick_emoji.id), } @@ -812,7 +825,7 @@ class RealmEmojiReactionTests(EmojiReactionBase): emoji.deactivated = True emoji.save(update_fields=['deactivated']) - result = self.post_reaction(self.default_reaction_info, sender='AARON') + result = self.post_other_reaction(self.default_reaction_info) self.assert_json_success(result) reactions = self.get_message_reactions(1, @@ -840,6 +853,7 @@ class RealmEmojiReactionTests(EmojiReactionBase): def test_remove_non_existent_realm_emoji_reaction(self) -> None: reaction_info = { + 'reaction_type': 'realm_emoji', 'emoji_name': 'non_existent', 'emoji_code': 'TBD', } @@ -876,10 +890,12 @@ class ReactionAPIEventTest(EmojiReactionBase): } events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): - result = self.post_reaction(reaction_info, - message_id=pm_id, - sender=reaction_sender.short_name) - self.assert_json_success(result) + self.api_post( + reaction_sender, + f'/api/v1/messages/{pm_id}/reactions', + reaction_info + ) + self.assertEqual(len(events), 1) event = events[0]['event'] @@ -911,14 +927,21 @@ class ReactionAPIEventTest(EmojiReactionBase): 'emoji_code': '1f354', 'reaction_type': 'unicode_emoji', } - add = self.post_reaction(reaction_info, message_id=pm_id, sender=reaction_sender.short_name) + add = self.api_post( + reaction_sender, + f'/api/v1/messages/{pm_id}/reactions', + reaction_info, + ) self.assert_json_success(add) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): - result = self.delete_reaction(reaction_info, - message_id=pm_id, - sender=reaction_sender.short_name) + result = self.api_delete( + reaction_sender, + f'/api/v1/messages/{pm_id}/reactions', + reaction_info, + ) + self.assert_json_success(result) self.assertEqual(len(events), 1)