Fix flaky dims playback test by waiting for playing condition #5414
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #5414 +/- ##
=======================================
Coverage 89.09% 89.09%
=======================================
Files 597 597
Lines 50612 50614 +2
=======================================
+ Hits 45091 45097 +6
+ Misses 5521 5517 -4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@Czaki : I assigned you this because you worked on similar things recently. |
Czaki
approved these changes
Dec 17, 2022
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
|
Thanks for the review! Will merge this after 24 hours. |
2 tasks
This was referenced Dec 22, 2022
melissawm
pushed a commit
to melissawm/napari
that referenced
this pull request
Jan 11, 2023
…#5414) Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from napari#5373
|
Assigned the 0.4.18 milestone because although this is mostly about tests and shouldn't matter, it also removes an unnecessary |
Czaki
pushed a commit
that referenced
this pull request
Jun 16, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Czaki
pushed a commit
that referenced
this pull request
Jun 17, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Czaki
pushed a commit
that referenced
this pull request
Jun 18, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Czaki
pushed a commit
that referenced
this pull request
Jun 19, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Czaki
pushed a commit
that referenced
this pull request
Jun 21, 2023
Before this change `test_qt_dims.py::test_play_button` tried to wait for `QtDims._animation_thread.started`, which had a few issues. 1. `waitSignal` returns a `SignalBlocker`, but does not actually wait. Based on [the `pytest-qt` docs](https://pytest-qt.readthedocs.io/en/latest/signals.html#) you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted. 2. The intent is to start waiting after mouse click has occurred, but it's possible (though unlikely) that the signal has already been emitted. 3. `QtDims._animation_thread` is `None` before the button has been clicked. This recently caused [a failure on what should be an unrelated PR](https://github.com/napari/napari/actions/runs/3709270881/jobs/6287719643#step:8:379). I don't know enough about `pytest-qt` to confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it. The fix here is to use `waitUntil` to wait on `QtDims.is_playing` instead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details. I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking. Finally, I removed some other leftovers from #5373
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Before this change
test_qt_dims.py::test_play_buttontried to wait forQtDims._animation_thread.started, which had a few issues.waitSignalreturns aSignalBlocker, but does not actually wait. Based on thepytest-qtdocs you should typically use that as a context manager to wait, where the code with in the context should eventually cause the signal to be emitted.QtDims._animation_threadisNonebefore the button has been clicked.This recently caused a failure on what should be an unrelated PR. I don't know enough about
pytest-qtto confidently explain the failure - but my guess is that the two mouse clicks are being registered as one that starts playback, but does not stop it.The fix here is to use
waitUntilto wait onQtDims.is_playinginstead. This makes the test slightly less inefficient, but also makes it simpler and less dependent on implementation details.I also removed a couple of timeout values because the default is 5000 (milliseconds) anyway - if waiting 5 seconds didn't work, waiting another 2 seconds is almost always just wishful thinking.
Finally, I removed some other leftovers from #5373
Type of change