ci(python-unit): narrow the driver preload; isolate the MCP user ContextVar

Addresses the first CI run and a three-lens cross-model review.

CI: the parallel step ran in 14m07s on the 4-core runners with one failure,
tests/unit_tests/mcp_service/test_middleware_logging.py (`assert 1 == 42`).
That is a leaked ContextVar, not xdist: sync tests that call
`_setup_user_context()` (test_gauge_chart's resource path resolves a real
user, id 1; six test_auth_user_resolution tests leave mocks) set
`_mcp_user_id_var` in the thread's base context and never clear it, and
pytest-asyncio copies that context into every later async test's task.
`on_call_tool`'s `finally` then overrides the patched user id with the leaked
one. It passed serially only because test_mcp_e2e_smoke sorts between leaker
and victim and exercises the real middleware, whose `finally` resets the var;
`loadfile` put that file on another worker. A per-test reset fixture in
tests/unit_tests/mcp_service/conftest.py scopes the var to each test.
Control: gauge -> middleware_logging in one process reproduces the exact
failure without the fixture and passes with it.

Review (unanimous across lenses): the session-wide `load_engine_specs()`
preload was the wrong seam -- full discovery, including third-party entry
points, in every worker and both serial coverage gates, and importing
Superset's ClickHouse spec before any app exists caches `product_name="dev"`
for the session. Narrowed to pre-importing only `clickhouse_connect`, the
third-party module that actually reads the clock at import; engine-spec
discovery itself stays lazy, so tests that exercise it still see a cold
state. Seven unit-test files combine freeze_time with engine-spec
resolution, so a narrow autouse preload beats per-test opt-in. A direct pin
of the preload's contract is added. Also from review: docstring opening line
and deferred-import justification per the constitution; explicit annotations
on the tuple constants; `--durations=0` so the pre-existing `--durations-min`
actually emits a report (it only filters an enabled one) -- which immediately
shows the critical path under loadfile: one 68s test and ~15s of
module-scoped app setup per file; and a note on `--maxfail` semantics.

Local full run, 8 workers, coverage on: 15,128 passed in 3m36s.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EA4MGEkUcDpyLReqjQBhPy

Author: Mike Bridge

.github/workflows/superset-python-unittest.yml

@@ -69,12 +69,17 @@ jobs:
       # even though every test passed. `--dist loadfile` keeps all tests from
       # one file on one worker, which preserves module-scoped fixture state;
       # pytest-cov combines the per-worker coverage data automatically.
+      # `--durations=0` makes `--durations-min` effective (the min flag only
+      # filters an enabled report) so the slow-file critical path under
+      # loadfile stays visible. `--maxfail` is a controller-level stop: tests
+      # already dispatched to workers finish, so the final failure count can
+      # slightly exceed it.
       - name: Python unit tests
         env:
           SUPERSET_TESTENV: true
           SUPERSET_SECRET_KEY: not-a-secret
         run: |
-          pytest -n auto --dist loadfile --durations-min=0.5 --cov-report= --cov=superset ./tests/common ./tests/unit_tests --cache-clear --maxfail=50 --junit-xml=test-results/junit-unit.xml
+          pytest -n auto --dist loadfile --durations=0 --durations-min=0.5 --cov-report= --cov=superset ./tests/common ./tests/unit_tests --cache-clear --maxfail=50 --junit-xml=test-results/junit-unit.xml
       # COVERAGE_FILE keeps these scoped gates off the default .coverage that
       # the step above wrote. pytest-cov starts a fresh data file per run, so
       # without it the last gate replaces the full-suite data and the report

tests/unit_tests/commands/report/base_test.py

@@ -37,12 +37,12 @@
 # requires every worker to collect the same test ids in the same order. A set
 # of strings iterates in per-process hash order (PYTHONHASHSEED), so a set here
 # makes workers disagree and xdist aborts with "Different tests were collected".
