Skip to content

bpo-44282: fix occasional test_incremental_editing failures on buildbots #26491

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
Merged
merged 1 commit into from
Jun 2, 2021
Merged

bpo-44282: fix occasional test_incremental_editing failures on buildbots #26491

merged 1 commit into from
Jun 2, 2021

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented Jun 2, 2021 •
edited by bedevere-bot
Loading

@taleinat
bpo-44282: fix occasional test_incremental_editing failures on buildbots
e49b2a5 
Signed-off-by: Tal Einat <532281+taleinat@users.noreply.github.com>
@taleinat taleinat added tests Tests in the Lib/test dir skip news needs backport to 3.10 only security fixes labels Jun 2, 2021
@taleinat taleinat requested a review from terryjreedy as a code owner June 2, 2021 19:45
@taleinat taleinat requested a review from vstinner June 2, 2021 19:46
@taleinat taleinat added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 2, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @taleinat for commit e49b2a5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 2, 2021
vstinner
vstinner reviewed Jun 2, 2021
View reviewed changes
Lib/idlelib/idle_test/test_colorizer.py
@@ -551,7 +551,7 @@ def test_long_multiline_string(self):
''')
self._assert_highlighting(source, {'STRING': [('1.0', '5.4')]})

@run_in_tk_mainloop
@run_in_tk_mainloop(delay=50)
Copy link
Member

Choose a reason for hiding this comment

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

What is this magic number? A number of seconds?

I suggest you to use test.support.SHORT_TIMEOUT or test.support.LONG_TIMEOUT: https://docs.python.org/dev/library/test.html#test.support.SHORT_TIMEOUT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is this magic number? A number of seconds?

Milliseconds. It's explained in the decorator's doc-string.

I suggest you to use test.support.SHORT_TIMEOUT or test.support.LONG_TIMEOUT

I wasn't aware of these, good to know! I don't think they're quite what's needed here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is this magic number? A number of seconds?

Milliseconds. It's explained in the decorator's doc-string.

Would you prefer that this was a datetime.timedelta instance?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's fine.

vstinner
vstinner approved these changes Jun 2, 2021
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Well, I don't understand how Tkinter handles time, so let's see if 50 ms is enough for everyone :-p

terryjreedy
terryjreedy approved these changes Jun 2, 2021
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Exactly what I meant as an alternative to skipping. While the latter would be okay for this test, we may have other tests where a much smaller delay, even 2 or 3 is sufficient.

@vstinner vstinner merged commit adef445 into python:main Jun 2, 2021
@miss-islington
Copy link
Contributor

Thanks @taleinat for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 2, 2021
@taleinat @miss-islington
bpo-44282: Fix occasional test_incremental_editing failures on buildb…
8be62bf 
…ots (pythonGH-26491)

Signed-off-by: Tal Einat <532281+taleinat@users.noreply.github.com>
(cherry picked from commit adef445)

Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 2, 2021
@bedevere-bot
Copy link

GH-26499 is a backport of this pull request to the 3.10 branch.

@vstinner
Copy link
Member

vstinner commented Jun 2, 2021

I merge the issue right now since it broke buildbots and I'm trying to recover the green color of buildbots!

@terryjreedy
Copy link
Member

A note to myself: Backport the revised tkinter helper file to 3.9 so we can use it with future tests that get backported to 3.9.

vstinner pushed a commit that referenced this pull request Jun 3, 2021
@miss-islington @taleinat
bpo-44282: Fix occasional test_incremental_editing failures on buildb…
9c89d62 
…ots (GH-26491) (GH-26499)

Signed-off-by: Tal Einat <532281+taleinat@users.noreply.github.com>
(cherry picked from commit adef445)

Co-authored-by: Tal Einat <532281+taleinat@users.noreply.github.com>
@taleinat
Copy link
Contributor Author

taleinat commented Jun 3, 2021 •
edited
Loading

A note to myself: Backport the revised tkinter helper file to 3.9 so we can use it with future tests that get backported to 3.9.

Note that the helper is currently missing on the 3.9 branch, as it was added as part of the PR which added the shell sidebar, which we haven't backported to 3.9. We'd likely want to backport that PR to 3.9 first.

@pablogsal pablogsal mentioned this pull request Jul 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@terryjreedy terryjreedy terryjreedy approved these changes

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@taleinat @bedevere-bot @miss-islington @vstinner @terryjreedy @the-knights-who-say-ni