Repository navigation
test.support.DirsOnSysPath should be replaced by importlib.test.util.import_state #58920
Description
| BPO | 14715 |
|---|---|
| Nosy | @warsaw, @brettcannon, @jaraco, @ericvsmith, @merwok, @asvetlov, @cjerdonek, @ericsnowcurrently |
| Files |
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-01-29.07:23:57.298>
created_at = <Date 2012-05-03.21:04:29.246>
labels = ['3.9']
title = 'test.support.DirsOnSysPath should be replaced by importlib.test.util.import_state'
updated_at = <Date 2020-01-29.07:23:57.297>
user = 'https://github.com/ericvsmith'bugs.python.org fields:
activity = <Date 2020-01-29.07:23:57.297>
actor = 'eric.smith'
assignee = 'none'
closed = True
closed_date = <Date 2020-01-29.07:23:57.298>
closer = 'eric.smith'
components = []
creation = <Date 2012-05-03.21:04:29.246>
creator = 'eric.smith'
dependencies = []
files = ['25614']
hgrepos = []
issue_num = 14715
keywords = ['patch']
message_count = 14.0
messages = ['159873', '159878', '159896', '160793', '160910', '160932', '160945', '160946', '160947', '160972', '161017', '161064', '360920', '360940']
nosy_count = 9.0
nosy_names = ['barry', 'brett.cannon', 'jaraco', 'eric.smith', 'eric.araujo', 'Arfrever', 'asvetlov', 'chris.jerdonek', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue14715'
versions = ['Python 3.9']
Activity
ericvsmith commentedon May 3, 2012
DirsOnSysPath doesn't clear sys.path_importer_cache, so it seems you'd always want to use import_state, which does clear it.
We might also want to modify import_state to remember the original objects, not just their values. DirsOnSysPath does do this, for sys.path.
If we do this, we should probably move import_state to test.support.
Also, couldn't import_state do with sys.modules what DirsOnSysPath does with sys.path? It could restore both the object and its contents.
brettcannon commentedon May 3, 2012
Yes, it could do all of this. One possibility that I was thinking of, was this could be a setUp()/tearDown() thing as well instead of a context manager. Another option is a simple decorator. Either way it might be easier to have it just save state and then in the body of the code set everything as desired instead of in the context manager's init.
merwok commentedon May 4, 2012
I tend to like context managers, or helpers that work with addCleanup, so that I keep the scaffolding next to the test code. setUp and tearDown are better in some cases, though. It is also possible to have an helper work as a decorator and a context manager and a setUp-tearDown thing with small effort.
ericvsmith commentedon May 16, 2012
Clean up code in test_pkgutil once this issue is fixed. See bpo-14817.
ericvsmith commentedon May 16, 2012
Here's what I imagine the new function will look like. I propose moving it to test.support and using it outside of importlib, specifically for the PEP-420 tests.
ericvsmith commentedon May 16, 2012
Also for use in test_pkgutil.
brettcannon commentedon May 17, 2012
So from what I can tell you are advocating not resetting anything by default but instead only save the details and then reset it later. That's fine with me (aligns more with the warnings context manager), the importlib tests will simply need to be updated so as to fix their assumptions that everything gets cleared.
ericvsmith commentedon May 17, 2012
Actually, I was planning on resetting everything, but I haven't gotten that to work yet. I can't figure out why (but I will!).
With the current patch, where things are not reset, the only test I had to change was test_path_importer_cache_empty_string. That change is in the attached -0 diff.
I can't decide which way I prefer it. I might add a parameter to control the behavior.
ericvsmith commentedon May 17, 2012
The current tests don't like setting sys.modules to [].
I like resetting everything (including sys.modules) back to the original state. Otherwise some tests, which do things like "import foo", affect later tests.
So, I think I'll leave the patch as-is. I'll rename it, fix all references to it, then check it in. After that I'll start using it in other tests.
brettcannon commentedon May 17, 2012
sys.modules is tricky thanks to built-in modules not liking to be re-imported (and why import_state in importlib.test didn't reset it).
If only one test fails because of my assumption, then I guess blanking them out really isn't as important as I thought. And if you have to blank everything out for a test, then you are just careful to blank everything out, no matter how verbose it gets.
ericvsmith commentedon May 17, 2012
I understand about sys.modules. Maybe I'll create another context manager (say, sys_modules_state) that does the same for sys.modules. I can always stack them together.
When loading pure-Python namespace packages, I want to make sure they get removed before the next test.
brettcannon commentedon May 18, 2012
I see three options (which can be combined). One is to keep using something like the uncache context manager to make sure the expected modules get removed. Two is to check the keys of sys.modules at the end of a context manager and if there are new keys either blindly clean up or raise an error/warning that stuff was left behind. Lastly is to leave all built-in modules (and maybe extension modules if you are worried they will do something silly during init) in sys.modules and then clear out the rest (although that might screw up importlib if we end up hiding importlib._bootstrap behind _frozen_importlib, in which case you will want to leave frozen modules as well or special-case _frozen_importlib).
brettcannon commentedon Jan 29, 2020
Do we still care about this, Eric?
ericvsmith commentedon Jan 29, 2020
I don't think so. I don't think the patch even did what I wanted it to do. I'll close it.