Skip to content
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

gh-95069: Move Lib/idlelib/idle_test/ to Lib/test/test_idle/ #94145

Closed
wants to merge 10 commits into from
Closed

gh-95069: Move Lib/idlelib/idle_test/ to Lib/test/test_idle/ #94145

wants to merge 10 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 22, 2022

  • Move Lib/idlelib/idle_test/ to Lib/test/test_idle/
  • Remove Lib/test/test_idle.py
  • test_idle: Replace idlelib.idle_test with test.test_idle
  • idlelib: Replace idlelib.idle_test with test.test_idle
  • README.txt: Replace idle_test with test_idle
  • Update references
  • Update build sytem
  • Add test_idle.main sub-module
  • Update tests

@vstinner
Copy link
Member Author

@terryjreedy asked to not move IDLE tests:

I would prefer to not have IDLE as a special case and have all tests in Lib/test/. But @terryjreedy is the IDLE maintainer, and so I will follow his decision :-)

I created a PR to see in practice the consequences of moving Lib/idlelib/idle_test/ to Lib/test/test_idle/.

IMO such PR should not be backported since it has an impact on Linux distributions which put Lib/idlelib/idle_test/ in a separated "test" package, separated from the "python" package, to keep the "python" package smaller. At least, Fedora does that for "python3-test".

If the PR is not backported, we rely on Git to detect renamed/moved files to automate backports. In my experience, Git is quite good at that. We move files often, and Git always smoothly finds the old files and automatically patchs the correct file on backport.

If I understood correctly, IDLE is developed as an application separated from Python, but its code lives in Python Lib/idlelib/ directory. The code is the same, or almost the same, in 3.10, 3.11 and main branches. So I'm not sure how to deal with the minor different introduced by this PR. For example, 'idlelib.idle_test.test_help' is replaced with 'test.test_idle.test_help' in Lib/idlelib/help.py:

$ tail Lib/idlelib/help.py

if __name__ == '__main__':
    from unittest import main
    main('test.test_idle.test_help', verbosity=2, exit=False)

    from test.test_idle.htest import run
    run(show_idlehelp)

asyncio was developed as a third-party project (named "tulip") and distributed on PyPI. I recall that it was painful to synchronize the code between the 3rd party project and all supported branches in Python. So well, I'm not sure why should be done with IDLE. It's up to you Terry ;-)

cc @serhiy-storchaka who also seems to be actively maintaining IDLE.

@vstinner
Copy link
Member Author

Oh, I'm not sure how to copy this code from the removed test_idle.py:

if __name__ == '__main__':
    tk.NoDefaultRoot()
    unittest.main(exit=False)
    tk._support_default_root = True
    tk._default_root = None

Maybe put a setUpModule() method somewhere.

@vstinner
Copy link
Member Author

PR rebased to fix a merge conflict (remove unused imports).

@terryjreedy terryjreedy changed the title gh-54781: Move Lib/idlelib/idle_test/ to Lib/test/test_idle/ gh-95069: Move Lib/idlelib/idle_test/ to Lib/test/test_idle/ Jul 20, 2022
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

As I said elsewhere, idle_test/ should remain, so don't remove it. htest.py and maybe something else should remain there. Revert changes to from idlelib.idle_test.htest import run. In the future, I may put other non-unittest integration tests in idle_test, or try to automate some of the suggested manual tests in the htests.

I opened a new issue, #95069, for moving IDLE test and retargeted this PR to that issue.

I need to work on some substantive improvements to IDLE before I spend much more time on this.

Comment on lines +1 to +4
from . import load_tests
import unittest

unittest.main()
Copy link
Member

Choose a reason for hiding this comment

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

IDLE runs with NoDefaultRoot. It MUST be tested that way, at least when an IDLE maintainer runs test.test_idle directly.

Suggested change
from . import load_tests
import unittest
unittest.main()
from . import load_tests, tk
import unittest
tk.NoDefaultRoot()
unittest.main(exit=False)
tk._support_default_root = True
tk._default_root = None

The tk calls were originally in the main part of test_idle.py. Later, a change detector was added that detected the changes even though the two values were reverted to their original values. The change warning was turned into a test failure. So the temporary patching was moved to only apply when running test.test_idle as main. I consider this a hack and would prefer that IDLE always be tested the way it is run. I could then add a test that _support_default_root is false.

Victor, why is this reverted change treated differently from mock patching and unpatching? Does the change detector run by regrtest run before the reversion? If so, is there any way to patch the whole test run so that the change is undone before the change check? If not, can we find out what change is seen? (Perhaps NoDefaultRoot does more than I think.) If nothing else, could the change detector special case ignore these particular changes?

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner
Copy link
Member Author

vstinner commented Aug 3, 2022

Sorry I lost track of the issue. I mostly wrote it to see how it can go. It seems like you want to do it differently. Feel free to do it your way and copy/paste anything from this PR. I close my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants