| msg159873 - (view) |
Author: Eric V. Smith (eric.smith) *  |
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) * |
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 (eric.araujo) * |
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) * |
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) * |
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) * |
Date: 2012-05-16 21:11 |
| Also for use in test_pkgutil. |
|
|
| msg160945 - (view) |
Author: Brett Cannon (brett.cannon) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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). |
|
|
| msg360920 - (view) |
Author: Brett Cannon (brett.cannon) * |
Date: 2020-01-29 00:49 |
| Do we still care about this, Eric? |
|
|
| msg360940 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2020-01-29 07:23 |
| I don't think so. I don't think the patch even did what I wanted it to do. I'll close it. |
|
|