refactor: Simplify msg template generation in conftest using factory fixtures. #1022

zee-bit · 2021-05-04T15:09:16Z

This is a prep commit for #967, that simplifies the message template generation logic in conftest. Using factory fixtures to generate templates for all messages(stream/PM/group), rather than separately defining for each type allows for code reusability and prevents duplication.

neiljp

@zee-bit Thanks for looking at these - this has the potential for simplifying things greatly 👍

In general we can avoid adding extra fixtures when functions should do fine, so for example I can't see a need for the last commit - unless that's something you intend to use directly in another commit/PR?

You've added some typing here - we don't actually type-check in tests right now, so that could be a good extra piece of work to follow-through with later!

Since the tests aren't type-checked yet it wouldn't affect this area straight away, but we should move towards improving the Message type which the generated data should later match - eg. a base typeddict and derived ones for streams and pms. We can then avoid the total=False case.

tests/conftest.py

neiljp · 2021-06-10T00:49:47Z

tests/conftest.py

+            "sender_id": 5140,
+            "content_type": "text/x-markdown",
+            "stream_id": stream_id,
+            "subject": "Test" if stream_id else "",


This seems something we might want customized in the stream message fixture instead?

I have fixed this, as per my below comment. Sorry for misunderstanding you here.

tests/conftest.py

neiljp

@zee-bit Just a few points I noticed, some from miscommunication before possibly. Otherwise should be good to go 👍

neiljp · 2021-06-22T01:45:10Z

tests/conftest.py

+        "sender_id": 5140,
+        "content_type": "text/x-markdown",
+        "stream_id": stream_id,
+        "subject": "",


This used to vary between stream/private. I see you override it later, but this should be a required factory parameter if it's a stream?

(I can see my earlier comment may have caused this change, but I was referring to (likely later) allowing the subject to be customized each time the stream message factory is used.)

I added a keyword arg subject with the default value of "" (for PMs/group)

neiljp · 2021-06-22T01:45:28Z

tests/conftest.py

+        "subject_links": [],
+        "avatar_url": "dummy_avatar_url",
+        "is_me_message": False,
+        "content": "Stream content here.",


This used to vary between stream/private. You could adjust it to generate the previous content internally, or make it a required named parameter?

I have customized the content internally, depending on msg_type. I don't see the need for having an extra parameter for this field, especially since we don't test on this and probably we shouldn't?

I was initially concerned that all the versions would imply a stream message based on the content in your previous version, which you've resolved 👍

We don't need precise control to support the refactor you've done here, but if we reuse these fixtures later rather than setting the content directly we might provide support in the factory "initializer".

neiljp · 2021-06-22T01:57:12Z

tests/conftest.py

+    stream_id: Optional[int] = None,
+    recipients: Union[str, List[Dict[str, Any]]] = "PTEST",


Based on the type we should assert on these to ensure they're used correctly, based on how you have this currently. An alternative could be to encode the extra data as part of the message type parameter (eg. one of two different tuple types), but that'll be less descriptive.

(We may end up going with a simpler approach with two different factories for private and stream messages. However, this is definitely an improvement over the previous approach, but needing these asserts for best behavior suggests there may be a simpler way to take later.)

I agree, asserting to ensure these values are used correctly is important, but I'm a bit unconvinced by the encoded msg_type approach. We could take the different factories approach, but that would not be much of a "re-useable" refactor anymore?

The encoded msg_type approach basically is like having two separate initializers/factories, with the type itself being the default for each of the two initializers, and the different function signature (set of parameters) being the set of values.
eg. ("private", ([])) and ("stream", (stream_id, stream_name, subject))
could give private_factory([recipients]) and stream_factory(stream_id, stream_name, subject)
(note that we can have common code for the core similar data generation which these could delegate to)

This commit adds a display_recipient_factory() in conftest that can generate the "display_recipients" field in msg_template for message belonging to either PM/group. This function requires a list of tuples containing the recipient_id and recipient_name for generating the fields. This function is not defined as a fixture since we currently only need this to create other fixtures and not directly in tests.

This commit adds a new function msg_template_factory that can generate templates for all message types, i.e. stream/PM/group with minimum input parameters. This new factory simplifies the code and reduces duplication in tests. All the different message template functions are defined as fixtures so that they can be reused throughout the tests. This new factory also makes it easy to create a large number of output templates with minimal modification of the input.

neiljp · 2021-06-22T22:47:14Z

@zee-bit Thanks for the refactor 👍 This makes our code more compact, readable and hopefully will be more reusable in future - merging now! 🎉

This leaves a lot less in #967 along with #1053 👍

zulipbot · 2022-02-28T04:07:23Z

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

zulipbot added the size: XL [Automatic label added by zulipbot] label May 4, 2021

zee-bit force-pushed the simplify-msg-template-conftest branch from 7c698a5 to 9d825c4 Compare May 6, 2021 15:34

zee-bit mentioned this pull request May 21, 2021

bugfix: core: Fix cases where message was not focused on narrowing with anchor. #967

Merged

zee-bit added the PR needs review PR requires feedback to proceed label May 29, 2021

zee-bit force-pushed the simplify-msg-template-conftest branch 2 times, most recently from 1d662b9 to 323fb28 Compare June 2, 2021 16:57

neiljp reviewed Jun 10, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback PR blocks other PR and removed PR needs review PR requires feedback to proceed labels Jun 10, 2021

zee-bit force-pushed the simplify-msg-template-conftest branch 5 times, most recently from 9e9277e to 79dd4ad Compare June 21, 2021 21:02

zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 21, 2021

neiljp reviewed Jun 22, 2021

View reviewed changes

neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jun 22, 2021

zee-bit force-pushed the simplify-msg-template-conftest branch 2 times, most recently from d805cd3 to a9cd8ca Compare June 22, 2021 06:16

zee-bit force-pushed the simplify-msg-template-conftest branch from a9cd8ca to 37248c5 Compare June 22, 2021 14:20

zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 22, 2021

zee-bit force-pushed the simplify-msg-template-conftest branch from 37248c5 to 34c6325 Compare June 22, 2021 22:17

neiljp merged commit e2dc2e4 into zulip:main Jun 22, 2021

neiljp added this to the Next Release milestone Jun 22, 2021

neiljp removed the PR needs review PR requires feedback to proceed label Jun 22, 2021

neiljp added area: refactoring area: tests labels Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Simplify msg template generation in conftest using factory fixtures. #1022

refactor: Simplify msg template generation in conftest using factory fixtures. #1022

zee-bit commented May 4, 2021

neiljp left a comment

neiljp Jun 10, 2021

zee-bit Jun 22, 2021

neiljp left a comment

neiljp Jun 22, 2021

zee-bit Jun 22, 2021

neiljp Jun 22, 2021

zee-bit Jun 22, 2021

neiljp Jun 22, 2021

neiljp Jun 22, 2021

zee-bit Jun 22, 2021 •

edited

Loading

neiljp Jun 22, 2021

neiljp commented Jun 22, 2021

zulipbot commented Feb 28, 2022

		stream_id: Optional[int] = None,
		recipients: Union[str, List[Dict[str, Any]]] = "PTEST",

refactor: Simplify msg template generation in conftest using factory fixtures. #1022

refactor: Simplify msg template generation in conftest using factory fixtures. #1022

Conversation

zee-bit commented May 4, 2021

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zee-bit Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp commented Jun 22, 2021

zulipbot commented Feb 28, 2022

zee-bit Jun 22, 2021 •

edited

Loading