Skip to content

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 21, 2022

Move Lib/tkinter/test/ to Lib/test/test_tkinter/.

Rename test_tk to test_tkinter, and rename test_ttk_guionly to test_ttk.

@vstinner vstinner requested a review from a team as a code owner June 21, 2022 15:39
@vstinner
Copy link
Member Author

I can still run tests on an newly installed (patched) Python 3.12:

$ /opt/py3.12/bin/python3.12 -m test test_tkinter -u all -v
(...)
Ran 731 tests in 6.853s

$ /opt/py3.12/bin/python3.12 -m test test_ttk -u all -v
(...)
Ran 308 tests in 7.905s

$ /opt/py3.12/bin/python3.12 -m test test_tkinter
test_tkinter skipped -- Use of the 'gui' resource not enabled
test_tkinter skipped (resource denied)

$ /opt/py3.12/bin/python3.12 -m test test_ttk
test_ttk skipped -- Use of the 'gui' resource not enabled
test_ttk skipped (resource denied)

@vstinner
Copy link
Member Author

Rename test_tk to test_tkinter

For me, it's disturbing when tests don't have the same name than the tested module. asyncio => test_asyncio. tkinter => test_tk ???

rename test_ttk_guionly to test_ttk

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
Copy link
Member Author

cc @terryjreedy @serhiy-storchaka

@vstinner
Copy link
Member Author

This PR is special. I made the new directory tree "flat". support.py and test_simpledialog.py now lives in Lib/test/test_tkinter/. Previously, test_simpledialog.py was in a test_tkinter/ sub-directory.

Before:

Lib/tkinter/test/
Lib/tkinter/test/README
Lib/tkinter/test/__init__.py
Lib/tkinter/test/support.py
Lib/tkinter/test/test_tkinter
Lib/tkinter/test/test_tkinter/__init__.py
Lib/tkinter/test/test_tkinter/test_colorchooser.py
Lib/tkinter/test/test_tkinter/test_font.py
Lib/tkinter/test/test_tkinter/test_geometry_managers.py
Lib/tkinter/test/test_tkinter/test_images.py
Lib/tkinter/test/test_tkinter/test_loadtk.py
Lib/tkinter/test/test_tkinter/test_messagebox.py
Lib/tkinter/test/test_tkinter/test_misc.py
Lib/tkinter/test/test_tkinter/test_simpledialog.py
Lib/tkinter/test/test_tkinter/test_text.py
Lib/tkinter/test/test_tkinter/test_variables.py
Lib/tkinter/test/test_tkinter/test_widgets.py
Lib/tkinter/test/test_ttk
Lib/tkinter/test/test_ttk/__init__.py
Lib/tkinter/test/test_ttk/test_extensions.py
Lib/tkinter/test/test_ttk/test_style.py
Lib/tkinter/test/test_ttk/test_widgets.py
Lib/tkinter/test/widget_tests.py

After:

Lib/test/test_ttk/
Lib/test/test_ttk/__init__.py
Lib/test/test_ttk/test_extensions.py
Lib/test/test_ttk/test_style.py
Lib/test/test_ttk/test_widgets.py

Lib/test/test_tkinter/
Lib/test/test_tkinter/README
Lib/test/test_tkinter/__init__.py
Lib/test/test_tkinter/support.py
Lib/test/test_tkinter/test_colorchooser.py
Lib/test/test_tkinter/test_font.py
Lib/test/test_tkinter/test_geometry_managers.py
Lib/test/test_tkinter/test_images.py
Lib/test/test_tkinter/test_loadtk.py
Lib/test/test_tkinter/test_messagebox.py
Lib/test/test_tkinter/test_misc.py
Lib/test/test_tkinter/test_simpledialog.py
Lib/test/test_tkinter/test_text.py
Lib/test/test_tkinter/test_variables.py
Lib/test/test_tkinter/test_widgets.py
Lib/test/test_tkinter/widget_tests.py

@vstinner
Copy link
Member Author

Windows (x64) job failed but I cannot get the logs. I cancelled the workflow. I still cannot see the logs. Strange.

@zware
Copy link
Member

zware commented Jun 21, 2022

Windows (x64) job failed but I cannot get the logs. I cancelled the workflow. I still cannot see the logs. Strange.

The failure is unrelated, see #94068.

@serhiy-storchaka serhiy-storchaka requested review from serhiy-storchaka and removed request for a team June 21, 2022 15:54
@vstinner
Copy link
Member Author

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]

@vstinner vstinner closed this Jun 21, 2022
@vstinner vstinner reopened this Jun 21, 2022
@zooba
Copy link
Member

zooba commented Jun 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.

Copy link
Member

Choose a reason for hiding this comment

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

Does python -m test.test_tkinter still work?

Copy link
Member

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 was test.test_tk before, not test.test_tkinter.

Copy link
Member Author

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.

Copy link
Member

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_xxx
python -m unittest test.test_xxx
python -m test test_xxx

Copy link
Member Author

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 -v

The 3 commands end with: Ran 308 tests.

Copy link
Member

Choose a reason for hiding this comment

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

Why not use just loader.discover()?

Copy link
Member Author

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 :-)

Copy link
Member

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?

Copy link
Member Author

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
Copy link
Member

Should it be backported?

@terryjreedy
Copy link
Member

I would prefer that ttk tests be included under test_tkinter, with names like test_ttk_widgets, so that python -m test.test_tkinter (and python -m test -ugui test_tkinter) tests everything. But re-organization details are up to Serhiy.

@terryjreedy
Copy link
Member

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
Copy link
Member Author

I rebased the PR to get the fix for the Windows build: #94068

@vstinner
Copy link
Member Author

Should it be backported?

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
Copy link
Member Author

I would prefer that ttk tests be included under test_tkinter

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
Copy link
Member Author

vstinner commented Jun 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.

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
Copy link
Member

Are you fine with the 2 proposed tests, or do you want to merge them?

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 test_ttk into test_tkinter, do not forget about test_tcl.

Maybe do this in a separate PR?

@vstinner
Copy link
Member Author

vstinner commented Jun 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 test_ttk into test_tkinter, do not forget about test_tcl.

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
Copy link
Member Author

@serhiy-storchaka: Does it look good to you like that?

@vstinner
Copy link
Member Author

@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.

vstinner added 3 commits June 22, 2022 19:14
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants