fix(plugins): preserve PluginRegistry across DATALAB_PLUGINS unit tests (#311)

Author: PierreRaybaut

datalab/config.py

@@ -9,6 +9,7 @@
 
 from __future__ import annotations
 
+import logging
 import os
 import os.path as osp
 import sys
@@ -118,29 +119,53 @@ def is_frozen(module_name: str) -> bool:
 #: (subset of :data:`OTHER_PLUGINS_PATHLIST`, kept around so that consumers
 #: such as the plugin configuration dialog can flag them as user-provided).
 DATALAB_PLUGINS_ENV_PATHS: list[str] = []
-_env_plugins = os.environ.get(DATALAB_PLUGINS_ENV_VAR, "")
-if _env_plugins:
-    import logging as _logging  # local import to avoid polluting module namespace
-
-    _logger = _logging.getLogger(__name__)
-    for _raw_path in _env_plugins.split(os.pathsep):
-        _path = _raw_path.strip()
-        if not _path:
+
+
+def parse_datalab_plugins_env_var(
+    env_value: str | None,
+    pathlist: list[str],
+    env_paths: list[str],
+) -> None:
+    """Parse ``DATALAB_PLUGINS`` and append valid directories to ``pathlist``.
+
+    Args:
+        env_value: Raw value of the ``DATALAB_PLUGINS`` environment variable
+         (``None`` or empty string is a no-op).
+        pathlist: Plugin search path list to extend in-place
+         (typically :data:`OTHER_PLUGINS_PATHLIST`).
+        env_paths: List of env-var-provided directories to extend in-place
+         (typically :data:`DATALAB_PLUGINS_ENV_PATHS`), used by the GUI to
+         flag entries originating from the environment variable.
+    """
+    if not env_value:
+        return
+
+    logger = logging.getLogger(__name__)
+    for raw_path in env_value.split(os.pathsep):
+        path = raw_path.strip()
+        if not path:
             continue
-        _path = osp.normpath(osp.expanduser(_path))
-        if osp.isdir(_path):
-            if _path not in OTHER_PLUGINS_PATHLIST:
-                OTHER_PLUGINS_PATHLIST.append(_path)
-            if _path not in DATALAB_PLUGINS_ENV_PATHS:
-                DATALAB_PLUGINS_ENV_PATHS.append(_path)
+        path = osp.normpath(osp.expanduser(path))
+        if osp.isdir(path):
+            if path not in pathlist:
+                pathlist.append(path)
+            if path not in env_paths:
+                env_paths.append(path)
         else:
-            _logger.warning(
+            logger.warning(
                 "%s: ignoring non-existent plugin directory '%s'",
                 DATALAB_PLUGINS_ENV_VAR,
-                _path,
+                path,
             )
 
 
+parse_datalab_plugins_env_var(
+    os.environ.get(DATALAB_PLUGINS_ENV_VAR),
+    OTHER_PLUGINS_PATHLIST,
+    DATALAB_PLUGINS_ENV_PATHS,
+)
+
+
 def get_mod_source_dir() -> str | None:
     """Return module source directory
 

datalab/tests/backbone/plugins_envvar_unit_test.py

@@ -7,67 +7,72 @@
 into entries of :data:`datalab.config.OTHER_PLUGINS_PATHLIST` and verifies
 that :func:`datalab.plugins.discover_plugins` actually finds plugin modules
 placed in those directories.
+
+The parsing function is tested directly (without reloading
+``datalab.config``) to avoid corrupting global singletons (``Conf``,
+``PluginRegistry``, the ``PluginBase`` metaclass) shared with the rest of
+the test session.
 """
 
 # guitest: skip
 
 from __future__ import annotations
 
-import importlib
 import os
 import sys
 import textwrap
 from pathlib import Path
 
 import pytest
 
+from datalab import config as config_mod
+from datalab import plugins as plugins_mod
 
-def _reload_config(monkeypatch: pytest.MonkeyPatch, env_value: str | None) -> object:
-    """Reload ``datalab.config`` with a controlled ``DATALAB_PLUGINS`` value."""
-    if env_value is None:
-        monkeypatch.delenv("DATALAB_PLUGINS", raising=False)
-    else:
-        monkeypatch.setenv("DATALAB_PLUGINS", env_value)
-    import datalab.config as config_mod
 
-    return importlib.reload(config_mod)
+def test_env_var_unset_leaves_default_paths() -> None:
+    """When the env var is empty/unset, the path list is left untouched."""
+    pathlist: list[str] = ["/initial/path"]
+    env_paths: list[str] = []
 
+    config_mod.parse_datalab_plugins_env_var(None, pathlist, env_paths)
+    config_mod.parse_datalab_plugins_env_var("", pathlist, env_paths)
 
-def test_env_var_unset_leaves_default_paths(monkeypatch: pytest.MonkeyPatch) -> None:
-    """When the env var is unset, the default plugin path list is unchanged."""
-    config_mod = _reload_config(monkeypatch, None)
-    assert config_mod.OTHER_PLUGINS_PATHLIST  # at least the bundled directory
+    assert pathlist == ["/initial/path"]
+    assert env_paths == []
 
 
-def test_env_var_adds_existing_directories(
-    monkeypatch: pytest.MonkeyPatch, tmp_path: Path
-) -> None:
-    """Existing directories listed in the env var are appended to the path list."""
+def test_env_var_adds_existing_directories(tmp_path: Path) -> None:
+    """Existing directories listed in the env var are appended to both lists."""
     dir1 = tmp_path / "plugins_a"
     dir2 = tmp_path / "plugins_b"
     dir1.mkdir()
     dir2.mkdir()
     env_value = os.pathsep.join([str(dir1), str(dir2)])
 
-    config_mod = _reload_config(monkeypatch, env_value)
-    paths = [os.path.normpath(p) for p in config_mod.OTHER_PLUGINS_PATHLIST]
-    assert os.path.normpath(str(dir1)) in paths
-    assert os.path.normpath(str(dir2)) in paths
+    pathlist: list[str] = []
+    env_paths: list[str] = []
+    config_mod.parse_datalab_plugins_env_var(env_value, pathlist, env_paths)
+
+    expected = [os.path.normpath(str(dir1)), os.path.normpath(str(dir2))]
+    assert [os.path.normpath(p) for p in pathlist] == expected
+    assert [os.path.normpath(p) for p in env_paths] == expected
 
 
 def test_env_var_skips_missing_directories(
-    monkeypatch: pytest.MonkeyPatch, tmp_path: Path, caplog: pytest.LogCaptureFixture
+    tmp_path: Path, caplog: pytest.LogCaptureFixture
 ) -> None:
-    """Missing directories are skipped silently and a warning is logged."""
+    """Missing directories are skipped and a warning is logged."""
     existing = tmp_path / "exists"
     existing.mkdir()
     missing = tmp_path / "does_not_exist"
     env_value = os.pathsep.join([str(existing), str(missing), "  "])
 
+    pathlist: list[str] = []
+    env_paths: list[str] = []
     with caplog.at_level("WARNING", logger="datalab.config"):
-        config_mod = _reload_config(monkeypatch, env_value)
+        config_mod.parse_datalab_plugins_env_var(env_value, pathlist, env_paths)
 
-    paths = [os.path.normpath(p) for p in config_mod.OTHER_PLUGINS_PATHLIST]
+    paths = [os.path.normpath(p) for p in pathlist]
     assert os.path.normpath(str(existing)) in paths
     assert os.path.normpath(str(missing)) not in paths
     assert any("does_not_exist" in r.message for r in caplog.records)
@@ -76,7 +81,12 @@ def test_env_var_skips_missing_directories(
 def test_discover_plugins_finds_module_in_env_dir(
     monkeypatch: pytest.MonkeyPatch, tmp_path: Path
 ) -> None:
-    """``datalab_*`` module in a ``DATALAB_PLUGINS`` directory is discovered."""
+    """A ``datalab_*`` module in an env-var directory is discovered.
+
+    ``OTHER_PLUGINS_PATHLIST`` is mutated in-place (and restored after) to
+    avoid reloading ``datalab.config`` or ``datalab.plugins``, which would
+    invalidate the ``PluginBase`` metaclass and break later tests.
+    """
     plugin_dir = tmp_path / "extra_plugins"
     plugin_dir.mkdir()
     plugin_name = "datalab_envvar_discovery_probe"
@@ -91,22 +101,22 @@ def test_discover_plugins_finds_module_in_env_dir(
         encoding="utf-8",
     )
 
-    monkeypatch.setenv("DATALAB_PLUGINS", str(plugin_dir))
-    # Ensure config picks up the env var, then reload plugins module so it
-    # imports the refreshed OTHER_PLUGINS_PATHLIST.
-    importlib.reload(importlib.import_module("datalab.config"))
-    plugins_mod = importlib.reload(importlib.import_module("datalab.plugins"))
-
-    # Make sure plugins are enabled for discovery
-    from datalab.config import Conf
-
-    monkeypatch.setattr(Conf.main.plugins_enabled, "get", lambda *a, **kw: True)
-
+    extra_path = str(plugin_dir)
+    config_mod.OTHER_PLUGINS_PATHLIST.append(extra_path)
+    sys.modules.pop(plugin_name, None)
     try:
+        # Ensure plugins are considered enabled regardless of user config
+        monkeypatch.setattr(
+            config_mod.Conf.main.plugins_enabled, "get", lambda *a, **kw: True
+        )
         modules = plugins_mod.discover_plugins()
         discovered_names = {m.__name__ for m in modules}
         assert plugin_name in discovered_names
     finally:
+        try:
+            config_mod.OTHER_PLUGINS_PATHLIST.remove(extra_path)
+        except ValueError:
+            pass
         sys.modules.pop(plugin_name, None)