Skip to content

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

Merged
vstinner merged 1 commit intopython:mainfrom
taleinat:bpo-44282/fix-test_incremental_editing
Jun 2, 2021
Merged

bpo-44282: fix occasional test_incremental_editing failures on buildbots#26491
vstinner merged 1 commit intopython:mainfrom
taleinat:bpo-44282/fix-test_incremental_editing

Conversation

@taleinat
Copy link
Contributor

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

@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

bedevere-bot commented Jun 2, 2021

🤖 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
self._assert_highlighting(source, {'STRING': [('1.0', '5.4')]})

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

@vstinner vstinner Jun 2, 2021

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

@taleinat taleinat Jun 2, 2021

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

@taleinat taleinat Jun 2, 2021

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

@vstinner vstinner Jun 2, 2021

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

miss-islington commented Jun 2, 2021

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 <[email protected]>
(cherry picked from commit adef445)

Co-authored-by: Tal Einat <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 2, 2021
@bedevere-bot
Copy link

bedevere-bot commented Jun 2, 2021

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

terryjreedy commented Jun 3, 2021

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 <[email protected]>
(cherry picked from commit adef445)

Co-authored-by: Tal Einat <[email protected]>
@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