-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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.
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
bpo-35730: Use known good font in IDLE test_sqeezer #11552
Conversation
Tk guarantees that TkFixedFont exists.
I did that to prevent merging the change too early. First we have to make sure that the test pass on the Gentoo buildbot. |
cls.font_family = get_font_family() | ||
|
||
@classmethod | ||
def tearDownClass(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably true -- at the moment. But this will be needed if root is created in setUpClass.
|
||
@classmethod | ||
def setUpClass(cls): | ||
cls.font_family = get_font_family() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also just be a global constant, i.e. set FIXED_WIDTH_FONT_FAMILY = get_font_family()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would have to be wrapped in try...except SkipTest...
to avoid skipping all non-gui tests. As it is, the current line needs the same to avoid skipping non-gui tests within SqueezerTest. But, looking ahead, I want to split SqueezerTest into SqueezerGuiTest and SqueezerNoGuiTest and move root creation into the GUI class setUpClass. (This should also make it faster.)
@terryjreedy, this is a great fix! I just added a few minor comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I agree with @taleinat 's coments
@vstinner Removing the backport label does not prevent merging. Omitting both news and skip news, which I did intentionally, with the intention to add one later, did prevent merging, at least initially. Adding skip news turned the button green and enabled merging ;-). Now, how does one force a particular buildbot to test a particular PR without merging? |
I don't want to block this PR. I want to block a backport to 3.7 until we can double check that this change fix the Gentoo buildbot on master. |
Use https://devguide.python.org/buildbots/#custom-builders to test your PR on all buildbots at once. If you only want to test on one specific buildbot, contact the buildbot worker owner who may or may not answer :-) |
I've triggered a run on all buildbots via EDIT: One of the relevant Gentoo builders had an error running the tests for the custom build:
|
That's annoying. Looking at https://buildbot.python.org/all/#/grid, the only normally green buildbots to turn red are the 3 Gentoo bots. The other non-refleak bot had the same error on test startup. The refleaks bot did run the test, failing with '6 not greater than 6'. I strongly suspect from the latter that the fix would not work on the normal Gentoo bots and that there is something peculiar about tk on the Gentoo machine. I am inclined to let this sit a bit. In the latter, I noticed some new 'Theme Changed' messages from recent tests, including squeezer. I will look into at previous issues where I dealt with them. |
I've got another idea how to make this test work properly; see PR #11585. |
I merged the other solution, so closing this. |
Tk guarantees that TkFixedFont exists.
https://bugs.python.org/issue35730