Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.5k
gh-54781: Move Lib/tkinter/test/test_ttk/ to Lib/test/test_ttk/#94070
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vstinner commentedJun 21, 2022
I can still run tests on an newly installed (patched) Python 3.12: |
vstinner commentedJun 21, 2022
For me, it's disturbing when tests don't have the same name than the tested module. asyncio => test_asyncio. tkinter => test_tk ???
I can keep test_ttk_guionly name if you prefer, but for me, tests should use the GUI. Only the special tests test_ttk_textonly should have a special name, no? |
vstinner commentedJun 21, 2022
vstinner commentedJun 21, 2022
This PR is special. I made the new directory tree "flat". Before: After: |
vstinner commentedJun 21, 2022
Windows (x64) job failed but I cannot get the logs. I cancelled the workflow. I still cannot see the logs. Strange. |
zware commentedJun 21, 2022
vstinner commentedJun 21, 2022
Oh. Windows build failed with an unrelated error: Error: D:\a\cpython\cpython\Modules\socketmodule.c(7475,5): error C2065: 'HVSOCKET_CONTAINER_PASSTHRU': undeclared identifier [D:\a\cpython\cpython\PCbuild_socket.vcxproj] |
zooba commentedJun 21, 2022
Yeah, seems we're in the middle of the rollout of new CI images, so there's a chance you'll get the older image and not see the error. But you'll need my PR for it to be reliable. |
Lib/test/test_tkinter/__init__.py Outdated
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.
Doespython -m test.test_tkinter still work?
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.
It would work if a__main__ module gets added. But it wastest.test_tk before, nottest.test_tkinter.
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.
Ah you care about this command? If you do, I can add a__main__.py file.
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.
Yes, please add it. And test different ways of running tests:
python -m test.test_xxxpython -m unittest test.test_xxxpython -m test test_xxxThere 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.
I added__main__ sub-modules. Example:
./python -m test.test_ttk -v./python -m test test_ttk -v -u all./python -m unittest test.test_ttk -vThe 3 commands end with:Ran 308 tests.
Lib/test/test_tkinter/__init__.py Outdated
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.
Why not use justloader.discover()?
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.
I copy/paste the code from test_asyncio, it works, so I didn't change it :-)
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.
Doesloader.discover() work with zipimport?
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.
Does loader.discover() work with zipimport?
I have no idea. If there is an issue, I suggest to fix all load_tests() functions of Lib/test/ in a separated PR.
serhiy-storchaka commentedJun 21, 2022
Should it be backported? |
terryjreedy commentedJun 21, 2022
I would prefer that ttk tests be included under test_tkinter, with names like |
terryjreedy commentedJun 21, 2022
This patch will require changes to the scripts *nix distributions use to make tkinter (and IDLE) a separate install. I don't know how they would feel about backports. |
vstinner commentedJun 21, 2022
I rebased the PR to get the fix for the Windows build:#94068 |
vstinner commentedJun 21, 2022
This specific PR change the name of two tests, so no, it should not be backported. It also changes the build system (Linux, VS project). Honestly, it sounds risky and I would prefer to not backport it. |
vstinner commentedJun 21, 2022
Oh. I tried to minimize changes in this PR. But I'm open to reorganizing tests. @serhiy-storchaka: Are you fine with the 2 proposed tests, or do you want to merge them? |
vstinner commentedJun 21, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Once issue#54781 will be fixed, I will properly document these changes in What's New in Python 3.12. IMO it fits into the Build Changes section. |
serhiy-storchaka commentedJun 21, 2022
I was going to ask about merging them, but I am fine with the current state of this PR. If you are going to merge Maybe do this in a separate PR? |
vstinner commentedJun 21, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Oh, merging 3 Tkinter tests sound out of my skills. I don't know well this module and I would prefer if someone else do it. Here I'm trying to focus on fixing the 12 years old issue#54781 :-) Perfect is the enemy of good ;-) |
vstinner commentedJun 21, 2022
@serhiy-storchaka: Does it look good to you like that? |
vstinner commentedJun 22, 2022
@serhiy-storchaka wrote "I am fine with the current state of this PR". I plan to merge the PR tomorrow if I don't hear back from@serhiy-storchaka. |
* Add Lib/test/test_ttk/__init__.py based on test_ttk_guionly.py.* Remove Lib/test/test_ttk_guionly.py.
Remove Lib/test/test_tk.py.
Uh oh!
There was an error while loading.Please reload this page.
Move Lib/tkinter/test/ to Lib/test/test_tkinter/.
Rename test_tk to test_tkinter, and rename test_ttk_guionly to test_ttk.