Reactivate the importing test. #136
Conversation
| @@ -112,6 +112,9 @@ def __bootstrap__(): | |||
| ext_filepath = pkg_resources.resource_filename(__name__, {ext_file!r}) | |||
| m = load_from_spec(Spec({module_name!r}, ext_filepath)) | |||
| m.__file__ = ext_filepath | |||
| m.__loader__ = __loader__ | |||
There was a problem hiding this comment.
I'm not 100% sure that copy the loader and spec are the perfect thing to do, but:
- These are the loader and spec that were used to import the module, so they're correct in that sense.
- There isn't another loader and spec than can be used to import an hpy.universal extension.
test/test_importing.py
Outdated
| import sys | ||
| modname = 'mytest' | ||
| so_filename = self.compile_module(""" | ||
| if not hasattr(sys, "executable"): |
There was a problem hiding this comment.
Is is to check whether we are inside app-level tests or not. I should probably factor this out into a small helper function but I'm not particular sure what to call it. is_app_level is one idea, but perhaps that's too PyPy specific?
There was a problem hiding this comment.
ideally, the HPy tests should be independent of the implementation. But practically speaking, being able to do if is_pypy_apptest will probably be very useful.
What about introducing a generic way to check the implementation? E.g., we could have self.implementation which by default is sys.implementation.name (so, 'cpython', 'pypy', etc.), but pypy's test/support.py could override it and set to 'pypy_apptest'. And we could also add a self.is_pypy which does return self.implementation in ('pypy', 'pypy_apptest') maybe?
There was a problem hiding this comment.
I added some more methods that describe the possible capabilities of the test runner that the hpy tests currently care about. I dabbled with making something more generic (e.g. a .capabilities(...) method that returns a set of supported behaviours) but that was actually more code and less clear than the simple methods, so I went back to those.
| so_filename = self.compile_module(""" | ||
| if not hasattr(sys, "executable"): | ||
| pytest.skip() | ||
| mod = self.make_module(""" |
There was a problem hiding this comment.
make_module now does a proper import, so we don't need to do anything different in this test to import the module normally.
There was a problem hiding this comment.
I fear that we are stumbling across each other feet in this PR and #142, sorry for that :(.
I strongly believe that make_module should NOT do a proper import. Basically, the biggest difference between loading a module in memory and "properly importing" it is that the latter modifies some global state (e.g. sys.modules) plus it has interactions with importlib caches, etc.. And for tests, the least we mess with global state, the better it is.
E.g. for testing the debug mode, we need to load the same module twice (with and without the debug mode support), so if we do "proper imports" we need to un-import the module between tests etc. It's a mess. That's why self.make_module was originally written like that, it even had a comment to make this behavior explicit ;)
See also commit ba33924: I believe that the code written this way is much cleaner and doesn't need all the hacks to sys.path, sys.modules, etc.
That said, we surely need a way to properly import modules of course, but it's probably better/easier to leave it clearly separate from the low-level stuff used by tests. So I propose the following:
hpy.universal.loadis a very low-level function which just loads the module in memory, without "importing" it- we write a helper (maybe called
hpy.importlibor something like that) which useshpy.universal.loadunder the hood hpy.importlibcan have its own tests of course: these can be written by callingself.compile_module+hpy.importlib.do_proper_importinstead of relying onself.make_moduleto do everything- the .py stubs can call
hpy.importlibto do whatever is appropriate
What do you think about this solution?
|
@antocuni Ready for a next review, I think. |
|
@antocuni I am happy for us to go back to Probably we need the current import code in |
sounds like a good idea, +1 |
|
Thanks! |
No description provided.