Yelp / Testify

A more pythonic testing framework.
Other
304 stars 67 forks source link

testify should namespace its imports (especially plugins/profile) #342

Closed struys closed 7 years ago

struys commented 7 years ago

@bukzor's notes on this:

testify imports all of its plugins without namespace qualification. One of these plugins is named 'profile.py'. After that, there is a 'profile' entry in sys.modules pointing to that file. Later, when hotshot tries to import the stdlib profile.py, it gets the wrong thing and explodes.

buck describes a solution: "The usual name of /usr/lib/python2.6/dist-packages/testify/plugins/profile.py would be testify.plugins.profile. Instead, testify adds that directory to the sys.path, and imports profile. It would be better to import it with the fully-qualified name, or to remove it from the sys.modules after it has what it wants, so as to not collide with the legitimate profile module. Testify does quite a bit of logic based on what it finds during discovery. For more information, see the long line of spaghetti code at "/usr/lib/python2.6/dist-packages/testify/test_discovery.py", line 57.

Since testify only uses the plugins' module-objects, and doesn't try to import them in the "normal way", I think it's quite valid to just delete the sys.modules entry after doing the import-by-name. Also, I can't think of a general-purpose way to calculate the most-qualified name for plugins on the TESTIFY-PLUGINS path. It would look at bit like this:

import sys plugins = [] 
for plugin in plugin_paths: 
    plugin_name = strip_ext(basename(plugin)) # Don't get confused if somehting by that name is already imported 
    orig_module = sys.modules.get(plugin_name) 
    sys.path.insert(0, plugin_dir(plugin)) 
    plugins.append(__import__(plugin_name)) 
    del sys.path[0] 
    if orig_module is None: 
        del sys.modules[plugin_name(plugin)] 
    else: 
        sys.modules[plugin_name(plugin)] = orig_module
asottile commented 7 years ago

I'm wondering why /usr/lib/python2.6 got involved at all. We don't use the system installed testify (in fact, it's not installed at the system on any of our boxes any more).

The formatting of this post also is broken :(

asottile commented 7 years ago

Though the issue text is a mess. This is done as I understand it:

https://github.com/Yelp/Testify/blob/335f04a7de583026a1e0d04d006f4c40524fd89d/testify/test_program.py#L66-L72

Relevant PR: https://github.com/Yelp/Testify/pull/303

struys commented 7 years ago

@asottile thanks for digging into it.

I attempted to clean up the issue text a bit just so we have a record of the historical problem, @bukzor let me know if it doesn't make sense anymore.