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

turtledemo modules imported by test___all__ cause side effects or failures #66081

Closed
ned-deily opened this issue Jun 29, 2014 · 8 comments
Closed
Labels
easy tests Tests in the Lib/test dir

Comments

@ned-deily
Copy link
Member

BPO 21882
Nosy @terryjreedy, @pitrou, @ned-deily

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-07-23.20:00:27.657>
created_at = <Date 2014-06-29.23:56:01.579>
labels = ['easy', 'tests']
title = 'turtledemo modules imported by test___all__ cause side effects or failures'
updated_at = <Date 2014-07-23.20:00:27.656>
user = 'https://github.com/ned-deily'

bugs.python.org fields:

activity = <Date 2014-07-23.20:00:27.656>
actor = 'terry.reedy'
assignee = 'none'
closed = True
closed_date = <Date 2014-07-23.20:00:27.657>
closer = 'terry.reedy'
components = ['Tests']
creation = <Date 2014-06-29.23:56:01.579>
creator = 'ned.deily'
dependencies = []
files = []
hgrepos = []
issue_num = 21882
keywords = ['easy']
message_count = 8.0
messages = ['221921', '221922', '221937', '221966', '221978', '221981', '221988', '223772']
nosy_count = 5.0
nosy_names = ['terry.reedy', 'pitrou', 'ned.deily', 'jesstess', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue21882'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

@ned-deily
Copy link
Member Author

Although the turtledemo modules are not run directly during by "make test" or by "python -m test -uall", they are currently being inadvertently imported by test___all__. This can lead to test failures and side effects because some of the turtledemo modules execute code on import, rather than only when being run via calls to their main() functions. A quick glance shows problems with the following demos: clock (calls mode("logo") which causes a window to appear), colormixer (which unconditionally calls sys.setrecursionlimit()), and two_canvases (which is not structured using functions at all). Depending on how tests are run, these problems can cause serious side effects.

At a minimum,

  1. test___all__ should be changed to exclude turtledemo modules.

It would also be nice to make the demos better citizens:
2. move the mode() call to main() in clock
3. move the setrecursionlimit call to main() and save and restore the original value on exit
4. restructure two_canvases to be like the other demos.
5. double-check all demos for other cases where interpreter state is changed and not restored.

@ned-deily ned-deily added tests Tests in the Lib/test dir easy labels Jun 29, 2014
@pitrou
Copy link
Member

pitrou commented Jun 30, 2014

  1. test___all__ should be changed to exclude turtledemo modules.

Agreed. In general, the test suite shouldn't open any GUI windows except if the "gui" resource is enabled (which isn't the default).
The turtledemo behaviour is quite new in that regard.

@terryjreedy
Copy link
Member

  1. Add 'good citizenship' note to demohelp.txt. Also point out that all turtle initialiazation should be in main anyway (see 2. and 3. below). Demo should run independently of what other demos do.

re 2: If the mode call is needed, it is a bug that it is not main() as another demo could change it.

re 3: ditto, though much less likely.

re 4: As mentioned in bpo-14117, two_canvases is buggy in that the code is not displayed. I added a comment in the file about things that don't work. A main function is the next thing to try anyway.

If no one does the turtledemo changes, I probably will soon.

@terryjreedy
Copy link
Member

I am working on the turtle demos now. Victor gave more info in bpo-21884.

I was partly wrong in my comments. turtledemo uses reload to re-initialize demos when one switches between them. I am tempted to remove this as part of discouraging side-effects on import. It is not a good example to be followed.

@terryjreedy terryjreedy self-assigned this Jun 30, 2014
@python-dev
Copy link
Mannequin

python-dev mannequin commented Jun 30, 2014

New changeset c173a34f20c0 by Terry Jan Reedy in branch '2.7':
Issue bpo-21882: In turtle demos, remove module scope gui and sys calls by
http://hg.python.org/cpython/rev/c173a34f20c0

New changeset fcfa9c5a00fd by Terry Jan Reedy in branch '3.4':
Issue bpo-21882: In turtle demos, remove module scope gui and sys calls by
http://hg.python.org/cpython/rev/fcfa9c5a00fd

@terryjreedy
Copy link
Member

  1. Done

  2. I just removed the setrecursionlimit call, added for 3.0. I moved the colormixer sliders around for longer than anyone is likely to and it ran fine.

  3. two-canvases works fine now. The extra window just has to be clicked away.

  4. nim had a call to turtle.Screen, now in main().

  5. Done
    Let's see what the buildbots say.

  6. Since demos are part of the delivered stdlib, it could be argued that they should get minimal sanity check of being importable. I don't care either way. I leave this to either of you.

@terryjreedy terryjreedy removed their assignment Jun 30, 2014
@terryjreedy
Copy link
Member

The __all__ test now passes on Snow Leapard.

@terryjreedy
Copy link
Member

I opened bpo-22051 about removing reload to force better code. Since the tests are passing and I think the demo modules should be minimally tested by importing, and no one has said otherwise since the fix, I am closing this.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

3 participants