Skip to content

WIP: tests: helper: Add tests for set_count. #548

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

WIP: tests: helper: Add tests for set_count. #548

wants to merge 6 commits into from

Conversation

@kaustubh-nair
Copy link
Member

@kaustubh-nair kaustubh-nair commented Mar 18, 2020 •
edited
Loading

  • Test streams
  • Test pms
  • Test self-pms
  • Test group pms
  • Test messages sent by user ( set_count not called)
  • Test muted streams
  • Test muted topics
  • Test topics whose view is open ( UI test, fix in different PR )
  • Test screen updated
  • Test mentions

@zulipbot zulipbot added the size: L [Automatic label added by zulipbot] label Mar 18, 2020
@kaustubh-nair
Copy link
Member Author

kaustubh-nair commented Mar 18, 2020 •
edited
Loading

This is an important stepping stone to fixing #535.
Handling read events will require modifications to set_count. Since, set_count is not being tested at all, I'm adding tests for the same. It might be difficult to cover every aspect of set_counts, but the goal is to test atleast the major features, so that significant bugs do not arise.

@kaustubh-nair kaustubh-nair force-pushed the test_set_count branch 2 times, most recently from 8454e32 to 812a667 Compare March 18, 2020 09:58
@kaustubh-nair kaustubh-nair changed the title tests: helper: Add test for set_count. tests: helper: Add tests for set_count. Mar 18, 2020
@kaustubh-nair kaustubh-nair changed the title tests: helper: Add tests for set_count. WIP: tests: helper: Add tests for set_count. Mar 18, 2020
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 18, 2020
@kaustubh-nair
Copy link
Member Author

kaustubh-nair commented Mar 18, 2020

There's will be a little bit of duplication across the tests for setting up and initializing variables. I'm wondering if it would be viable to abstract out the setup, or would it make the tests too complex?

kaustubh-nair
kaustubh-nair commented Mar 18, 2020
View reviewed changes
tests/helper/test_helper.py Outdated
(-1, [15], 4, 3, {200: 2, 201: 1}), # self-pm
(1, [16, 10], 6, 5, {200: 4, 201: 1}),
(1, [], 4, 3, {200: 2, 201: 1}),
# (1, [15], 4, 3, {200: 2, 201: 1}), # self-pm # FIXME
Copy link
Member Author

@kaustubh-nair kaustubh-nair Mar 18, 2020

Choose a reason for hiding this comment

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

The test fails when a pm sent by the user is to be marked as unread. I don't know if such a case will ever arise, but set_count should probably handle it anyway.

Copy link
Member

@sumanthvrao sumanthvrao Mar 23, 2020

Choose a reason for hiding this comment

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

I vaguely remember discovering the same fact a while ago. I think the reason I concluded was that the server does not set the unread for self PM's hence there is no point setting the counts from ZT side

Copy link
Member Author

@kaustubh-nair kaustubh-nair Mar 26, 2020

Choose a reason for hiding this comment

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

Okay, I'll remove this then

@neiljp neiljp added the PR needs review PR requires feedback to proceed label Mar 19, 2020
@kaustubh-nair
Copy link
Member Author

kaustubh-nair commented Mar 22, 2020

@neiljp @sumanthvrao, if you have time to give this a review please let me know if these tests look okay. :)
And also the extent to which I should be testing set_count, so I can go ahead with the rest of the tests?

@sumanthvrao sumanthvrao mentioned this pull request Mar 24, 2020
Merged
@sumanthvrao
Copy link
Member

sumanthvrao commented Mar 25, 2020

This looks like a great start @kaustubh-nair . If possible we can try reusing some fixtures in conftest.py which may help us avoid the duplication (not entirely I suppose).

The list you have mentioned above seems comprehensive enough! Let's go ahead with that.

@kaustubh-nair
Copy link
Member Author

kaustubh-nair commented Mar 25, 2020

The fixtures in conftest.py don't contain enough messages to properly test the counts, so I ended up adding new ones

@kaustubh-nair kaustubh-nair force-pushed the test_set_count branch 2 times, most recently from 08d2a32 to 3b3ba04 Compare March 28, 2020 14:40
@kaustubh-nair kaustubh-nair mentioned this pull request Mar 28, 2020
Open
@kaustubh-nair
Copy link
Member Author

kaustubh-nair commented Mar 30, 2020

@sumanthvrao @neiljp This is ready for an initial review. I do realize there is a LOT of duplication going on here. I will push some refactor commits sometime later tomorrow.

@kaustubh-nair kaustubh-nair force-pushed the test_set_count branch 2 times, most recently from 413acd3 to 686f563 Compare April 12, 2020 14:39
@kaustubh-nair
tests: helper: Test set_count for stream messages.
d448374 
Add test for verifying unread_counts when marking streams as
read/unread.

Added new index fixture that contains a large number of messages.
@kaustubh-nair
tests: helper: Test set_count for private messages.
76fec18 
Add test for verifying unread counts when marking self-pm and one-on-one
pm as read/unread.

Add new pm entries to 'index_multiple_messages' fixture.
@kaustubh-nair
tests: helper: Test set_count for group private messages.
5ff614c 
Add test for verifying unread counts when marking group-pm
as read/unread.

Add new pm entries to 'index_multiple_messages' fixture.
@kaustubh-nair
tests: helper: Test set_count for muted streams.
53961f9 
Add test for verifying unread_counts when marking muted streams
read/unread. The 'all_msg' attribute in unread counts does not change
while changing the read status of such messages.
@kaustubh-nair
tests: helper: Test set_count for muted topics.
07b1709 
Add test for verifying unread_counts when marking muted topics
read/unread. The 'all_msg' attribute in unread counts does not change
while changing the read status of such messages.
@kaustubh-nair
tests: helper: Test screen updated on setting unread counts.
f736248 
Ensure controller.update_screen is called while setting unread counts.
@zulipbot
Copy link
Member

zulipbot commented May 13, 2020

Heads up @kaustubh-nair, 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 upstream/master branch and resolve your pull request's merge conflicts accordingly.

Base automatically changed from master to main January 30, 2021 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@sumanthvrao sumanthvrao sumanthvrao left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

has conflicts PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kaustubh-nair @sumanthvrao @zulipbot @neiljp