Compare commits

..
6 changed files with 93 additions and 161 deletions
+4 -2
View File
@@ -99,8 +99,10 @@ cleanup. Specifically:
`os.environ[...] = ...` that outlives the test. `os.environ[...] = ...` that outlives the test.
- **Current working directory** - never `chdir` without restoring; never assert - **Current working directory** - never `chdir` without restoring; never assert
against cwd-relative paths like `./data`. Use a temp workspace helper instead. against cwd-relative paths like `./data`. Use a temp workspace helper instead.
- **Database** - the root `conftest.py` defaults `DATABASE_URL` to an in-memory - **Database** - the root `conftest.py` defaults `DATABASE_URL` to a private,
SQLite for collection safety. A test that needs a real file-backed DB must opt worker-owned SQLite file for collection safety and consistent schema across
TestClient threads. This fallback is not a per-test data fixture. A test that
writes or depends on particular rows must opt
in explicitly via `tests.helpers.sqlite_db.make_temp_sqlite` and bind its in explicitly via `tests.helpers.sqlite_db.make_temp_sqlite` and bind its
`SessionLocal` onto the module under test. Do not rely on a persistent `SessionLocal` onto the module under test. Do not rely on a persistent
on-disk DB existing. on-disk DB existing.
+14 -1
View File
@@ -12,9 +12,17 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
# File-backed databases remain fixture-owned. Cleanup restores the caller. # File-backed databases remain fixture-owned. Cleanup restores the caller.
# Keep the existing generated environment-reference locations stable. # Keep the existing generated environment-reference locations stable.
_database_environment = pytest.MonkeyPatch() _database_environment = pytest.MonkeyPatch()
_database_environment.setenv("DATABASE_URL", "sqlite:///:memory:")
from tests.helpers.worker_runtime import bootstrap_runtime, configure_runtime from tests.helpers.worker_runtime import bootstrap_runtime, configure_runtime
_runtime_environment = bootstrap_runtime() _runtime_environment = bootstrap_runtime()
# A file keeps the collection schema consistent across TestClient threads;
# SingletonThreadPool can evict the populated in-memory main-thread database.
# One patcher owns this variable, including restoration of an inherited URL.
_database_environment.setenv(
"DATABASE_URL",
f"sqlite:///{_runtime_environment.root / 'collection.db'}"
if _runtime_environment is not None else "sqlite:///:memory:",
)
_collection_database_engine = None
# Pre-import real heavy modules BEFORE any test file's module-level stubs can # Pre-import real heavy modules BEFORE any test file's module-level stubs can
# replace them with MagicMock. Some test files (e.g. test_llm_core_sanitize_*) # replace them with MagicMock. Some test files (e.g. test_llm_core_sanitize_*)
@@ -26,6 +34,7 @@ try:
import sqlalchemy.orm # noqa: F401 import sqlalchemy.orm # noqa: F401
import core.database # noqa: F401 import core.database # noqa: F401
import src.database import src.database
_collection_database_engine = core.database.engine
except ImportError: except ImportError:
pass # not installed - the stubs below will handle it pass # not installed - the stubs below will handle it
@@ -100,6 +109,10 @@ def pytest_configure(config):
only registers marker names; it imports no production module. only registers marker names; it imports no production module.
""" """
config.add_cleanup(_database_environment.undo) config.add_cleanup(_database_environment.undo)
if _collection_database_engine is not None:
# Registered after runtime cleanup, so connections close before the
# owned file is removed (also required on Windows).
config.add_cleanup(_collection_database_engine.dispose)
import pathlib import pathlib
from tests._taxonomy import discover_markers from tests._taxonomy import discover_markers
+11 -2
View File
@@ -85,6 +85,9 @@ def test_actual_tests_restore_process_state_and_ignore_inherited_database(tmp_pa
import os import os
import sys import sys
import pytest import pytest
import tempfile
from pathlib import Path
from sqlalchemy.engine import make_url
def snapshot(): def snapshot():
import core import core
@@ -107,7 +110,11 @@ def test_actual_tests_restore_process_state_and_ignore_inherited_database(tmp_pa
class StateGuard: class StateGuard:
def pytest_sessionstart(self): def pytest_sessionstart(self):
self.initial = snapshot() self.initial = snapshot()
assert self.initial[0] == "sqlite:///:memory:" assert self.initial[0] != inherited_url
self.default_path = Path(make_url(self.initial[0]).database)
assert self.default_path.is_file()
assert self.default_path.name == "collection.db"
assert self.default_path.parent == Path(tempfile.gettempdir()).parent
def pytest_collection_finish(self): def pytest_collection_finish(self):
assert snapshot() == self.initial, "collection changed database bindings" assert snapshot() == self.initial, "collection changed database bindings"
@@ -118,9 +125,11 @@ def test_actual_tests_restore_process_state_and_ignore_inherited_database(tmp_pa
assert snapshot() == self.initial, "test leaked database or module state" assert snapshot() == self.initial, "test leaked database or module state"
inherited_url = os.environ["DATABASE_URL"] inherited_url = os.environ["DATABASE_URL"]
guard = StateGuard()
result = pytest.main(["-q", "-p", "no:cacheprovider", *sys.argv[1:]], result = pytest.main(["-q", "-p", "no:cacheprovider", *sys.argv[1:]],
plugins=[StateGuard()]) plugins=[guard])
assert os.environ["DATABASE_URL"] == inherited_url assert os.environ["DATABASE_URL"] == inherited_url
assert not guard.default_path.exists(), "collection database outlived its runtime"
raise SystemExit(result) raise SystemExit(result)
''') ''')
result = subprocess.run( result = subprocess.run(
@@ -93,7 +93,7 @@ def test_custom_filter_reuses_convolve_kernel():
assert "convolve(readSource(snap), k, divisor, bias, 1)" in EFFECTS assert "convolve(readSource(snap), k, divisor, bias, 1)" in EFFECTS
def test_blur_and_custom_filter_aliases_registered(): def test_new_filter_aliases_registered():
assert "'box-blur': 'blur-box'" in EDITOR assert "'box-blur': 'blur-box'" in EDITOR
assert "'surface-blur': 'blur-surface'" in EDITOR assert "'surface-blur': 'blur-surface'" in EDITOR
assert "custom: 'other-custom'" in EDITOR assert "custom: 'other-custom'" in EDITOR
+34 -155
View File
@@ -12,8 +12,6 @@ exercising every procfs-touching code path on both a Linux and a non-Linux
host. ``test_cookbook_stop_without_procfs.py`` covers the behaviour itself. host. ``test_cookbook_stop_without_procfs.py`` covers the behaviour itself.
""" """
import ast import ast
import pytest
from pathlib import Path from pathlib import Path
REPO_ROOT = Path(__file__).resolve().parent.parent REPO_ROOT = Path(__file__).resolve().parent.parent
@@ -77,112 +75,37 @@ def _scan_sites(tree: ast.AST) -> list[ast.Call]:
return sites return sites
def _condition_requires_procfs(node: ast.AST, truth: bool) -> bool: def _guard_lines(tree: ast.AST) -> list[int]:
"""Whether this Boolean outcome proves a direct procfs check succeeded. """Line numbers of calls that test whether procfs is present."""
lines = []
An ignored call, another function's check, or a check on only one possible for node in ast.walk(tree):
OR branch provides no proof. Keep unknown expressions conservative. if not isinstance(node, ast.Call):
""" continue
if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.Not): func = node.func
return _condition_requires_procfs(node.operand, not truth) name = func.attr if isinstance(func, ast.Attribute) else getattr(func, "id", "")
if isinstance(node, ast.BoolOp): if name not in _GUARD_FUNCS:
facts = [_condition_requires_procfs(value, truth) for value in node.values] continue
all_values_required = isinstance(node.op, ast.And) == truth subject_is_procfs = (
return any(facts) if all_values_required else all(facts) name in {"has_procfs", "is_wsl"}
if not isinstance(node, ast.Call): or (isinstance(func, ast.Attribute) and _is_procfs_root(func.value))
return False or any(_is_procfs_root(arg) for arg in node.args)
func = node.func )
name = func.attr if isinstance(func, ast.Attribute) else getattr(func, "id", "") if subject_is_procfs:
subject_is_procfs = ( lines.append(node.lineno)
name == "has_procfs" return lines
or (isinstance(func, ast.Attribute) and _is_procfs_root(func.value))
or any(_is_procfs_root(arg) for arg in node.args)
)
return truth and name in _GUARD_FUNCS and subject_is_procfs
def _guarded_scan_ids(tree: ast.AST) -> set[int]: def _enclosing_scope(tree: ast.AST, node: ast.AST) -> ast.AST:
"""Follow conditional/early-exit paths instead of comparing line numbers. """Smallest function/module scope containing ``node``."""
best = tree
Each function starts with no procfs assumption. Facts from a loop or try for candidate in ast.walk(tree):
body cannot escape to code which may run without that body completing. if not isinstance(candidate, (ast.FunctionDef, ast.AsyncFunctionDef)):
This intentionally requires direct checks; arbitrary aliases/call graphs continue
are not a proof that procfs exists at the enumeration site. end = getattr(candidate, "end_lineno", None) or candidate.lineno
""" if candidate.lineno <= node.lineno <= end:
scans = {id(site) for site in _scan_sites(tree)} if best is tree or candidate.lineno > best.lineno:
guarded = set() best = candidate
return best
def expression(node, present):
if node is None:
return
if id(node) in scans and present:
guarded.add(id(node))
if isinstance(node, ast.BoolOp):
for value in node.values:
expression(value, present)
# Later AND operands run only after a true prefix; later OR
# operands run only after a false prefix.
present = present or _condition_requires_procfs(value, isinstance(node.op, ast.And))
elif isinstance(node, ast.IfExp):
expression(node.test, present)
expression(node.body, present or _condition_requires_procfs(node.test, True))
expression(node.orelse, present or _condition_requires_procfs(node.test, False))
elif isinstance(node, (ast.ListComp, ast.SetComp, ast.GeneratorExp, ast.DictComp)):
for generator in node.generators:
expression(generator.iter, present)
for condition in generator.ifs:
expression(condition, present)
present = present or _condition_requires_procfs(condition, True)
for attr in ("elt", "key", "value"):
expression(getattr(node, attr, None), present)
elif isinstance(node, ast.Lambda):
expression(node.body, False)
else:
for child in ast.iter_child_nodes(node):
expression(child, present)
def block(statements, present):
for statement in statements:
if isinstance(statement, (ast.FunctionDef, ast.AsyncFunctionDef)):
block(statement.body, False)
for value in statement.decorator_list + statement.args.defaults:
expression(value, present)
elif isinstance(statement, ast.ClassDef):
block(statement.body, present)
elif isinstance(statement, ast.If):
expression(statement.test, present)
yes = block(statement.body, present or _condition_requires_procfs(statement.test, True))
no = block(statement.orelse, present or _condition_requires_procfs(statement.test, False))
continuing = [fact for fact in (yes, no) if fact is not None]
if not continuing:
return None
present = all(continuing)
elif isinstance(statement, (ast.Return, ast.Raise, ast.Break, ast.Continue)):
for child in ast.iter_child_nodes(statement):
expression(child, present)
return None
elif isinstance(statement, (ast.For, ast.AsyncFor, ast.While)):
condition = statement.test if isinstance(statement, ast.While) else statement.iter
expression(condition, present)
body_present = present or (isinstance(statement, ast.While) and _condition_requires_procfs(condition, True))
block(statement.body, body_present)
block(statement.orelse, present)
elif isinstance(statement, (ast.Try, getattr(ast, "TryStar", ast.Try))):
block(statement.body, present)
block(statement.orelse, present)
block(statement.finalbody, present)
for handler in statement.handlers:
block(handler.body, present)
elif isinstance(statement, (ast.With, ast.AsyncWith)):
for item in statement.items:
expression(item.context_expr, present)
block(statement.body, present)
else:
expression(statement, present)
return present
block(tree.body, False)
return guarded
def _app_python_files() -> list[Path]: def _app_python_files() -> list[Path]:
@@ -216,10 +139,12 @@ def _collect_sites() -> tuple[list[str], list[str]]:
sites = _scan_sites(tree) sites = _scan_sites(tree)
if not sites: if not sites:
continue continue
guards = _guarded_scan_ids(tree) guards = _guard_lines(tree)
for site in sites: for site in sites:
scope = _enclosing_scope(tree, site)
start = getattr(scope, "lineno", 0)
label = f"{path.relative_to(REPO_ROOT)}:{site.lineno}" label = f"{path.relative_to(REPO_ROOT)}:{site.lineno}"
if id(site) in guards: if any(start <= g < site.lineno for g in guards):
guarded.append(label) guarded.append(label)
else: else:
unguarded.append(label) unguarded.append(label)
@@ -229,7 +154,7 @@ def _collect_sites() -> tuple[list[str], list[str]]:
def test_every_procfs_scan_is_guarded_by_an_existence_check(): def test_every_procfs_scan_is_guarded_by_an_existence_check():
_guarded, unguarded = _collect_sites() _guarded, unguarded = _collect_sites()
assert not unguarded, ( assert not unguarded, (
"procfs pid scans without an effective conditional existence guard — " "procfs pid scans with no existence check in the enclosing function — "
"these raise FileNotFoundError on macOS and Windows: " "these raise FileNotFoundError on macOS and Windows: "
+ ", ".join(unguarded) + ", ".join(unguarded)
) )
@@ -251,49 +176,3 @@ def test_the_guard_detector_still_sees_the_known_scans():
f"root was renamed. Found: {sorted(found)}" f"root was renamed. Found: {sorted(found)}"
) )
assert len(found) >= 3, f"expected at least 3 procfs scans, found {sorted(found)}" assert len(found) >= 3, f"expected at least 3 procfs scans, found {sorted(found)}"
@pytest.mark.parametrize("body", [
'Path("/proc").exists(); os.listdir("/proc")',
'if Path("/proc").exists():\n pass\nos.listdir("/proc")',
'if not Path("/proc").exists():\n os.listdir("/proc")',
'if ready or Path("/proc").exists():\n os.listdir("/proc")',
'if ready:\n Path("/proc").exists()\nos.listdir("/proc")',
'os.listdir("/proc"); Path("/proc").exists()',
'if Path("/tmp").exists():\n os.listdir("/proc")',
'def other():\n Path("/proc").exists()\nos.listdir("/proc")',
'if not has_procfs():\n pass\nos.listdir("/proc")',
'for x in []:\n if not has_procfs():\n return\nos.listdir("/proc")',
'if has_procfs():\n return\nelse:\n os.listdir("/proc")',
'[pid for pid in os.listdir("/proc") if has_procfs()]',
], ids=["ignored", "expired-branch", "inverted", "unsafe-or", "sibling-branch",
"too-late", "wrong-path", "other-function", "no-early-exit", "loop-may-not-run",
"wrong-else", "late-comprehension-filter"])
def test_guard_detector_rejects_ineffective_checks(body):
tree = ast.parse("def scan():\n" + "\n".join(" " + line for line in body.splitlines()))
sites = _scan_sites(tree)
assert sites, "negative control must contain a recognized enumeration"
assert any(id(site) not in _guarded_scan_ids(tree) for site in sites)
@pytest.mark.parametrize("body", [
'if Path("/proc").exists():\n os.listdir("/proc")',
'if not Path("/proc").exists():\n return\nos.listdir("/proc")',
'if not has_procfs():\n raise RuntimeError()\nos.listdir(PROC_ROOT)',
'if has_procfs():\n pass\nelse:\n return\nPROC_ROOT.iterdir()',
'if ready and has_procfs():\n os.scandir("/proc")',
'if not has_procfs() or disabled:\n return\nPROC_ROOT.glob("*")',
'if sys.platform == "linux" and has_procfs():\n PROC_ROOT.iterdir()',
'for item in items:\n if not has_procfs():\n continue\n os.listdir("/proc")',
'if os.path.isdir("/proc"):\n os.listdir("/proc")',
'if has_procfs() and list(PROC_ROOT.iterdir()):\n pass',
'if not has_procfs() or list(PROC_ROOT.iterdir()):\n pass',
'[pid for item in items if has_procfs() for pid in os.listdir("/proc")]',
], ids=["positive-branch", "early-return", "early-raise", "else-exits", "and",
"negative-or", "platform-and-procfs", "loop-continue", "isdir",
"short-circuit-and", "short-circuit-or", "guarded-comprehension"])
def test_guard_detector_preserves_effective_checks(body):
tree = ast.parse("def scan():\n" + "\n".join(" " + line for line in body.splitlines()))
sites = _scan_sites(tree)
assert sites, "positive control must contain a recognized enumeration"
assert all(id(site) in _guarded_scan_ids(tree) for site in sites)
+29
View File
@@ -79,3 +79,32 @@ def test_tmp_path_under_private_runtime_fits_unix_socket(tmp_path):
# witness. A nested pytest-of-<user> basetemp made it 110 bytes under xdist. # witness. A nested pytest-of-<user> basetemp made it 110 bytes under xdist.
with socket.socket(socket.AF_UNIX) as sock: with socket.socket(socket.AF_UNIX) as sock:
sock.bind(str(tmp_path / "tmux.sock")) sock.bind(str(tmp_path / "tmux.sock"))
def test_collection_database_survives_thread_connection_churn():
"""TestClient threads must not lose the collection database or its schema.
SingletonThreadPool's default five-connection limit used to close the main
thread's in-memory database after repeated route clients. Each new thread
also saw an empty database. Exercise more threads than that limit and then
verify both compatibility factories still see all registered tables.
"""
from concurrent.futures import ThreadPoolExecutor
from sqlalchemy import inspect
import core.database as database
import src.database as compatibility
expected = set(database.Base.metadata.tables)
def table_names(factory):
with factory() as session:
return set(inspect(session.connection()).get_table_names())
assert {"documents", "model_endpoints", "mcp_servers", "calendars"} <= expected
assert expected <= table_names(database.SessionLocal)
for _ in range(8):
# A fresh executor creates a fresh thread rather than reusing one.
with ThreadPoolExecutor(max_workers=1) as executor:
assert expected <= executor.submit(table_names, database.SessionLocal).result()
assert expected <= table_names(database.SessionLocal)
assert expected <= table_names(compatibility.SessionLocal)