zulip-terminal
zulip-terminal copied to clipboard
zulip
Improve tests for Model._handle_message_event
@kaustubh-nair Thanks for working on this 👍 . I tried some manual testing and the results are promising.
One follow-up point worth mentioning here (in case you or someone else wants to pick it) is to try and bridge the 'holes' in index forming which this might trigger. For instance, the new logic will solve the negative unread_count issues we have been seeing, but suppose we index a non-current message which is received and then the user scrolls down (keypress down) causing new message to be fetched, how will the index handle this? Will the re-index cause any problem since the message may potentially exist already in the index?
This don't need to be solved right away, but it's worth keeping in mind of the impact the PR may have on index related items.
@kaustubh-nair I just pushed the code part of this PR, since this is an important fix that I don't want to leave unmerged for too long - both because it improves the overall experience, and since it may expose other bugs.
Let's discuss whether you want to leave this PR open to improve the tests for this method, or whether to add this as another issue so we don't forget it.
@neiljp Pushing some commits for refactoring tests now.
PS. If you think the current commits don't require any more changes, you can merge them. I'll add commits on top.
Edit: Oh, you did push the commit didn't see that. Okay, I'll add the tests then.
The main commit here was merged, so renamed the PR. Does need a rebase though.
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/main branch and resolve your pull request's merge conflicts accordingly.
