messages: Add tests for transform_content. #1507

rsashank · 2024-05-29T14:58:21Z

What does this PR do, and why?

This update PR tests for the transform_content class method for self.message_links and self.time_mentions. The addition of these tests addresses feedback from issue #1455, which pointed out the lack of tests for these aspects.

@neiljp said:

It'd be good to see some tests for what ends up in self.code_snippets, though we don't seem to have tests for the others.

External discussion & connections

How did you test this?

Manually - Behavioral changes
Manually - Visual changes
Adapting existing automated tests
Adding automated tests for new behavior (or missing tests)
Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

It is a minimal coherent idea
It has a commit summary following the documented style (title & body)
It has a commit summary describing the motivation and reasoning for the change
It individually passes linting and tests
[] It contains test additions for any new behavior
It flows clearly from a previous branch commit, and/or prepares for the next commit

Niloth-p

Same as the other PR,

type annotations
test ids

Nice work with the noqa exceptions 😉

Nicely done!
I haven't looked into the scope and history behind this PR yet.
But the implementation lgtm.

tests/ui_tools/test_messages.py

Niloth-p · 2024-05-31T10:23:18Z

tests/ui_tools/test_messages.py

+        [
+            (
+                """
+                <p><a href="https://github.com/zulip/zulip-terminal/pull/1">https://github.com/zulip/zulip-terminal/pull/1</a></p>


Should we have some link where the text is not the same as the link?
One of the first two?

tests/ui_tools/test_messages.py

rsashank · 2024-05-31T17:48:56Z

Thanks for helping out with the noqa exceptions and for the detailed review! :) @Niloth-p

mounilKshah

Thanks for working on this @rsashank
The test cases considered for each of the functions seem appropriate and enough for testing the respective aspects of the transform_content function.

mounilKshah · 2024-06-16T03:55:08Z

tests/ui_tools/test_messages.py

@@ -1585,14 +1587,120 @@ def test_keypress_EDIT_MESSAGE(
            # fmt: on
        ],
    )
-    def test_transform_content(self, mocker, raw_html, expected_content):
+    def test_transform_content(self, raw_html: str, expected_content: Any) -> None:


I am not sure if there's a need for this change as we don't usually define data type for the function arguments or the function return variable as we decide what to pass.
You may not keep them in the newly created test function as well.

This file is not currently type-checked, though it would be good to do so (there's an issue for it). Adding these right now are more like comments, which is fine but won't be tested or maintained, which is the downside - though if they keep passing manual review then they should help when we do apply mypy to these files.

Given the introduction of the similar transform_content tests, it would make sense to rename this test to define what set of cases it handles, so I'd consider it reasonable to update nearby lines like the types.

neiljp

@rsashank This looks like an improvement to me 👍

In addition to my points inline, I'd consider the case of no links/mentions, and multiple ones.

Did you see an improvement in numeric coverage compared to this rebased against main?

Historically we tested transform_content separately since it became a wrapper around soup2markup for quotes. These tests could be considered more complex tests for soup2markup, but transform_content seems fine too - the current soup2markup test is a simple rendering test only. One readability confusion in that regard is that all these tests are not near the soup2markup test (L676).

This PR has an interesting connection to the muted user work; there I mentioned that we likely need to parse the message to get these fields in any case, which could suggest we do the transform completely separately (earlier) than the header calculation. We currently remake the entire messagebox, I recall, but storing the content earlier may make updating only the content/author easier for muting/unmuting events.

neiljp · 2024-09-23T20:00:49Z

tests/ui_tools/test_messages.py

@@ -1585,14 +1587,120 @@ def test_keypress_EDIT_MESSAGE(
            # fmt: on
        ],
    )
-    def test_transform_content(self, mocker, raw_html, expected_content):
+    def test_transform_content(self, raw_html: str, expected_content: Any) -> None:


This file is not currently type-checked, though it would be good to do so (there's an issue for it). Adding these right now are more like comments, which is fine but won't be tested or maintained, which is the downside - though if they keep passing manual review then they should help when we do apply mypy to these files.

Given the introduction of the similar transform_content tests, it would make sense to rename this test to define what set of cases it handles, so I'd consider it reasonable to update nearby lines like the types.

neiljp · 2024-09-23T20:20:48Z

tests/ui_tools/test_messages.py

+        ids=[
+            "github_pr",
+            "external_link_no_footlink",
+            "internal_link",
+            "github_link_with_text",
+        ],


Let's have these ids inline.

Also the first is a github_pr (link) but it'd be useful to clarify what form it takes, ie. the link matches the text in that first case.

github_pr_link_matches_text?

neiljp · 2024-09-23T20:22:36Z

tests/ui_tools/test_messages.py

+            ),
+            (
+                """
+                <p><a href="#narrow/stream/206-zulip-terminal/topic/announce">https://chat.zulip.zulip/#narrow/stream/206-zulip-terminal/topic/announce</a></p>


Are these test cases all from real example messages, to indicate this is what we actually receive?

Yes, I printed in debug mode and got them.

neiljp · 2024-09-23T20:25:36Z

tests/ui_tools/test_messages.py

+                ],
+            ),
+        ],
+        ids=["test_date", "distant_future_date", "past_date"],


