From 237791eb2f22907b4fa88d1833be972c901a1d1e Mon Sep 17 00:00:00 2001 From: MilagrosMarin Date: Mon, 20 Jul 2026 17:02:53 +0200 Subject: [PATCH 1/2] tests: add coverage for deferred audit findings + isolate test_settings from ambient env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two test-suite hardening changes, all in tests/, no source-code changes: ## 1. Coverage additions for 3 audit-deferred findings (6 test functions) - tests/integration/test_dependencies.py TestLoadAllShortCircuit (2 tests) Guards the ~53ms/populate-key perf fix from PR #1499 by spying on Dependencies.load during a repeat load_all_upstream/load_all_downstream call. Verified: gating the early-return at dependencies.py:266-267, :300-301 with 'if False and ...' makes both tests fail; restored. - tests/integration/test_gc.py TestDeleteSchemaPathPruning (2 tests) Covers the parent-directory pruning loop in gc.py delete_schema_path. Inserts two rows in separate pk-dirs, deletes one, runs collect(), asserts the orphan's pk-dir is pruned, the sibling's is preserved, and the store root is untouched. Verified: removing the pruning loop at gc.py:293-302 makes test_pruning_removes_empty_pk_directory fail; restored. - tests/integration/test_object.py TestStoragePathGeneration (2 tests) Covers build_object_path with schema_prefix='' (the documented pre-2.3.1 legacy layout — not previously tested). Asserts the path is {schema}/{table}/pk=v/field_token.dat (no leading empty segment). Verified: removing the 'if prefix:' guard at storage.py:267-269 makes both tests fail; restored. ## 2. Env-isolation fixture for tests/unit/test_settings.py Four tests in TestStoreEnv and TestBackendConfiguration read DJ_HOST/ DJ_USER/DJ_PASS/DJ_PORT/DJ_BACKEND via pydantic-settings and fail if any of those vars is exported in the developer's shell — even when the test's own contract is 'ignore config file' or 'auto-detect port'. CI runs in a clean env so this only bites devs who source a local .env, but the tests' contract shouldn't depend on ambient env cleanliness. Adds a class-level @pytest.fixture(autouse=True) that monkeypatch.delenv's the 5 DJ_* vars at the top of each test in both classes. Individual tests that need to set DJ_* explicitly still work — the fixture runs first, so their setenv wins. Verified: pytest tests/unit/test_settings.py::TestStoreEnv tests/unit/test_settings.py::TestBackendConfiguration → 19/19 pass locally with the fixture. --- tests/integration/test_dependencies.py | 61 +++++++++++++ tests/integration/test_gc.py | 113 +++++++++++++++++++++++++ tests/integration/test_object.py | 50 +++++++++++ tests/unit/test_settings.py | 26 ++++++ 4 files changed, 250 insertions(+) diff --git a/tests/integration/test_dependencies.py b/tests/integration/test_dependencies.py index 7d9c5dd6e..27c25afab 100644 --- a/tests/integration/test_dependencies.py +++ b/tests/integration/test_dependencies.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + from pytest import raises from datajoint import errors @@ -50,3 +52,62 @@ def test_unique_dependency(thing_tables): # duplicate foreign key attributes = not ok with raises(errors.DuplicateError): c.insert1(dict(a=1, b1=1, b2=1)) + + +class TestLoadAllShortCircuit: + """Finding #7 (audit deferred): the ``load_all_upstream`` / + ``load_all_downstream`` helpers must short-circuit when the graph already + covers every schema they would discover. The guard at + ``dependencies.py:266-267`` and ``:300-301`` was introduced by PR #1499 as + the ~53 ms/key populate speedup; a regression would silently pass CI + because the correctness of the graph is unaffected — only its cost. + + These tests count calls to ``Dependencies.load`` on the second invocation. + The first call must load once (to populate ``_loaded_schemas``); the + second must NOT call ``load`` at all — the ``if self._loaded and + known_schemas <= self._loaded_schemas: return`` early-return owns the + entire path. + """ + + def test_load_all_downstream_short_circuits_on_repeat(self, thing_tables): + # thing_tables ensures at least one schema is activated on the + # connection, so the ``if not known_schemas: self.load(); return`` + # bail-out branch does not fire. + a, _, _, _, _ = thing_tables + conn = a.connection + + # Warm up the graph once — this call is allowed to invoke load(). + conn.dependencies.load_all_downstream() + assert conn.dependencies._loaded is True + assert conn.dependencies._loaded_schemas, "warm-up must populate _loaded_schemas" + + # Second identical call must hit the early-return: no load(), no rebuild. + with patch.object( + type(conn.dependencies), "load", wraps=conn.dependencies.load + ) as spy_load: + conn.dependencies.load_all_downstream() + assert spy_load.call_count == 0, ( + "load_all_downstream must short-circuit when _loaded_schemas already " + "covers the discovered set (#1493, PR #1499). Deleting the " + "`if self._loaded and known_schemas <= self._loaded_schemas: return` " + "at dependencies.py:266-267 would make this test fail." + ) + + def test_load_all_upstream_short_circuits_on_repeat(self, thing_tables): + a, _, _, _, _ = thing_tables + conn = a.connection + + conn.dependencies.load_all_upstream() + assert conn.dependencies._loaded is True + assert conn.dependencies._loaded_schemas + + with patch.object( + type(conn.dependencies), "load", wraps=conn.dependencies.load + ) as spy_load: + conn.dependencies.load_all_upstream() + assert spy_load.call_count == 0, ( + "load_all_upstream must short-circuit when _loaded_schemas already " + "covers the discovered set (#1493, PR #1499). Deleting the " + "`if self._loaded and known_schemas <= self._loaded_schemas: return` " + "at dependencies.py:300-301 would make this test fail." + ) diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 95e427055..211377fa9 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -843,3 +843,116 @@ def test_scan_errors_in_stats_dict(self): assert len(stats["scan_errors"]) == 1 assert stats["scan_errors"][0].startswith("list_hash_paths(s):") assert "s3 access denied" in stats["scan_errors"][0] + + +class TestDeleteSchemaPathPruning: + """Finding #8 (audit deferred): ``GarbageCollector.delete_schema_path`` + prunes now-empty parent directories after removing the orphaned file + (``gc.py:277-306``). The pruning walk was previously uncovered — no test + exercised the ``while parent and parent != root and parent.startswith(root)`` + loop, its ``fs.ls`` "not empty" break, its exception guards, or the + stop-at-store-root boundary. + + A regression to "delete the file, leave every parent dir behind" would + silently pass CI and gradually accumulate empty directories on every + store, so a live-data test that inspects the on-disk tree after + ``collect(dry_run=False)`` is the right lock. + """ + + @pytest.fixture + def schema_prune(self, connection_test, prefix, mock_stores): + schema = dj.Schema( + f"{prefix}_test_gc_prune", + context={"GcObjectTest": GcObjectTest}, + connection=connection_test, + ) + schema(GcObjectTest) + yield schema + schema.drop() + + def test_pruning_removes_empty_pk_directory(self, schema_prune): + """Deleting the only file under a pk-dir must remove the file AND + prune the now-empty pk-dir; a sibling pk-dir with a live file survives. + + Regressing ``delete_schema_path`` to skip the ``while ... rmdir`` + pruning loop would leave the empty ``rid=1/`` directory behind and + make this test fail. + """ + from pathlib import Path + + # Two rows in separate pk-dirs: rid=1 will be orphaned, rid=2 stays live. + GcObjectTest.insert1({"rid": 1, "results": b"orphan-me"}) + GcObjectTest.insert1({"rid": 2, "results": b"live"}) + + collector = _gc(schema_prune) + cfg = schema_prune.connection._config + store_root = Path(cfg.get_store_spec("local")["location"]) + + # Sanity: both pk-dirs exist on disk pre-delete. + refs_all = collector.schema_references() + assert len(refs_all) == 2 + for ref in refs_all: + assert (store_root / ref).exists(), f"expected {ref} to exist pre-delete" + + # Orphan rid=1 and reclaim. + (GcObjectTest & {"rid": 1}).delete(prompt=False) + refs_live = collector.schema_references() + live_ref = next(iter(refs_live)) + dead_ref = next(iter(refs_all - refs_live)) + + stats = collector.collect(dry_run=False) + assert stats["schema_paths_deleted"] >= 1 + + # The orphaned file is gone. + assert not (store_root / dead_ref).exists(), ( + f"orphaned schema file must be deleted; still present at {dead_ref}" + ) + # The pruning loop must have removed the now-empty pk-dir. + dead_pk_dir = (store_root / dead_ref).parent + assert not dead_pk_dir.exists(), ( + f"parent pk-directory must be pruned when its last file is removed; " + f"still present at {dead_pk_dir} — deleting the pruning loop at " + f"gc.py:293-302 would make this test fail" + ) + + # The sibling pk-dir with a live file must be preserved. + live_pk_dir = (store_root / live_ref).parent + assert live_pk_dir.exists(), ( + f"pk-directory holding a live file must not be pruned; missing {live_pk_dir}" + ) + assert (store_root / live_ref).exists(), ( + f"live schema file must survive collect(); missing {live_ref}" + ) + + # The table directory (grandparent) still holds live content, so it + # must be preserved — the pruning walk must stop at "not empty". + assert live_pk_dir.parent.exists(), ( + "table directory must survive when it still holds a live pk-dir" + ) + + def test_pruning_stops_at_store_root(self, schema_prune): + """After removing the last orphan under a schema, the pruning walk may + remove the pk-dir, table-dir, schema-dir, and section-dir but MUST + stop at the store root — a regression to "keep walking past the root" + would be catastrophic (removes user's storage directory itself). + + The boundary ``while parent and parent != root and parent.startswith(root)`` + owns this invariant. + """ + from pathlib import Path + + GcObjectTest.insert1({"rid": 1, "results": b"only-row"}) + + collector = _gc(schema_prune) + cfg = schema_prune.connection._config + store_root = Path(cfg.get_store_spec("local")["location"]) + assert store_root.exists() + + (GcObjectTest & {"rid": 1}).delete(prompt=False) + collector.collect(dry_run=False) + + # Store root must survive even when its entire contents were pruned. + assert store_root.exists(), ( + f"pruning walk must stop at store root; root itself was removed at {store_root} — " + f"the `parent != root and parent.startswith(root)` boundary in gc.py:295 owns this" + ) diff --git a/tests/integration/test_object.py b/tests/integration/test_object.py index ec40f40b4..eeae0a9f3 100644 --- a/tests/integration/test_object.py +++ b/tests/integration/test_object.py @@ -130,6 +130,56 @@ def test_build_object_path_with_partition(self): # section prefix first, then partition attrs (schema_prefix default "_schema") assert path.startswith("_schema/subject_id=1") + def test_build_object_path_empty_schema_prefix(self): + """Finding #19 (audit deferred): ``schema_prefix=""`` reproduces the + documented legacy pre-2.3.1 storage layout (no section prefix). + + The guard at ``storage.py:267-269`` (``prefix = schema_prefix.strip("/"); + if prefix: parts.append(prefix)``) exists specifically so an empty + prefix does NOT produce a path with an empty leading segment (e.g. + ``"/myschema/MyTable/..."``). Every other test in this class passes + ``schema_prefix="_schema"`` and would still pass if the ``if prefix:`` + guard were deleted, so a regression to always-prepend would go + unnoticed. This test locks in the pre-2.3.1 layout. + """ + path, token = build_object_path( + schema="myschema", + table="MyTable", + field="data_file", + primary_key={"id": 42}, + ext=".dat", + schema_prefix="", + ) + assert path == f"myschema/MyTable/id=42/data_file_{token}.dat", ( + f"empty schema_prefix must produce a path with NO leading section " + f"segment (documented legacy layout); got {path!r}" + ) + # Regression sentinel: deleting the ``if prefix:`` guard would produce + # a leading empty segment, i.e. ``"/myschema/..."``. + assert not path.startswith("/"), ( + f"path must not have a leading empty segment when schema_prefix is empty; " + f"got {path!r}" + ) + + def test_build_object_path_empty_schema_prefix_with_partition(self): + """Finding #19 companion: legacy layout must also compose correctly + with a partition pattern — partition attrs lead, no empty segment + before them.""" + path, token = build_object_path( + schema="myschema", + table="MyTable", + field="data", + primary_key={"subject_id": 1, "session_id": 2}, + ext=".dat", + partition_pattern="{subject_id}", + schema_prefix="", + ) + assert path.startswith("subject_id=1/myschema/"), ( + f"empty schema_prefix + partition must start with the partition " + f"segment (no empty leading segment); got {path!r}" + ) + assert not path.startswith("/") + class TestObjectRef: """Tests for ObjectRef class.""" diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index 0aeed2c67..3606c83c2 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -509,6 +509,19 @@ def test_load_store_arbitrary_attr(self, tmp_path): class TestStoreEnv: """Test DJ_STORES env var and DJ_IGNORE_CONFIG_FILE flag.""" + @pytest.fixture(autouse=True) + def _isolate_dj_env(self, monkeypatch): + """Clear ambient DJ_* env vars so tests are isolated from the developer's shell. + + pydantic-settings reads any DJ_* prefixed variable and merges it into the + settings object; a developer with ``DJ_HOST``/``DJ_USER``/``DJ_PASS``/ + ``DJ_PORT``/``DJ_BACKEND`` exported (e.g. from a local ``.env``) would + otherwise see assertions on the config's defaults or file-loaded values + fail here even though the test's own contract is unrelated. + """ + for var in ("DJ_HOST", "DJ_USER", "DJ_PASS", "DJ_PORT", "DJ_BACKEND"): + monkeypatch.delenv(var, raising=False) + def _isolate_filesystem(self, monkeypatch, tmp_path): """chdir into a tmp_path with a .git sentinel so find_config_file stops there.""" (tmp_path / ".git").mkdir() @@ -937,6 +950,19 @@ def test_database_prefix_empty_no_warning(self): class TestBackendConfiguration: """Test database backend configuration and port auto-detection.""" + @pytest.fixture(autouse=True) + def _isolate_dj_env(self, monkeypatch): + """Clear ambient DJ_* env vars so tests are isolated from the developer's shell. + + ``DatabaseSettings()`` reads ``DJ_HOST``/``DJ_USER``/``DJ_PASS``/ + ``DJ_PORT``/``DJ_BACKEND`` from the process env; leaked values would + otherwise fail assertions on backend defaults and auto-detected ports. + Individual tests may still ``monkeypatch.setenv(...)`` to test explicit + settings — the fixture runs first, so their setenv wins. + """ + for var in ("DJ_HOST", "DJ_USER", "DJ_PASS", "DJ_PORT", "DJ_BACKEND"): + monkeypatch.delenv(var, raising=False) + def test_backend_default(self): """Test default backend is mysql.""" from datajoint.settings import DatabaseSettings From 7a2c158887215ed6df0408b5b8ee824240c74bdc Mon Sep 17 00:00:00 2001 From: MilagrosMarin Date: Mon, 20 Jul 2026 17:12:49 +0200 Subject: [PATCH 2/2] style: apply ruff-format (collapse multi-line asserts where they fit) --- tests/integration/test_dependencies.py | 8 ++------ tests/integration/test_gc.py | 16 ++++------------ tests/integration/test_object.py | 3 +-- 3 files changed, 7 insertions(+), 20 deletions(-) diff --git a/tests/integration/test_dependencies.py b/tests/integration/test_dependencies.py index 27c25afab..fb6217f33 100644 --- a/tests/integration/test_dependencies.py +++ b/tests/integration/test_dependencies.py @@ -82,9 +82,7 @@ def test_load_all_downstream_short_circuits_on_repeat(self, thing_tables): assert conn.dependencies._loaded_schemas, "warm-up must populate _loaded_schemas" # Second identical call must hit the early-return: no load(), no rebuild. - with patch.object( - type(conn.dependencies), "load", wraps=conn.dependencies.load - ) as spy_load: + with patch.object(type(conn.dependencies), "load", wraps=conn.dependencies.load) as spy_load: conn.dependencies.load_all_downstream() assert spy_load.call_count == 0, ( "load_all_downstream must short-circuit when _loaded_schemas already " @@ -101,9 +99,7 @@ def test_load_all_upstream_short_circuits_on_repeat(self, thing_tables): assert conn.dependencies._loaded is True assert conn.dependencies._loaded_schemas - with patch.object( - type(conn.dependencies), "load", wraps=conn.dependencies.load - ) as spy_load: + with patch.object(type(conn.dependencies), "load", wraps=conn.dependencies.load) as spy_load: conn.dependencies.load_all_upstream() assert spy_load.call_count == 0, ( "load_all_upstream must short-circuit when _loaded_schemas already " diff --git a/tests/integration/test_gc.py b/tests/integration/test_gc.py index 211377fa9..a9bd5a4a5 100644 --- a/tests/integration/test_gc.py +++ b/tests/integration/test_gc.py @@ -904,9 +904,7 @@ def test_pruning_removes_empty_pk_directory(self, schema_prune): assert stats["schema_paths_deleted"] >= 1 # The orphaned file is gone. - assert not (store_root / dead_ref).exists(), ( - f"orphaned schema file must be deleted; still present at {dead_ref}" - ) + assert not (store_root / dead_ref).exists(), f"orphaned schema file must be deleted; still present at {dead_ref}" # The pruning loop must have removed the now-empty pk-dir. dead_pk_dir = (store_root / dead_ref).parent assert not dead_pk_dir.exists(), ( @@ -917,18 +915,12 @@ def test_pruning_removes_empty_pk_directory(self, schema_prune): # The sibling pk-dir with a live file must be preserved. live_pk_dir = (store_root / live_ref).parent - assert live_pk_dir.exists(), ( - f"pk-directory holding a live file must not be pruned; missing {live_pk_dir}" - ) - assert (store_root / live_ref).exists(), ( - f"live schema file must survive collect(); missing {live_ref}" - ) + assert live_pk_dir.exists(), f"pk-directory holding a live file must not be pruned; missing {live_pk_dir}" + assert (store_root / live_ref).exists(), f"live schema file must survive collect(); missing {live_ref}" # The table directory (grandparent) still holds live content, so it # must be preserved — the pruning walk must stop at "not empty". - assert live_pk_dir.parent.exists(), ( - "table directory must survive when it still holds a live pk-dir" - ) + assert live_pk_dir.parent.exists(), "table directory must survive when it still holds a live pk-dir" def test_pruning_stops_at_store_root(self, schema_prune): """After removing the last orphan under a schema, the pruning walk may diff --git a/tests/integration/test_object.py b/tests/integration/test_object.py index eeae0a9f3..ba5cb47ee 100644 --- a/tests/integration/test_object.py +++ b/tests/integration/test_object.py @@ -157,8 +157,7 @@ def test_build_object_path_empty_schema_prefix(self): # Regression sentinel: deleting the ``if prefix:`` guard would produce # a leading empty segment, i.e. ``"/myschema/..."``. assert not path.startswith("/"), ( - f"path must not have a leading empty segment when schema_prefix is empty; " - f"got {path!r}" + f"path must not have a leading empty segment when schema_prefix is empty; " f"got {path!r}" ) def test_build_object_path_empty_schema_prefix_with_partition(self):