From 49c8a721036c23c0aaf3212841fea671d34c59fc Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Fri, 4 Sep 2026 14:07:35 +0800 Subject: [PATCH] fix: make the patched get_default_project_directory order-safe in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A product module first-imported while run_around_tests' autouse monkeypatch of gns3server.utils.path.get_default_project_directory is active (e.g. 'from gns3server.api.server import app' inside a test body) binds the patched lambda into its own namespace forever: module-level from-imports capture the object by value. Once the patch is reverted, that module keeps calling the stale lambda, which closed over the very test's tmppath — a directory deleted at that test's teardown. Every later test hitting psutil.disk_usage(get_default_project_directory()) then fails with FileNotFoundError, but only when the importing test runs before the API test files (full-suite collection order hides it). The replacement now resolves Config.instance().settings at call time instead of closing over the tmppath, so a frozen reference stays correct; the test that triggered this imports gns3_app at module top so no product import ever happens inside a patched window. Also pins pytest-random-order (inert without --random-order) and documents the order-independence rules for new tests in the gns3-api-test-writing skill, including the known-red legacy files whose tests share rows sequentially. --- .claude/skills/gns3-api-test-writing/SKILL.md | 15 +++++++++++++++ dev-requirements.txt | 3 ++- tests/conftest.py | 8 +++++++- tests/controller/test_compute.py | 5 ++++- 4 files changed, 28 insertions(+), 3 deletions(-) diff --git a/.claude/skills/gns3-api-test-writing/SKILL.md b/.claude/skills/gns3-api-test-writing/SKILL.md index 6149bf21f..991df03f2 100644 --- a/.claude/skills/gns3-api-test-writing/SKILL.md +++ b/.claude/skills/gns3-api-test-writing/SKILL.md @@ -56,3 +56,18 @@ Always pass `secret_key=DEFAULT_JWT_SECRET_KEY`: the autouse `run_around_tests` ## Failure-Diagnosis Heuristic **Passes in isolation, fails in a class run → suspect shared fixture state first** (`base_client.headers`, the `Config` singleton, class-scoped DB rows) — never the product code. Reproduce with `-k "test_a or test_b"` pairs to find the polluting test. Do not add debug prints to product code to chase test-order issues; make the test order-independent with explicit per-request headers instead. + +## Order Independence + +The suite runs in collection order by default, and a full run is green — but that hides order dependencies. Two real incident classes so far: + +1. **The frozen from-import** (fixed 2026-09): `run_around_tests` monkeypatches `gns3server.utils.path.get_default_project_directory` with a lambda. A product module first-imported *while that patch is active* (e.g. `from gns3server.api.server import app` written inside a test body) freezes the patched lambda into its namespace forever — later tests then get a deleted tmpdir path (`FileNotFoundError` from `psutil.disk_usage`). The patched lambda now resolves `Config.instance()` at call time, so freezing is benign — keep it that way. +2. **Sequential-scenario DB tests**: `tests/api/routes/controller/test_users.py`, `test_roles.py`, `test_pools.py`, `test_templates.py`, `test_images.py`, `test_groups.py`, `test_appliances.py`, `test_acl.py` and `tests/controller/test_rbac.py` build shared rows across tests within a file (a test asserts on users/roles created by earlier tests). They are known-red under any reordering — do not copy this pattern into new files. + +### Rules for new tests + +- **Import product modules at test-module top level**, never first-import inside a test body (an autouse fixture's monkeypatches are live there, and module import executes product `from`-imports). +- **Autouse patch replacements must resolve state at call time** (`Config.instance().settings...`), never close over test-local values (tmppaths, fixture objects) — a closed-over value survives into other tests if the replacement object gets frozen anywhere. +- **Verify a new test file is order-independent**: `venv/bin/python -m pytest tests/.py --random-order --random-order-seed=1 -q` (and a second seed). It must pass shuffled. `pytest-random-order` is pinned in `dev-requirements.txt`; it is inert unless `--random-order` is passed. +- A test that needs specific rows creates them itself (or via a fixture) — never relies on rows another test in the file created. +- Diagnosing a suspected order bug: rerun the exact failing pair with the seed printed by `--random-order` (`--random-order-seed=` reproduces it), then bisect to the polluting test. diff --git a/dev-requirements.txt b/dev-requirements.txt index 55aac6eaa..80d7da3b4 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -3,4 +3,5 @@ flake8==7.3.0 pytest-timeout==2.4.0 pytest-asyncio==1.4.0 httpx==0.28.1 -httpx_ws==0.7.2 # upgrading leads to failures in tests \ No newline at end of file +httpx_ws==0.7.2 # upgrading leads to failures in tests +pytest-random-order==1.2.0 # opt-in: --random-order --random-order-bucket=global (legacy suites are not shuffle-clean yet, see .claude/skills/gns3-api-test-writing) \ No newline at end of file diff --git a/tests/conftest.py b/tests/conftest.py index f67b04839..b0994ebb8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -418,7 +418,13 @@ def run_around_tests(monkeypatch, config, port_manager): # avoid monitoring for new images while testing config.settings.Server.auto_discover_images = False - monkeypatch.setattr("gns3server.utils.path.get_default_project_directory", lambda *args: os.path.join(tmppath, 'projects')) + # Resolve the projects directory from the Config singleton at call time instead of + # closing over this test's tmppath: a from-import executed anywhere while this + # patch is active (e.g. the first import of gns3server.api.server from inside a + # test body) freezes the patched object into the importing module's namespace + # forever, and a closed-over path would then point at a deleted directory in + # every later test (order-dependent FileNotFoundError in psutil.disk_usage). + monkeypatch.setattr("gns3server.utils.path.get_default_project_directory", lambda *args: Config.instance().settings.Server.projects_path) # Force sys.platform to the original value. Because it seems not be restored correctly after each test sys.platform = sys.original_platform diff --git a/tests/controller/test_compute.py b/tests/controller/test_compute.py index 58ba606c4..671b8e7a0 100644 --- a/tests/controller/test_compute.py +++ b/tests/controller/test_compute.py @@ -25,6 +25,7 @@ from unittest.mock import patch, MagicMock from gns3server.controller.project import Project from gns3server.controller.compute import Compute +from gns3server.api.server import app as gns3_app from gns3server.controller.controller_error import ( ControllerError, ControllerNotFoundError, @@ -581,8 +582,10 @@ async def test_connect_notification_poison_frame_autoreconnects(compute, monkeyp compute._http_session = session # allow the reconnection to be scheduled during the test + # (import gns3_app at module top: a first import from inside a test body + # would execute module-level from-imports while the autouse fixture's + # monkeypatches are active, freezing patched objects into namespaces) monkeypatch.delattr(sys, "_called_from_test", raising=False) - from gns3server.api.server import app as gns3_app monkeypatch.setattr(gns3_app.state, "exiting", False) async def fake_connect():