Merge branch 'xdist-early-activation'

Author: jaltmayerpizzorno

src/slipcover/pytest_plugin.py

@@ -36,37 +36,29 @@ def _get_worker_id() -> str:
     return os.environ.get("PYTEST_XDIST_WORKER", "main")
 
 
-def pytest_configure(config):
-    """Initialize slipcover in xdist workers.
-
-    This hook runs in both the controller and worker processes.
-    We only activate slipcover if SLIPCOVER_ENABLED is set AND we're in an xdist worker.
-    The controller (main process) is already handled by __main__.py.
+def _activate_worker():
+    """Activates instrumentation for an xdist worker as early as possible.
+
+    This runs at module import time (see the call at the bottom of this file),
+    not inside pytest_configure(): pytest_configure() only fires after pytest
+    has already auto-loaded every pytest11 entry-point plugin and read the
+    initial conftest.py files, so anything they import would already have
+    bypassed ImportManager by the time pytest_configure() ran. This module is
+    itself imported via that same entry-point autoload mechanism, so
+    top-level code here runs before conftest.py is ever read.
     """
     global _slipcover_instance, _file_matcher, _import_manager, _coverage_dir
 
-    # Only activate if SLIPCOVER_ENABLED is set (by __main__.py when running pytest)
-    if not os.environ.get("SLIPCOVER_ENABLED"):
+    # Only activate if SLIPCOVER_ENABLED is set (by __main__.py when running
+    # pytest) and we're in an xdist worker -- the controller is already
+    # handled by __main__.py, and this same guard is what pytest_configure()
+    # used before, just evaluated earlier.
+    if not (os.environ.get("SLIPCOVER_ENABLED") and _is_xdist_worker()):
         return
 
-    # Check if xdist is being used (look for -n option or xdist plugin config)
-    # Note: PYTEST_XDIST_TESTRUNUID is set later, so we check numprocesses
-    is_xdist = hasattr(config.option, 'numprocesses') and config.option.numprocesses
-
-    # Controller creates shared coverage directory for workers to write to
-    # We detect controller as: xdist is being used AND we're not a worker
-    if is_xdist and not _is_xdist_worker():
-        _coverage_dir = tempfile.mkdtemp(prefix="slipcover-xdist-")
-        os.environ["SLIPCOVER_COVERAGE_DIR"] = _coverage_dir
-        return  # Controller's slipcover is already set up by __main__.py
-
-    # Get coverage directory (workers inherit this from controller)
+    # Coverage directory (workers inherit this from the controller's env)
     _coverage_dir = os.environ.get("SLIPCOVER_COVERAGE_DIR")
 
-    # Only set up slipcover in workers - controller is handled by __main__.py
-    if not _is_xdist_worker():
-        return
-
     # Parse configuration from environment (set by __main__.py)
     branch = os.environ.get("SLIPCOVER_BRANCH") == "1"
     source = os.environ.get("SLIPCOVER_SOURCE")
@@ -97,6 +89,33 @@ def pytest_configure(config):
     _import_manager.__enter__()
 
 
+_activate_worker()
+
+
+def pytest_configure(config):
+    """Sets up the shared coverage directory in the controller.
+
+    Worker activation itself already happened at import time, above -- this
+    hook now only handles the controller side, which does need the `config`
+    object (to detect xdist via config.option.numprocesses).
+    """
+    global _coverage_dir
+
+    # Only activate if SLIPCOVER_ENABLED is set (by __main__.py when running pytest)
+    if not os.environ.get("SLIPCOVER_ENABLED"):
+        return
+
+    # Check if xdist is being used (look for -n option or xdist plugin config)
+    # Note: PYTEST_XDIST_TESTRUNUID is set later, so we check numprocesses
+    is_xdist = hasattr(config.option, 'numprocesses') and config.option.numprocesses
+
+    # Controller creates shared coverage directory for workers to write to
+    # We detect controller as: xdist is being used AND we're not a worker
+    if is_xdist and not _is_xdist_worker():
+        _coverage_dir = tempfile.mkdtemp(prefix="slipcover-xdist-")
+        os.environ["SLIPCOVER_COVERAGE_DIR"] = _coverage_dir
+
+
 def pytest_unconfigure(config):
     """Clean up import manager on shutdown."""
     global _import_manager

tests/test_xdist.py

@@ -219,6 +219,244 @@ def test_xdist_four_workers(tmp_path):
     assert [] == file_cov['missing_lines']
 
 
+# ---------------------------------------------------------------------------
+# Regression tests for modules imported by a conftest.py before pytest_configure()
+# runs in an xdist worker (issue #84's "pre-imported modules" report). Adapted
+# from PR #85 (https://github.com/plasma-umass/slipcover/pull/85, author @nurikk),
+# who first diagnosed this gap and prototyped a fix by retroactively instrumenting
+# already-imported objects after the fact. Unfortunately, that approach has some
+# significant issues: on Python <3.12 the rewritten bytecode wasn't reinstalled,
+# and on 3.12+ branch coverage came back as a false 100% (no AST-level branch
+# preinstrumentation for modules that were already compiled). The fix here takes
+# a different angle: activate ImportManager earlier, before conftest.py is ever
+# read, so these modules go through the normal instrumentation path from the
+# start and need no after-the-fact repair.
+# ---------------------------------------------------------------------------
+
+
+_CONFTEST_PREIMPORT = """\
+import sys
+sys.path.insert(0, str(__import__('pathlib').Path(__file__).resolve().parent))
+import target  # noqa: F401 -- imported before pytest_configure() runs in the worker
+"""
+
+
+def test_xdist_preimported_module_covered(tmp_path, monkeypatch):
+    """A module imported by conftest.py before collection should still be covered."""
+    monkeypatch.chdir(tmp_path)
+
+    (tmp_path / "target.py").write_text("""\
+def greet(name):
+    return f"hello {name}"
+""")
+    (tmp_path / "conftest.py").write_text(_CONFTEST_PREIMPORT)
+    (tmp_path / "test_it.py").write_text("""\
+from target import greet
+
+def test_greet():
+    assert greet("world") == "hello world"
+""")
+
+    out = tmp_path / "out.json"
+    result = subprocess.run(
+        [sys.executable, '-m', 'slipcover', '--source', str(tmp_path),
+         '--json', '--out', str(out),
+         '-m', 'pytest', '-n', '2', '-q', 'test_it.py'],
+        cwd=str(tmp_path), capture_output=True, text=True
+    )
+    assert result.returncode == 0, f"stdout: {result.stdout}\nstderr: {result.stderr}"
+
+    with out.open() as f:
+        cov = json.load(f)
+    check_summaries(cov)
+
+    keys = [k for k in cov['files'] if 'target.py' in k]
+    assert keys, f"target.py not in coverage: {list(cov['files'].keys())}"
+    assert cov['files'][keys[0]]['missing_lines'] == []
+
+
+def test_xdist_preimported_property_covered(tmp_path, monkeypatch):
+    """Property getter bodies in a pre-imported module should be covered."""
+    monkeypatch.chdir(tmp_path)
+
+    (tmp_path / "target.py").write_text("""\
+class Config:
+    @property
+    def name(self):
+        return "test"
+
+    @property
+    def value(self):
+        return 42
+""")
+    (tmp_path / "conftest.py").write_text(_CONFTEST_PREIMPORT)
+    (tmp_path / "test_it.py").write_text("""\
+from target import Config
+
+def test_name():
+    assert Config().name == "test"
+
+def test_value():
+    assert Config().value == 42
+""")
+
+    out = tmp_path / "out.json"
+    result = subprocess.run(
+        [sys.executable, '-m', 'slipcover', '--source', str(tmp_path),
+         '--json', '--out', str(out),
+         '-m', 'pytest', '-n', '2', '-q', 'test_it.py'],
+        cwd=str(tmp_path), capture_output=True, text=True
+    )
+    assert result.returncode == 0, f"stdout: {result.stdout}\nstderr: {result.stderr}"
+
+    with out.open() as f:
+        cov = json.load(f)
+    check_summaries(cov)
+
+    keys = [k for k in cov['files'] if 'target.py' in k]
+    assert keys, f"target.py not in coverage: {list(cov['files'].keys())}"
+    assert cov['files'][keys[0]]['missing_lines'] == []
+
+
+def test_xdist_preimported_wrapped_function_covered(tmp_path, monkeypatch):
+    """functools.wraps-decorated function bodies in a pre-imported module should be covered."""
+    monkeypatch.chdir(tmp_path)
+
+    (tmp_path / "target.py").write_text("""\
+import functools
+
+def decorator(fn):
+    @functools.wraps(fn)
+    def wrapper(*args, **kwargs):
+        return fn(*args, **kwargs)
+    return wrapper
+
+@decorator
+def compute(x):
+    return x * 2
+""")
+    (tmp_path / "conftest.py").write_text(_CONFTEST_PREIMPORT)
+    (tmp_path / "test_it.py").write_text("""\
+from target import compute
+
+def test_compute():
+    assert compute(21) == 42
+""")
+
+    out = tmp_path / "out.json"
+    result = subprocess.run(
+        [sys.executable, '-m', 'slipcover', '--source', str(tmp_path),
+         '--json', '--out', str(out),
+         '-m', 'pytest', '-n', '2', '-q', 'test_it.py'],
+        cwd=str(tmp_path), capture_output=True, text=True
+    )
+    assert result.returncode == 0, f"stdout: {result.stdout}\nstderr: {result.stderr}"
+
+    with out.open() as f:
+        cov = json.load(f)
+    check_summaries(cov)
+
+    keys = [k for k in cov['files'] if 'target.py' in k]
+    assert keys, f"target.py not in coverage: {list(cov['files'].keys())}"
+    # line 6: "return fn(*args, **kwargs)" inside the wrapped function body
+    assert 6 not in cov['files'][keys[0]]['missing_lines']
+
+
+def test_xdist_preimported_nested_attr_covered(tmp_path, monkeypatch):
+    """A function stashed on a nested object attribute in a pre-imported module
+    should be covered -- demonstrating early activation needs no bespoke
+    object-graph walking to handle shapes like this."""
+    monkeypatch.chdir(tmp_path)
+
+    (tmp_path / "target.py").write_text("""\
+class Task:
+    def __init__(self, fn):
+        self.fn = fn
+
+class Workflow:
+    def __init__(self, task):
+        self._task = task
+
+def _impl(x):
+    return x + 1
+
+workflow = Workflow(Task(_impl))
+""")
+    (tmp_path / "conftest.py").write_text(_CONFTEST_PREIMPORT)
+    (tmp_path / "test_it.py").write_text("""\
+from target import workflow
+
+def test_workflow():
+    assert workflow._task.fn(41) == 42
+""")
+
+    out = tmp_path / "out.json"
+    result = subprocess.run(
+        [sys.executable, '-m', 'slipcover', '--source', str(tmp_path),
+         '--json', '--out', str(out),
+         '-m', 'pytest', '-n', '2', '-q', 'test_it.py'],
+        cwd=str(tmp_path), capture_output=True, text=True
+    )
+    assert result.returncode == 0, f"stdout: {result.stdout}\nstderr: {result.stderr}"
+
+    with out.open() as f:
+        cov = json.load(f)
+    check_summaries(cov)
+
+    keys = [k for k in cov['files'] if 'target.py' in k]
+    assert keys, f"target.py not in coverage: {list(cov['files'].keys())}"
+    # line 10: "return x + 1" inside _impl
+    assert 10 not in cov['files'][keys[0]]['missing_lines']
+
+
+def test_xdist_preimported_module_branch_coverage(tmp_path, monkeypatch):
+    """Branch coverage for a pre-imported module must reflect real, not vacuous,
+    coverage: only the branch actually exercised should be covered, and the
+    untaken branch must show up in missing_branches rather than a false 100%."""
+    monkeypatch.chdir(tmp_path)
+
+    (tmp_path / "target.py").write_text("""\
+def check(x):
+    if x > 0:
+        return "positive"
+    else:
+        return "non-positive"
+""")
+    (tmp_path / "conftest.py").write_text(_CONFTEST_PREIMPORT)
+    (tmp_path / "test_it.py").write_text("""\
+from target import check
+
+def test_positive():
+    assert check(1) == "positive"
+""")
+
+    out = tmp_path / "out.json"
+    result = subprocess.run(
+        [sys.executable, '-m', 'slipcover', '--branch', '--source', str(tmp_path),
+         '--json', '--out', str(out),
+         '-m', 'pytest', '-n', '2', '-q', 'test_it.py'],
+        cwd=str(tmp_path), capture_output=True, text=True
+    )
+    assert result.returncode == 0, f"stdout: {result.stdout}\nstderr: {result.stderr}"
+
+    with out.open() as f:
+        cov = json.load(f)
+    check_summaries(cov)
+
+    keys = [k for k in cov['files'] if 'target.py' in k]
+    assert keys, f"target.py not in coverage: {list(cov['files'].keys())}"
+    module_cov = cov['files'][keys[0]]
+
+    executed_branches = [tuple(b) for b in module_cov.get('executed_branches', [])]
+    missing_branches = [tuple(b) for b in module_cov.get('missing_branches', [])]
+
+    # Only the true branch (line 2 -> line 3) was exercised; the else branch
+    # (line 2 -> line 5) was never taken and must show up as missing -- a vacuous
+    # "0 missing branches" here is exactly the false-100% bug being guarded against.
+    assert (2, 3) in executed_branches, f"true branch not covered: {executed_branches}"
+    assert (2, 5) in missing_branches, f"else branch should be missing: {missing_branches}"
+
+
 def test_xdist_fail_under_uses_merged_coverage(tmp_path):
     """--fail-under must be checked against the merged (all-workers) coverage,
     not just the coordinator process' own view. Under xdist, actual test code