I'd suggest inlining these too.

This commit introduces tests for the `transform_content` class method, specifically focusing on the `message_links` and `time_mentions`. These enhancements improve the reliability and testing coverage of the `transform_content` method.

rsashank · 2024-09-30T13:44:59Z

Updated this PR @neiljp! Thanks for the feedback

zulipbot added the size: L [Automatic label added by zulipbot] label May 29, 2024

rsashank added area: tests PR needs buddy review PR needs review PR requires feedback to proceed PR needs mentor review labels May 29, 2024

rsashank force-pushed the transform_content branch from 223c99d to 9d51e8e Compare May 31, 2024 08:13

Niloth-p reviewed May 31, 2024

View reviewed changes

Niloth-p removed the PR needs buddy review label May 31, 2024

rsashank force-pushed the transform_content branch from 9d51e8e to 44fd0b4 Compare May 31, 2024 12:04

zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels May 31, 2024

rsashank force-pushed the transform_content branch from 44fd0b4 to 6db6378 Compare May 31, 2024 12:11

mounilKshah reviewed Jun 16, 2024

View reviewed changes

mounilKshah added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs mentor review labels Jun 16, 2024

rsashank removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jun 26, 2024

neiljp reviewed Sep 23, 2024

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 Sep 23, 2024

rsashank force-pushed the transform_content branch 2 times, most recently from 4d8ae77 to 9b09c07 Compare September 29, 2024 17:56

messages: Add tests for transform_content.

290ae47

This commit introduces tests for the `transform_content` class method, specifically focusing on the `message_links` and `time_mentions`. These enhancements improve the reliability and testing coverage of the `transform_content` method.

rsashank force-pushed the transform_content branch from 9b09c07 to 290ae47 Compare September 30, 2024 13:43

rsashank 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 Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

messages: Add tests for transform_content. #1507

messages: Add tests for transform_content. #1507

rsashank commented May 29, 2024

Niloth-p left a comment

Niloth-p May 31, 2024

rsashank May 31, 2024

rsashank commented May 31, 2024

mounilKshah left a comment

mounilKshah Jun 16, 2024

neiljp Sep 23, 2024

neiljp left a comment

neiljp Sep 23, 2024

neiljp Sep 23, 2024

rsashank Sep 30, 2024

neiljp Sep 23, 2024

rsashank Sep 30, 2024

neiljp Sep 23, 2024

rsashank commented Sep 30, 2024

messages: Add tests for transform_content. #1507

Are you sure you want to change the base?

messages: Add tests for transform_content. #1507

Conversation

rsashank commented May 29, 2024

What does this PR do, and why?

External discussion & connections

How did you test this?

Self-review checklist for each commit

Niloth-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsashank commented May 31, 2024

mounilKshah 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

rsashank commented Sep 30, 2024