fix: source modules need to be re-imported. #1232

Author: nedbat

Diff for: 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 for: 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 for: 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 for: coverage/tomlconfig.py

@@ -10,7 +10,11 @@
 from coverage.exceptions import CoverageException
 from coverage.misc import import_third_party, substitute_variables
 
-# TOML support is an install-time extra option.
+# TOML support is an install-time extra option. (Import typing is here because
+# import_third_party will unload any module that wasn't already imported.
+# tomli imports typing, and if we unload it, later it's imported again, and on
+# Python 3.6, this causes infinite recursion.)
+import typing   # pylint: disable=unused-import, wrong-import-order
 tomli = import_third_party("tomli")
 
 

Diff for: 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 for: 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 for: 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):