idlelib tests: Fix refleak testing of test_zzdummy - #155393
Conversation
zware
added
type-bug
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Check test_zzdummy_user.py -- it can have the same leak.
| usercfg = zzdummy.idleConf.userCfg | ||
| testcfg = { | ||
|
|
||
| make_testcfg = lambda: { |
There was a problem hiding this comment.
Nit: define a function instead of lambda.
def make_testcfg():| text = cls.text = Text(cls.root) | ||
| cls.editor = DummyEditwin(root, text) | ||
| zzdummy.idleConf.userCfg = testcfg | ||
| zzdummy.idleConf.userCfg = make_testcfg() |
There was a problem hiding this comment.
Do this in setUp() instead of setUpClass().
|
The changes made a few months ago to test_zzdummy and the creation of a 2nd test file are, I am rather sure, wrong and should be reverted. The new test does not test anything worthwhile, in particular, it does not properly test the new feature it was meant to test. I did not do it yet because a) I discovered this about a month ago when Serhiy contributed about 14 patches that I considered higher priority (along with some other IDLE stuff) to investigate and justify; and b) I was not sure whether any of the rest of the merge needed reversion or modification. So I suggest you find the issue, revert the test changes, and backport, including to 3.15. If you do a clean revert, consider this comment my pre-review and approval. If there is still a leak, meaning that there was one before the change, fix it. And I approve of Serhiy's two suggestions if still relevant after the reversion. I am off to bed now. |
|
When you're done making the requested changes, leave the comment: |
… keybindings from user config (pythonGH-28713)" This reverts part of commit 208195d.
c9c2ba3 to
586fd6c
Compare
|
I have made the requested changes; please review again. In particular, this is now strictly a reversion of the |
|
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
|
!buildbot Windows.*Refleak |
|
🤖 New build scheduled with the buildbot fleet by @zware for commit 586fd6c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F155393%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
Thank you. Approved, including for 3.15 backport. Say so there.
|
I considered merging, but could not find the indicated BB on the linked page and was not sure if you meant it to be checked first. |
|
The buildbot result can be found in the 'Successful checks' section as buildbot/AMD64 Windows Server 2025 Refleaks PR; it passed. The link in the Bedevere message is broken; the next buildbot release should have a fix for that. |
|
Thanks @zware for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
|
Sorry, @zware, I could not cleanly backport this to |
|
GH-155718 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
Sorry, @zware, I could not cleanly backport this to |
|
GH-155720 is a backport of this pull request to the 3.14 branch. |
|
GH-155721 is a backport of this pull request to the 3.13 branch. |
4 participants
Create a fresh test config for each test run of
test_zzdummy.This prevents a first-run failure in
test_zzdummy.ZZDummyTest.test_existsdue to what appears to be config pollution caused bytest_zzdummy.ZZDummyMixin.test_reload(at least, hacking the failing check fromtest_existsintotearDownshowstest_reloadto be the first failure). The failure can be seen on the AMD64 Windows Server 2025 Refleaks builder, but is reproducible on Linux as well. Note that it does not cause the full buildbot run to fail, as the polluting test does not get re-run at the end.This is the smallest change I could make to fix the refleak run, but I doubt it's ideal :). If you'd rather treat this as a bug report against the test and fix it a better way, please go ahead!