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 tests from idlelib.idle_test to test.test_idle #115444

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 14, 2024

This work is based on #94145

I've addressed the original feedback.

move unittest-related files in idlelib.idle_test to a new test/test_idle directory and split test/test_idle.py into test_idle/__init__.py and test_idle/__main__.py

Done ✅

Note that I want idle_test and any non-unittest files (at least htest.py and part of README.txt) left as they are.

Done ✅

Among other comments:

  • git should handle the path change smoothly
  • The diff itself is very small

@terryjreedy please, share your feedback, so we can make this transition as convenient for you as possible :)

@vstinner
Copy link
Member

Well, for these tests, I will rely on @terryjreedy's decision. If he is fine with the overall approach, I can review the implementation.

@terryjreedy
Copy link
Member

I am still opposed to the idea (see issue) but will review this to see if it satisfies my objections to the previous proposed implementation.

@sobolevn
Copy link
Member Author

@terryjreedy did you have a chance to look at it? :)

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.

I want to make IDLE's tests as independent of location as sensibly possible, but not move them now. The comments are about what can/should go in a new PR to do the former.

This PR could be then simplified and retested before being closed until needed.

@@ -225,4 +225,4 @@ def get_entity(self, name):

if __name__ == '__main__':
from unittest import main
main('idlelib.idle_test.test_autocomplete', verbosity=2)
main('test.test_idle.test_autocomplete', verbosity=2)
Copy link
Member

Choose a reason for hiding this comment

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

I would add utestdir = 'test.test_idle to util.py and from idlelib.util import utestdir above each main line, which would then become main('test.test_idle.test_autocomplete', verbosity=2). I believe easy enough with sed.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, if tests were not installed, this would just crash, and prevent running any htest that follows. Running a file's unittest would have to be made conditional (or the exception caught). This if tests were moved, this should be encapsulated in a util function that would fail gracefully.

@@ -357,7 +357,7 @@ def removecolors(self):

def _color_delegator(parent): # htest #
from tkinter import Toplevel, Text
from idlelib.idle_test.test_colorizer import source
from test.test_idle.test_colorizer import source
Copy link
Member

Choose a reason for hiding this comment

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

This would introduce a bug if IDLE were installed and tests were not (which is normal on macOS) and this were run. My thought is to move the (illegal) sample code into a file in idle_test.

@@ -1,8 +1,10 @@
import unittest
import os

from test import support
Copy link
Member

Choose a reason for hiding this comment

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

This and the next change are pure style preference, independent of this issue. I don't care either way.

idle_dir = os.path.dirname(__file__)
else:
idle_dir = os.path.abspath(sys.path[0])
idle_dir = os.path.abspath(os.path.dirname(idlelib.__file__))
Copy link
Member

Choose a reason for hiding this comment

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

I believe all these test file changes make them independent of test file location. If so, they can be done now without moving the files. It would be easy to find out. Some of these look like improvements in any case. I would add a note to idle_test.README.txt that tests should not depend on being in idle_test.

@bedevere-app
Copy link

bedevere-app bot commented Mar 18, 2024

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

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.

4 participants