classification
Title: test.support.DirsOnSysPath should be replaced by importlib.test.util.import_state
Type: Stage:
Components: Versions: Python 3.3
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, asvetlov, barry, brett.cannon, chris.jerdonek, eric.smith, eric.snow, jason.coombs, merwok
Priority: normal Keywords: patch

Created on 2012-05-03 21:04 by eric.smith, last changed 2013-01-26 05:56 by eric.snow.

Files
File name Uploaded Description Edit
issue14715-0.diff eric.smith, 2012-05-16 18:38 review
Messages (12)
msg159873 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-03 21:04
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.
msg159878 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-05-03 21:52
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.
msg159896 - (view) Author: √Čric Araujo (merwok) * (Python committer) Date: 2012-05-04 03:25
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.
msg160793 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-16 01:44
Clean up code in test_pkgutil once this issue is fixed. See issue 14817.
msg160910 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-16 18:38
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.
msg160932 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-16 21:11
Also for use in test_pkgutil.
msg160945 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-05-17 01:29
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.
msg160946 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-17 01:34
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.
msg160947 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-17 01:43
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.
msg160972 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-05-17 15:21
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.
msg161017 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2012-05-17 21:33
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.
msg161064 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-05-18 16:50
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).
History
Date User Action Args
2013-01-26 05:56:55eric.snowsetnosy: + eric.snow
2012-12-23 20:02:08asvetlovsetnosy: + asvetlov
2012-07-08 23:59:28chris.jerdoneksetnosy: + chris.jerdonek
2012-06-17 20:40:03pitrouunlinkissue14657 dependencies
2012-05-18 16:50:24brett.cannonsetmessages: + msg161064
2012-05-17 21:33:43eric.smithsetmessages: + msg161017
2012-05-17 16:23:24Arfreversetnosy: + Arfrever
2012-05-17 15:21:53brett.cannonsetmessages: + msg160972
2012-05-17 01:43:40eric.smithsetmessages: + msg160947
2012-05-17 01:34:36eric.smithsetmessages: + msg160946
2012-05-17 01:29:59brett.cannonsetmessages: + msg160945
2012-05-16 21:11:17eric.smithsetmessages: + msg160932
2012-05-16 18:38:24eric.smithsetfiles: + issue14715-0.diff
keywords: + patch
messages: + msg160910
2012-05-16 01:44:16eric.smithsetmessages: + msg160793
2012-05-06 16:13:04brett.cannonlinkissue14657 dependencies
2012-05-04 03:25:31merwoksetnosy: + merwok
messages: + msg159896
2012-05-03 21:52:38brett.cannonsetmessages: + msg159878
2012-05-03 21:04:29eric.smithcreate