diff --git a/CHANGES.rst b/CHANGES.rst index 696dd4b0b..2b7add47d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -27,11 +27,20 @@ Unreleased was ignored as a third-party package. That problem (`issue 1231`_) is now fixed. +- Packages named as "source packages" (with ``source``, or ``source_pkgs``, or + pytest-cov's ``--cov``) might have been only partially measured. Their + top-level statements could be marked as unexecuted, because they were + imported by coverage.py before measurement began (`issue 1232`_). This is + now fixed, but the package will be imported twice, once by coverage.py, then + again by your test suite. This could cause problems if importing the package + has side effects. + - The :meth:`.CoverageData.contexts_by_lineno` method was documented to return a dict, but was returning a defaultdict. Now it returns a plain dict. It also no longer returns negative numbered keys. .. _issue 1231: https://github.com/nedbat/coveragepy/issues/1231 +.. _issue 1232: https://github.com/nedbat/coveragepy/issues/1232 .. _changes_601: diff --git a/coverage/inorout.py b/coverage/inorout.py index c90e3d594..2c216ea9d 100644 --- a/coverage/inorout.py +++ b/coverage/inorout.py @@ -18,6 +18,7 @@ from coverage.exceptions import CoverageException from coverage.files import TreeMatcher, FnmatchMatcher, ModuleMatcher from coverage.files import prep_patterns, find_python_files, canonical_filename +from coverage.misc import sys_modules_saved from coverage.python import source_for_file, source_for_morf @@ -270,27 +271,28 @@ def debug(msg): # Check if the source we want to measure has been installed as a # third-party package. - for pkg in self.source_pkgs: - try: - modfile, path = file_and_path_for_module(pkg) - debug(f"Imported source package {pkg!r} as {modfile!r}") - except CoverageException as exc: - debug(f"Couldn't import source package {pkg!r}: {exc}") - continue - if modfile: - if self.third_match.match(modfile): - debug( - f"Source is in third-party because of source_pkg {pkg!r} at {modfile!r}" - ) - self.source_in_third = True - else: - for pathdir in path: - if self.third_match.match(pathdir): + with sys_modules_saved(): + for pkg in self.source_pkgs: + try: + modfile, path = file_and_path_for_module(pkg) + debug(f"Imported source package {pkg!r} as {modfile!r}") + except CoverageException as exc: + debug(f"Couldn't import source package {pkg!r}: {exc}") + continue + if modfile: + if self.third_match.match(modfile): debug( - f"Source is in third-party because of {pkg!r} path directory " + - f"at {pathdir!r}" + f"Source is in third-party because of source_pkg {pkg!r} at {modfile!r}" ) self.source_in_third = True + else: + for pathdir in path: + if self.third_match.match(pathdir): + debug( + f"Source is in third-party because of {pkg!r} path directory " + + f"at {pathdir!r}" + ) + self.source_in_third = True for src in self.source: if self.third_match.match(src): diff --git a/coverage/misc.py b/coverage/misc.py index 9c414d88c..29397537c 100644 --- a/coverage/misc.py +++ b/coverage/misc.py @@ -3,6 +3,7 @@ """Miscellaneous stuff for coverage.py.""" +import contextlib import errno import hashlib import importlib @@ -49,6 +50,28 @@ def isolate_module(mod): os = isolate_module(os) +class SysModuleSaver: + """Saves the contents of sys.modules, and removes new modules later.""" + def __init__(self): + self.old_modules = set(sys.modules) + + def restore(self): + """Remove any modules imported since this object started.""" + new_modules = set(sys.modules) - self.old_modules + for m in new_modules: + del sys.modules[m] + + +@contextlib.contextmanager +def sys_modules_saved(): + """A context manager to remove any modules imported during a block.""" + saver = SysModuleSaver() + try: + yield + finally: + saver.restore() + + def import_third_party(modname): """Import a third-party module we need, but might not be installed. @@ -63,16 +86,11 @@ def import_third_party(modname): The imported module, or None if the module couldn't be imported. """ - try: - mod = importlib.import_module(modname) - except ImportError: - mod = None - - imported = [m for m in sys.modules if m.startswith(modname)] - for name in imported: - del sys.modules[name] - - return mod + with sys_modules_saved(): + try: + return importlib.import_module(modname) + except ImportError: + return None def dummy_decorator_with_args(*args_unused, **kwargs_unused): diff --git a/doc/source.rst b/doc/source.rst index 8debd575f..bab57a723 100644 --- a/doc/source.rst +++ b/doc/source.rst @@ -39,6 +39,11 @@ in their names will be skipped (they are assumed to be scratch files written by text editors). Files that do not end with ``.py``, ``.pyw``, ``.pyo``, or ``.pyc`` will also be skipped. +.. note:: Modules named as sources may be imported twice, once by coverage.py + to find their location, then again by your own code or test suite. Usually + this isn't a problem, but could cause trouble if a module has side-effects + at import time. + You can further fine-tune coverage.py's attention with the ``--include`` and ``--omit`` switches (or ``[run] include`` and ``[run] omit`` configuration values). ``--include`` is a list of file name patterns. If specified, only diff --git a/tests/mixins.py b/tests/mixins.py index 0638f3366..95b2145a3 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -15,6 +15,7 @@ import pytest +from coverage.misc import SysModuleSaver from tests.helpers import change_dir, make_file, remove_files @@ -96,21 +97,11 @@ def _save_sys_path(self): @pytest.fixture(autouse=True) def _module_saving(self): """Remove modules we imported during the test.""" - self._old_modules = list(sys.modules) + self._sys_module_saver = SysModuleSaver() try: yield finally: - self._cleanup_modules() - - def _cleanup_modules(self): - """Remove any new modules imported since our construction. - - This lets us import the same source files for more than one test, or - if called explicitly, within one test. - - """ - for m in [m for m in sys.modules if m not in self._old_modules]: - del sys.modules[m] + self._sys_module_saver.restore() def clean_local_file_imports(self): """Clean up the results of calls to `import_local_file`. @@ -120,7 +111,7 @@ def clean_local_file_imports(self): """ # So that we can re-import files, clean them out first. - self._cleanup_modules() + self._sys_module_saver.restore() # Also have to clean out the .pyc file, since the timestamp # resolution is only one second, a changed file might not be diff --git a/tests/test_misc.py b/tests/test_misc.py index 077c24344..740022322 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -165,8 +165,15 @@ class ImportThirdPartyTest(CoverageTest): run_in_temp_dir = False def test_success(self): + # Make sure we don't have pytest in sys.modules before we start. + del sys.modules["pytest"] + # Import pytest mod = import_third_party("pytest") + # Yes, it's really pytest: assert mod.__name__ == "pytest" + print(dir(mod)) + assert all(hasattr(mod, name) for name in ["skip", "mark", "raises", "warns"]) + # But it's not in sys.modules: assert "pytest" not in sys.modules def test_failure(self):