Multiple footer reset bug fix #725
Conversation
zulipbot
added
size: M
kaustubh-nair
force-pushed
the
rapid-footer-updates
branch
2 times, most recently
from
bbdfd8a to
3cc2b1c
Compare
There was a problem hiding this comment.
@kaustubh-nair Left a couple of comments, but broadly looks good!
tests/ui/test_ui.py
Outdated
| view.controller.update_screen.assert_called_once_with() | ||
| view._reset_footer_text.assert_called_once_with(duration) | ||
|
|
||
| def _reset_footer_text(self, view, mocker, duration=46.2): |
There was a problem hiding this comment.
Since it is a test, I think ti should start with test_*. Added I think pytest won't run this unless it starts with test__reset_footer_......
Do you want to remove the two blank lines below (81, 83)? It feels a little odd. And if we can save some space and not decrease readability, we should go for it.
There was a problem hiding this comment.
Oops, good catch - the tests weren't running.
@neiljp Suggests having blank lines surrounding the method under test 😅
There was a problem hiding this comment.
The blank lines clarify the arrange, act and assert segments. Personally, I like having them. 😉
There was a problem hiding this comment.
While this does add extra spacing, this is a style I've been moving towards for the reasons that @preetmishra mentioned.
That said, if a test needs zero setup, I might be tempted to avoid the one blank line, but consistency would still be good and we don't currently have many of those - perhaps unfortunately.
There was a problem hiding this comment.
Sure looks like a cleaner style. 👍
tests/ui/test_ui.py
Outdated
| @@ -84,6 +86,15 @@ def _reset_footer_text(self, view, mocker, duration=46.2): | |||
| mock_sleep.assert_called_once_with(duration) | |||
| view.set_footer_text.assert_called_once_with() | |||
|
|
|||
| def _reset_footer_text_multiple_resets(self, view, mocker, duration=1): | |||
There was a problem hiding this comment.
Same comment as previous, it should begin with test__.....
kaustubh-nair
force-pushed
the
rapid-footer-updates
branch
from
3cc2b1c to
0cf71c5
Compare
zulipbot
added
size: L
| executor.submit(view._reset_footer_text, duration=0.5) | ||
|
|
||
| view.set_footer_text.assert_called_once_with() | ||
| assert time.sleep.call_count == 3 |
There was a problem hiding this comment.
I tried something different here - this is more of an integration test rather than the unit tests we have everywhere.
There was a problem hiding this comment.
(Also, pytest cannot call methods asynchronously?)
There was a problem hiding this comment.
There may be a plugin for pytest to work with asychronous behavior, but currently we have a simpler solution where our asynch decorator knows whether it's in a test environment.
kaustubh-nair
force-pushed
the
rapid-footer-updates
branch
from
0cf71c5 to
2b3f47b
Compare
There was a problem hiding this comment.
@kaustubh-nair This PR does resolve the situation, based on manual testing 👍
That said, this doesn't use the queue in the way that I'd expected, and will essentially fire up two threads for each footer call with a duration, from what I can tell. My understanding is that a queue is typically used between threads, rather than within one - though as I said, this does work for this problem right now.
One additional behavior we may want to extend this to include, which I'm not sure if I mentioned, is to de-duplicate the calls for the same message, ie. avoid updating the text if we're already in a time period with that message. That's not so obvious an issue to users, but the idea would be that if we're in a 3-second update with message foo, then we don't restart the time period again.
| executor.submit(view._reset_footer_text, duration=0.5) | ||
|
|
||
| view.set_footer_text.assert_called_once_with() | ||
| assert time.sleep.call_count == 3 |
There was a problem hiding this comment.
There may be a plugin for pytest to work with asychronous behavior, but currently we have a simpler solution where our asynch decorator knows whether it's in a test environment.
Move the duration based logic that sets footer to a default value to a new method. Note that this method is private - for other classes that want to set footer to default value should stick to using set_footer_text() for the sake of uniformity. Tests amended.
Currently, if multiple calls are made to set the footer along with a duration to reset it (for example, when tries to edit an uneditable message too often by pressing the key too many times) then it leads to repeated footer updates because all those calls lead to resetting the footer. The goal of this commit is to change that, and reset the footer only once, by ignoring other updates. A queue is maintained containing number of sleep threads currently active, and if there are none left then the footer is reset. This ensures that only the last call to set_footer_text() resets the footer, thus avoiding multiple resets. Also, note that it is not strictly necessary to store the duration in the queue, we could any number we like. However it is kept this way in case we would like to do more with this in the future, say, spacing out tight reset calls with only a small duration between them. Test added. Fixes zulip#647.
kaustubh-nair
force-pushed
the
rapid-footer-updates
branch
from
2b3f47b to
113c495
Compare
|
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 |
|
@kaustubh-nair I'm closing this in favor of your later #748 (thanks for both during GSoC!). That is a simpler solution, though doesn't avoid threads per call, and I suspect we may need an improvement later to more cleanly handle 'stacking' footer-text-updates. |
This PR introduces a queue to keep track of the currently active sleep threads, and resets the footer only if the queue is empty.
Fixes #647
(Alternate fix: #748)