bpo-35771: IDLE: Fix and optimize flaky tool-tip hover delay tests#15634
bpo-35771: IDLE: Fix and optimize flaky tool-tip hover delay tests#15634terryjreedy merged 6 commits intopython:masterpython/cpython:masterfrom taleinat:bpo-35771/idle-tooltip-hover-delay-teststaleinat/cpython:bpo-35771/idle-tooltip-hover-delay-testsCopy head branch name to clipboard
Conversation
This is done to avoid waiting for a delay to expire multiple times.
|
@ZackerySpytz, your review would be welcome. @terryjreedy, this is not the approach you recommended in your comment on GH-14926. IMO, in this case, it is better to actually test with Tk rather than with a mock. |
terryjreedy
left a comment
There was a problem hiding this comment.
An average speedup of .004 seconds and greatly decreased chance of spurious failure is a win. I will merge with my revisions when CI passes.
| @@ -1,27 +1,31 @@ | ||
| from functools import wraps |
There was a problem hiding this comment.
This first few changes are irrelevant to the issue and are a regression from the 'fail fast (as soon as possible)' design used in IDLE tests. If importing the tested module fails, doing anything else is useless. If the whole module requires gui and that fails (as is does for a majority of buildbots) doing anything else is useless. I have changed back to this.
I am also adding module docstring with coverage.
| global root | ||
| root.update() | ||
|
|
||
|
|
There was a problem hiding this comment.
idlelib is not consitently double spaced around functions. To me, it is less needed than for classes. And PEP8 has this, which I think applies here: "Blank lines may be omitted between a bunch of related one-liners".
That aside, root_update just adds call overhead. I am replacing it.
| self.top, self.button = _make_top_and_button(self) | ||
|
|
||
| def is_tipwindow_shown(self, tooltip): | ||
| return tooltip.tipwindow and tooltip.tipwindow.winfo_viewable() |
There was a problem hiding this comment.
I might have used an even shorten name ('tip_shown'), but even as is, this is a definite improvement in readability.
|
|
||
| def test_dont_show_on_mouse_leave_before_delay(self): | ||
| tooltip = Hovertip(self.button, 'ToolTip text', hover_delay=50) | ||
| time.sleep(0.15) |
There was a problem hiding this comment.
One sleep of .15 (to Windows accuracy of around .017) is faster than 2 sleeps of .10 (ditto).
| @@ -0,0 +1,2 @@ | ||
| Increase the ``hover_delay`` in IDLE's test_tooltip, and run both tests | ||
| waiting for a delay to expire simultaneously, to reduce total run time. |
There was a problem hiding this comment.
I changed this to what users are more likely to want to know. "To avoid occasional spurious test_idle failures on slower machines, increase the hover_delay in test_tooltip."
|
Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
|
Sorry @taleinat and @terryjreedy, I had trouble checking out the |
|
GH-15657 is a backport of this pull request to the 3.7 branch. |
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup. (cherry picked from commit 132acab) Co-authored-by: Tal Einat <taleinat+github@gmail.com>
|
Thanks @taleinat for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
GH-15658 is a backport of this pull request to the 3.8 branch. |
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup. (cherry picked from commit 132acab) Co-authored-by: Tal Einat <taleinat+github@gmail.com>
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup. (cherry picked from commit 132acab) Co-authored-by: Tal Einat <taleinat+github@gmail.com>
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup. (cherry picked from commit 132acab) Co-authored-by: Tal Einat <taleinat+github@gmail.com>
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup.
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup.
Extending the hover delay in test_tooltip should avoid spurious test_idle failures. One longer delay instead of two shorter delays results in a net speedup.
A 50ms delay will often causes failures on slower machines.
This PR is an alternative to PR GH-14926. In this PR, both of the tests with a delay have been merged into one, to avoid multiple
time.sleep()calls, reducing the overall run time.For reference, the total run time for
test_idleon my machine is about 7.5 seconds. 0.15 seconds, which is the delay time added by these tests, is 2% of that.https://bugs.python.org/issue35771