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

Remove doctest import from heapq #85469

Closed
alexchandel mannequin opened this issue Jul 14, 2020 · 8 comments
Closed

Remove doctest import from heapq #85469

alexchandel mannequin opened this issue Jul 14, 2020 · 8 comments
Labels
3.9 only security fixes 3.10 only security fixes performance Performance or resource usage tests Tests in the Lib/test dir

Comments

@alexchandel
Copy link
Mannequin

alexchandel mannequin commented Jul 14, 2020

BPO 41297
Nosy @rhettinger, @ronaldoussoren, @stevendaprano, @alexchandel

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 2020-07-19.07:37:57.123>
created_at = <Date 2020-07-14.21:49:24.164>
labels = ['invalid', 'tests', '3.9', '3.10', 'performance']
title = 'Remove doctest import from heapq'
updated_at = <Date 2020-07-19.07:37:57.122>
user = 'https://github.com/alexchandel'

bugs.python.org fields:

activity = <Date 2020-07-19.07:37:57.122>
actor = 'ronaldoussoren'
assignee = 'none'
closed = True
closed_date = <Date 2020-07-19.07:37:57.123>
closer = 'ronaldoussoren'
components = ['Tests']
creation = <Date 2020-07-14.21:49:24.164>
creator = 'alexchandel'
dependencies = []
files = []
hgrepos = []
issue_num = 41297
keywords = []
message_count = 8.0
messages = ['373658', '373661', '373663', '373809', '373856', '373866', '373880', '373932']
nosy_count = 4.0
nosy_names = ['rhettinger', 'ronaldoussoren', 'steven.daprano', 'alexchandel']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue41297'
versions = ['Python 3.9', 'Python 3.10']

@alexchandel
Copy link
Mannequin Author

alexchandel mannequin commented Jul 14, 2020

heapq.py imports doctest in the last 4 lines to perform unit tests:

    if __name__ == "__main__":
    
        import doctest # pragma: no cover
        print(doctest.testmod()) # pragma: no cover

This disrupts dependency tracking modules and software, like modulegraph and pyinstaller, as doctest brings in many dependencies (including ctypes as of 3.8).

This functionality could be factored out into a separate unit-testing file.

@alexchandel alexchandel mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.10 only security fixes 3.9 only security fixes tests Tests in the Lib/test dir performance Performance or resource usage labels Jul 14, 2020
@stevendaprano
Copy link
Member

The idiom of a module running doctests on itself when executed as a script is a common idiom.

If modulegraph and pyinstaller can't cope a module importing another module from inside an if statement, that's a bug in them, not in the heapq module (and many others).

This requested change is a regression, taking away functionality. 3.8 and older are in feature freeze, so this could only apply to 3.9 and 3.10, and even then, I do not believe it should apply at all.

@stevendaprano stevendaprano removed 3.7 (EOL) end of life 3.8 only security fixes labels Jul 14, 2020
@rhettinger
Copy link
Contributor

I concur with Steven.

@ronaldoussoren
Copy link
Contributor

I have no opinion on the proposed change.

The "disruption" alex c talks about is that this imports gets seen by modulegraph (and hence pyinstaller and py2app), which will then include doctest and all its dependencies in standalone bundles even though doctest isn't actually used.

@rhettinger
Copy link
Contributor

Ronald, can modulegraph be made smarter with respect to sections of code guarded by __name__ == '__main__'? That Python idiom is old and pervasive.

@rhettinger
Copy link
Contributor

If not, could modulegraph add a flag to drop imports commonly found in main sections: doctest, unittest, argparse, etc.?

Asking the standard library to change seems like the tail wagging the dog — it only paints over the problem since third-party modules, other standard library modules, and user code commonly use main sections as well.

@ronaldoussoren
Copy link
Contributor

modulegraph already knows where the import is done, and users of the library can use that information to make decisions.

There's no need to make changes to either heapq.py or modulegraph.

For py2app I've made the choice to no be smart about inclusions and just try to include everything that may be imported because disk space is cheap these days. I just exclude optional dependencies (in py2app, not modulegraph) when I've manually checked that it is safe to do so and the dependency is large.

@rhettinger
Copy link
Contributor

Can we close 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
3.9 only security fixes 3.10 only security fixes performance Performance or resource usage tests Tests in the Lib/test dir
Projects
None yet
Development

No branches or pull requests

3 participants