Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Disable cycling while editing a PM #1280

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[WIP] Disable cycling while editing a PM #1280

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b45eb77
boxes: Create _setup_common_private_compose.
Apr 16, 2023
7cfffe0
tests: boxes: Add tests for _setup_common_private_compose.
Apr 16, 2023
cdb0fad
boxes: Create update_recipients_from_user_ids function.
Apr 16, 2023
415f31f
tests: boxes: Add tests for update_recipients_from_user_ids.
Apr 16, 2023
2a66b5d
tests: boxes: Add tests for private_box_view.
Apr 16, 2023
691296d
boxes: Add private_box_edit_view function for edit compose box.
Apr 16, 2023
f1590cb
tests: boxes: Add tests for private_box_edit_view.
Apr 16, 2023
ba32129
messages: Call private_box_edit_view when editing private message.
Apr 16, 2023
40e6067
boxes: Disable cycling while editing a Private Message.
Apr 25, 2023
ac20ad7
boxes: Footer message if tab key pessed while editing a private message.
Apr 25, 2023
2bd5228
refactor: Change name of update_recipients function.
Subhasish-Behera Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions tests/ui_tools/test_boxes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1706,6 +1706,39 @@ def focus_val(x: str) -> int:
expected_focus_col_name
)

def test_private_box_view(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re this commit in general:

  • This would be good to have right at the start of the refactoring of the DM code in this PR, so when you refactor and move code around, this can act as a guarantee that the function continues to work, as long as the asserts cover the relevant parts of code
  • If you want to list in the commit summary what you test (assert) in the test, I'd suggest using a bulleted list of what you're testing and what it corresponds to - focusing on what features you're ensuring work (though mentioning how you do that is fine to add)

self,
mocker: MockerFixture,
user_dict: List[Dict[str, Any]],
user_id_email_dict: Dict[int, str],
) -> None:
recipient_user_ids = [11]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest placing this as a default argument, but I'm not sure if this has the typical python issue with potential errors from persistent mutable default arguments. Typing it as a Mapping rather than Dict may help.

If we do this, that eases any parametrizing them later - and saves a blank line in this case ;)


write_box = WriteBox(self.view)
write_box.to_write_box = mocker.MagicMock(spec=ReadlineEdit)
enable_autocomplete = mocker.patch.object(ReadlineEdit, "enable_autocomplete")
Comment on lines +1721 to +1722
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_write_box is already a mock, so is the enable_autocomplete line necessary? It seems like it patches one part of the mock, but not to anything in particular, and returns that part. You do test that mocked/patched part, but I'm not sure it would be any different than

enable_autocomplete = write_box.to_write_box.enable_autocomplete

or alternatively substituting that inline where you have the assert_has_calls, since you only use/assert enable_autocomplete once in the test.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems rather that the previous line isn't necessary, as the write_box.to_write_box will be completely replaced by a new ReadlineEdit in this line.

So we need to patch ReadlineEdit here, but not patch to_write_box initially.

write_box.model.user_id_email_dict = user_id_email_dict
write_box.model.user_dict = user_dict
connect_signal_mock = mocker.patch.object(urwid, "connect_signal")
write_box.private_box_view(recipient_user_ids=recipient_user_ids)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Unless the test is only a few lines, please separate the line being tested from the setup and assert lines by blank lines, to make it easier to pick out which lines are which (you did this in the previous commit!)

enable_autocomplete.assert_has_calls(
[
mock.call(
func=write_box._to_box_autocomplete,
key=primary_key_for_command("AUTOCOMPLETE"),
key_reverse=primary_key_for_command("AUTOCOMPLETE_REVERSE"),
),
mock.call(
func=write_box.generic_autocomplete,
key=primary_key_for_command("AUTOCOMPLETE"),
key_reverse=primary_key_for_command("AUTOCOMPLETE_REVERSE"),
),
]
)
connect_signal_mock.assert_called_once()
assert isinstance(write_box.to_write_box, ReadlineEdit)
assert write_box.to_write_box.text == "To: Human 1 <[email protected]>"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could consider a multi-person DM here, to cover a more complex case, or parametrize this for empty/single/multi. The latter isn't necessary, but would cover all the cases and you might find it useful to flex your pytest muscles - particularly if you extract them into a fixture (see a comment on a later commit)

Note how if you parametrize this, you can consider that as a series of changes:

  • the literal text here can be made a test function parameter (expected_...)
  • the recipient_user_ids (if moved into the argument list, as commented elsewhere) and the new parameter can lose their default values and be assigned by parametrize tuples


def test__setup_common_private_compose(self, mocker: MockerFixture) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate you committing to writing this test, though I'm not entirely sold on the importance of this test, since it's heavily focused on the UI only and risks mimicking the code itself (as per various other UI tests we have).

The DM vs DM-edit tests currently in later commits have more significance due to expressing the differences. Also note that the DM test, which should really be the first commit (see another comment) does already test some of this behavior. It can often be tricky knowing whether to test internal helper functions like this, since we could change the implementation of the DM view to not use this, or use this differently, and then we have a test that's less used/useful, or perhaps needs updating in addition to the test for the other functions, which adds more overhead to what needs doing without necessarily adding a benefit.

For now let's keep this, though note that comments I make regarding other tests also apply here to various degrees.

write_box = WriteBox(self.view)
write_box.to_write_box = mocker.MagicMock()
Expand Down