tests: boxes: Add SAVE_AS_DRAFT keypress test for write box.
#950
Conversation
tests/ui_tools/test_boxes.py
Outdated
| if stream_message: | ||
| write_box.stream_id = 1 | ||
| draft = { | ||
| 'type': 'stream', | ||
| 'to': 'Current Stream', | ||
| 'content': 'Random message', | ||
| 'subject': 'Topic' | ||
| } | ||
| else: | ||
| write_box.to_write_box = mocker.Mock() | ||
| draft = { | ||
| 'type': 'private', | ||
| 'to': ['[email protected]'], | ||
| 'content': 'Random message' | ||
| } |
There was a problem hiding this comment.
Maybe a parametrize could be replaced with this conditional check?
There was a problem hiding this comment.
Thanks for the feedback @Ezio-Sarthak . I have parametrized the draft_message now.
There was a problem hiding this comment.
@mkp6781 I support @Ezio-Sarthak 's feedback, but other than my point below this looks close to ready 👍
Note that the 'fixes' text will close the issue, so you'll need to rephrase it slightly or just remove it.
tests/ui_tools/test_boxes.py
Outdated
| @pytest.mark.parametrize('draft_message_in_current_session', [ | ||
| True, None, |
There was a problem hiding this comment.
You're using a descriptive parameter as a setting here, which is a little confusing.
neiljp
added
the
PR awaiting update
mkp6781
force-pushed
the
test_keypress_for_write_box
branch
from
4004b4b to
b3a75dd
Compare
zulipbot
added
PR needs review
There was a problem hiding this comment.
Thanks for noticing the missing tests. This should be almost ready to merge. 👍
You still haven't addressed Neil's concern about removing/rephrasing the fixes text, and the parametrize is not used properly as we'd expect. You can take a look at other similar tests to figure out how parametrize works, or you can consult the pytest docs. :)
tests/ui_tools/test_boxes.py
Outdated
| { | ||
| 'type': 'private', | ||
| 'to': ['[email protected]'], | ||
| 'content': 'Random message' | ||
| }, |
There was a problem hiding this comment.
Maybe add one test fixture for PM huddle too?
There was a problem hiding this comment.
We have only two data structures PrivateComposition and StreamComposition. Testing already covers both these cases. For PM huddle, its just a case of appending more emails to the existing list. Is there a need for a separate parameter?
tests/ui_tools/test_boxes.py
Outdated
| write_box.stream_write_box = mocker.Mock(edit_text="Current Stream") | ||
| write_box.title_write_box = mocker.Mock(edit_text="Topic") | ||
| write_box.msg_write_box = mocker.Mock(edit_text="Random message") | ||
| write_box.recipient_emails = ['[email protected]'] |
There was a problem hiding this comment.
Looks like you aren't using the data from test fixture, and still providing a hard-coded value matching with the fixtures - that's why the tests aren't failing. This shouldn't happen; otherwise, there's no reason to use parametrize, right :)
Plus, the above three write_box objects are only for stream message and the last one for PM's, so probably better to have separate cases; but it's minor.
There was a problem hiding this comment.
Looks like you aren't using the data from test fixture, and still providing a hard-coded value matching with the fixtures - that's why the tests aren't failing. This shouldn't happen; otherwise, there's no reason to use parametrize, right
Yeah my bad. :)
tests/ui_tools/test_boxes.py
Outdated
| if draft_composition['type'] == "stream": | ||
| write_box.stream_id = 1 | ||
| else: | ||
| write_box.to_write_box = mocker.Mock() |
There was a problem hiding this comment.
The second point in the above review could be shifted here.
mkp6781
force-pushed
the
test_keypress_for_write_box
branch
from
b3a75dd to
884f260
Compare
zulipbot
added
size: L
mkp6781
force-pushed
the
test_keypress_for_write_box
branch
from
884f260 to
960469a
Compare
zulipbot
added
size: M
mkp6781
force-pushed
the
test_keypress_for_write_box
branch
from
960469a to
bd6ee16
Compare
mkp6781
force-pushed
the
test_keypress_for_write_box
branch
from
bd6ee16 to
fa8c70a
Compare
zulipbot
added
size: L
|
I have rebased this branch. Used Black to reformat the code so that it passes linting tests. |
|
Heads up @mkp6781, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
Marking this as a completion candidate, but the underlying code has changed somewhat, so in addition to a rebase and improving the tests, this will need some adjustment to fit the new code - this will require digging for eg. renames and similar. |
neiljp
added
PR completion candidate
|
I'd like to work on this. |
No description provided.