Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reactivate the importing test. #136

Merged
merged 5 commits into from
Dec 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions hpy/devel/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure that copy the loader and spec are the perfect thing to do, but:

  1. These are the loader and spec that were used to import the module, so they're correct in that sense.
  2. There isn't another loader and spec than can be used to import an hpy.universal extension.

m.__package__ = __package__
m.__spec__ = __spec__
sys.modules[__name__] = m

__bootstrap__()
Expand Down
34 changes: 30 additions & 4 deletions test/support.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,10 @@ def load_module(self, name, mod_filename):
"Test module {!r} already present in sys.modules".format(name))
importlib.invalidate_caches()
mod_dir = os.path.dirname(mod_filename)
sys.path.insert(0, mod_dir)
try:
sys.path.insert(0, mod_dir)
module = importlib.import_module(name)
assert sys.modules[name] is module
finally:
# assert that the module import didn't change the sys.path entry
# that was added above, then remove the entry.
Expand All @@ -243,9 +244,34 @@ def make_module(self, main_src, name='mytest', extra_sources=()):
return self.compiler.make_module(ExtensionTemplate, main_src, name,
extra_sources)

def should_check_refcount(self):
# defaults to True on CPython, but is set to False by e.g. PyPy
return sys.implementation.name == 'cpython'
def supports_refcounts(self):
""" Returns True if the underlying Python implementation supports
reference counts.

By default returns True on CPython and False on other
implementations.
"""
return sys.implementation.name == "cpython"

def supports_ordinary_make_module_imports(self):
""" Returns True if `.make_module(...)` loads modules using a
standard Python import mechanism (e.g. `importlib.import_module`).

By default returns True because the base implementation of
`.make_module(...)` uses an ordinary import. Sub-classes that
override `.make_module(...)` may also want to override this
method.
"""
return True

def supports_sys_executable(self):
""" Returns True is `sys.executable` is set to a value that allows
a Python equivalent to the current Python to be launched via, e.g.,
`subprocess.run(...)`.

By default returns `True` if sys.executable is set to a true value.
"""
return bool(getattr(sys, "executable", None))


# the few functions below are copied and adapted from cffi/ffiplatform.py
Expand Down
8 changes: 4 additions & 4 deletions test/test_cpy_compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

class TestCPythonCompatibility(HPyTest):

# One note about the should_check_refcount() in the tests below: on
# One note about the supports_refcounts() in the tests below: on
# CPython, handles are actually implemented as INCREF/DECREF, so we can
# check e.g. after an HPy_Dup the refcnt is += 1. However, on PyPy they
# are implemented in a completely different way which is unrelated to the
Expand Down Expand Up @@ -35,7 +35,7 @@ def test_frompyobject(self):
x = mod.f()
assert x[0] == 1234
assert len(x) == 2
if self.should_check_refcount():
if self.supports_refcounts():
assert x == [1234, +1]

def test_aspyobject(self):
Expand Down Expand Up @@ -97,7 +97,7 @@ def test_hpy_close(self):
@INIT
""")
x = mod.f()
if self.should_check_refcount():
if self.supports_refcounts():
assert x == -1

def test_hpy_dup(self):
Expand All @@ -123,7 +123,7 @@ def test_hpy_dup(self):
@INIT
""")
x = mod.f()
if self.should_check_refcount():
if self.supports_refcounts():
assert x == +1

def test_many_handles(self):
Expand Down
2 changes: 1 addition & 1 deletion test/test_hpyerr.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def test_FatalError(self):
@EXPORT(f)
@INIT
""")
if not hasattr(sys, "executable"):
if not self.supports_sys_executable():
# if sys.executable is not available (e.g. inside pypy app-level)
# tests, then skip the rest of this test
return
Expand Down
2 changes: 1 addition & 1 deletion test/test_hpytype.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ def test_HPy_New(self):
def test_refcount(self):
import pytest
import sys
if not self.should_check_refcount():
if not self.supports_refcounts():
pytest.skip()
mod = self.make_module("""
@DEFINE_PointObject
Expand Down
30 changes: 9 additions & 21 deletions test/test_importing.py
Original file line number Diff line number Diff line change
@@ -1,31 +1,19 @@
import pytest as pytest_collecting

from .support import HPyTest

# this function should probably goes somewhere into hpy.universal and/or and
# hpy package and/or an import hook, or whatever. I do not want to think about
# this now.
def import_module_properly(mod):
raise NotImplementedError("fix me eventually")

# this was moved from support.py, where it did not belong
## class HPyLoader(ExtensionFileLoader):
## def create_module(self, spec):
## import hpy.universal
## return hpy.universal.load_from_spec(spec)


class TestImporting(HPyTest):

@pytest_collecting.mark.xfail
def test_importing_attributes(self):
import sys
modname = 'mytest'
so_filename = self.compile_module("""
import pytest
if not self.supports_ordinary_make_module_imports():
pytest.skip()
mod = self.make_module("""
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_module now does a proper import, so we don't need to do anything different in this test to import the module normally.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. hpy.universal.load is a very low-level function which just loads the module in memory, without "importing" it
  2. we write a helper (maybe called hpy.importlib or something like that) which uses hpy.universal.load under the hood
  3. hpy.importlib can have its own tests of course: these can be written by calling self.compile_module + hpy.importlib.do_proper_import instead of relying on self.make_module to do everything
  4. the .py stubs can call hpy.importlib to do whatever is appropriate

What do you think about this solution?

@INIT
""", name=modname)
mod = import_module_properly(so_filename, modname)
assert mod in sys.modules
""", name='mytest')
assert mod.__name__ == 'mytest'
assert mod.__package__ == ''
assert mod.__doc__ == 'some test for hpy'
assert mod.__loader__.name == 'mytest'
assert mod.__spec__.loader is mod.__loader__
assert mod.__spec__.name == 'mytest'
assert mod.__file__