-REPORT_TYPES = (
+REPORT_TYPES: tuple[ReportScheduleType, ...] = (
     ReportScheduleType.ALERT,
     ReportScheduleType.REPORT,
 )
 
-TEST_SCHEDULES_EVERY_MINUTE = (
+TEST_SCHEDULES_EVERY_MINUTE: tuple[str, ...] = (
     "* * * * *",
     "1-5 * * * *",
     "10-20 * * * *",
@@ -51,13 +51,15 @@
     "10,20,30,40-45 * * * *",
 )
 
-TEST_SCHEDULES_SINGLE_MINUTES = (
+TEST_SCHEDULES_SINGLE_MINUTES: tuple[str, ...] = (
     "1,5,8,10,12 * * * *",
     "10 1 * * *",
     "27,2 1-5 * * *",
 )
 
-TEST_SCHEDULES = TEST_SCHEDULES_EVERY_MINUTE + TEST_SCHEDULES_SINGLE_MINUTES
+TEST_SCHEDULES: tuple[str, ...] = (
+    TEST_SCHEDULES_EVERY_MINUTE + TEST_SCHEDULES_SINGLE_MINUTES
+)
 
 
 def dynamic_alert_minimum_interval(**kwargs) -> int:

tests/unit_tests/conftest.py

@@ -74,23 +74,34 @@ def session(get_session) -> Iterator[Session]:
 
 
 @pytest.fixture(scope="session", autouse=True)
-def _preload_engine_specs() -> None:
+def _preload_clock_reading_drivers() -> None:
+    """Import third-party drivers that read the clock at import, before any test.
+
+    ``clickhouse_connect`` normalises the local timezone through ``dateutil``
+    at module import time, which raises ``AttributeError`` under a frozen
+    ``freeze_time`` clock. Superset's ClickHouse engine spec imports it
+    lazily, on the first ``load_engine_specs()`` call, so whichever test
+    first resolves an engine spec pays that import -- and if that test runs
+    under ``freeze_time`` (seven unit-test files combine ``freeze_time`` with
+    engine-spec resolution) the import fails. Serially some earlier test
+    always paid it outside a frozen clock; under pytest-xdist each worker
+    starts cold, so the failure moves with worker assignment.
+
+    Only the third-party module is imported here, deliberately: importing
+    Superset's own engine-spec modules this early would also cache
+    app-context-dependent values (the ClickHouse spec resolves its
+    ``product_name`` from ``current_app`` on import), and running full
+    ``load_engine_specs()`` discovery would execute every registered
+    third-party entry point in every worker. Engine-spec discovery itself
+    stays lazy, so tests that exercise it still observe a cold state.
     """
-    Import every engine-spec module once per test session, before any test.
-
-    ``load_engine_specs()`` imports the spec modules lazily on first use, and
-    some of them pull in third-party drivers that read the local timezone at
-    import time (``clickhouse_connect`` via ``dateutil``). That import fails
-    under a ``freeze_time`` clock. In a serial run some earlier test always
-    paid the import cost outside a frozen clock, so the dependency was
-    invisible; under pytest-xdist every worker starts cold, and whichever
-    ``freeze_time`` test is the first engine-spec importer on its worker
-    fails -- a flake that moves with worker assignment. Loading here gives
-    every worker the same import state a serial run reached by accident.
-    """
-    from superset.db_engine_specs import load_engine_specs
-
-    load_engine_specs()
+    # Deferred, guarded import: the driver is an optional dependency, so a
+    # module-top import would make this conftest fail to load in an
+    # environment where it is not installed.
+    try:
+        import clickhouse_connect  # noqa: F401
+    except ImportError:
+        pass
 
 
 @pytest.fixture(scope="module")

tests/unit_tests/mcp_service/conftest.py

@@ -22,8 +22,34 @@
 RBAC logic is tested directly in test_auth_rbac.py.
 """
 
+from collections.abc import Iterator
+
 import pytest
 
+from superset.mcp_service.auth import _mcp_user_id_var
+
+
+@pytest.fixture(autouse=True)
+def isolate_mcp_user_id_var() -> Iterator[None]:
+    """Reset the ``_mcp_user_id_var`` ContextVar around every MCP test.
+
+    ``_setup_user_context()`` sets the var and relies on ``on_call_tool``'s
+    ``finally`` to clear it. Tests that call ``_setup_user_context()`` (or set
+    the var) directly, without going through the middleware, leave it set in
+    the thread's base context, and pytest-asyncio copies that context into
+    every later async test's task -- so a resolved user id leaks into
+    unrelated tests. In a serial run a middleware-exercising test file that
+    sorts between the leaking file and its victim happened to clear the var;
+    under pytest-xdist the files land on different workers and the leak
+    surfaces as a flake. Scoping the var to each test removes the ordering
+    dependence.
+    """
+    token = _mcp_user_id_var.set(None)
+    try:
+        yield
+    finally:
+        _mcp_user_id_var.reset(token)
+
 
 @pytest.fixture(autouse=True)
 def disable_mcp_rbac(app):

tests/unit_tests/test_clock_reading_driver_preload.py

@@ -0,0 +1,50 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Pin the harness guarantee behind ``_preload_clock_reading_drivers``.
+
+``clickhouse_connect`` reads the local timezone at import time and fails
+under a ``freeze_time`` clock, so the unit-test session imports it before any
+test runs. A test can therefore resolve the ClickHouse engine spec under a
+frozen clock on any worker, regardless of what ran before it. This test
+asserts that guarantee directly; if the preload fixture is removed, it fails
+on any worker where nothing else happened to import the driver first.
+"""
+
+import importlib.util
+import sys
+
+import pytest
+from freezegun import freeze_time
+
+
+@pytest.mark.skipif(
+    importlib.util.find_spec("clickhouse_connect") is None,
+    reason="clickhouse_connect is not installed",
+)
+def test_clock_reading_driver_is_preloaded_before_tests() -> None:
+    assert "clickhouse_connect" in sys.modules
+
+
+@pytest.mark.skipif(
+    importlib.util.find_spec("clickhouse_connect") is None,
+    reason="clickhouse_connect is not installed",
+)
+@freeze_time("2021-04-01T00:00:00Z")
+def test_clickhouse_engine_spec_resolves_under_frozen_clock() -> None:
+    from superset.db_engine_specs import get_engine_spec
+
+    assert get_engine_spec("clickhousedb", "connect") is not None