Skip to content

tests: add coverage for deferred audit findings + isolate test_settings from ambient env#1513

Merged
dimitri-yatsenko merged 2 commits into
datajoint:masterfrom
MilagrosMarin:tests/2.3.1-coverage-and-env-isolation
Jul 20, 2026
Merged

tests: add coverage for deferred audit findings + isolate test_settings from ambient env#1513
dimitri-yatsenko merged 2 commits into
datajoint:masterfrom
MilagrosMarin:tests/2.3.1-coverage-and-env-isolation

Conversation

@MilagrosMarin

Copy link
Copy Markdown
Contributor

Two test-suite hardening changes, all in tests/, no source-code changes.

1. Coverage for 3 audit-deferred findings (6 test functions)

Adds targeted regression guards for behaviors that had no direct coverage. Each test's docstring cites the underlying code and describes what regression it catches.

  • tests/integration/test_dependencies.py::TestLoadAllShortCircuit (2 tests) — guards the ~53 ms/populate-key perf fix from #1499. Warm up the graph, then spy on Dependencies.load during a second identical load_all_upstream/load_all_downstream call; assert call_count == 0. Regressing the early-return at dependencies.py:266-267 and :300-301 (e.g. if False and ...) makes both tests fail.

  • tests/integration/test_gc.py::TestDeleteSchemaPathPruning (2 tests) — covers the parent-directory pruning loop in delete_schema_path. Inserts two rows into separate PK directories, deletes one, runs collect(dry_run=False), then asserts: the orphan's file is gone, its now-empty PK directory is pruned, the sibling PK directory is preserved, and the store root itself survives. Regressing the while ... rmdir loop at gc.py:293-302 makes test_pruning_removes_empty_pk_directory fail.

  • tests/integration/test_object.py::TestStoragePathGeneration (2 tests) — covers build_object_path with schema_prefix="" (the documented pre-2.3.1 legacy layout — no test previously exercised it). Asserts the path is {schema}/{table}/pk=v/field_token.dat with no leading empty segment. Regressing the if prefix: guard at storage.py:267-269 (always-prepend) makes both tests fail.

All 6 tests confirm existing behavior; no source-code changes needed. Each was verified in place: gate/remove the source condition → tests fail; restore → tests pass.

2. Env-isolation fixture for tests/unit/test_settings.py

Four tests in TestStoreEnv (test_ignore_config_file_skips_secrets, test_ignore_config_file_default_loads_both) and TestBackendConfiguration (test_backend_postgresql, test_backend_env_var) construct pydantic-settings objects that read DJ_HOST/DJ_USER/DJ_PASS/DJ_PORT/DJ_BACKEND from the process env. A developer with any of those vars exported (e.g. from a local .env) sees these tests fail even though their own contract is "ignore config file" or "auto-detect port" — because a leaked ambient value overrides the default under test.

Reproduces cleanly:

  • DJ_HOST=localhost DJ_USER=root DJ_PASS=x pytest tests/unit/test_settings.py::TestStoreEnv::test_ignore_config_file_skips_secretsFAIL
  • env -i pytest tests/unit/test_settings.py::TestStoreEnv::test_ignore_config_file_skips_secretsPASS

CI runs in a clean env, so this isn't a CI regression — but it bites devs who source project .env files.

Adds a class-level @pytest.fixture(autouse=True) to both TestStoreEnv and TestBackendConfiguration that monkeypatch.delenv's the five DJ_* vars at the top of each test. Tests that explicitly monkeypatch.setenv(...) still work — the fixture runs first, so their setenv wins. Also protects other tests in these classes from the same latent issue (there are more than just the 4 flagged).

Verified locally: pytest tests/unit/test_settings.py::TestStoreEnv tests/unit/test_settings.py::TestBackendConfiguration → 19/19 pass with the fixture.


Follow-up to the v2.3.1 audit (post-#1503, #1504, #1505, #1506, #1510, #1511). Docs-issue traceable: the 3 coverage additions close deferred audit findings #7, #8, #19; the env-isolation fix surfaced during a follow-up local test run.

…gs from ambient env

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 datajoint#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.
@dimitri-yatsenko

Copy link
Copy Markdown
Member

Reviewed — verified all four files against the source; the tests are correct and well-targeted (test-only, no source changes). Each new test genuinely fails if its guarded behavior is regressed:

  • TestLoadAllShortCircuit — the early-return if self._loaded and known_schemas <= self._loaded_schemas: return exists in both load_all_downstream and load_all_upstream. Spying on load is the right probe: discovery queries still run; only the expensive fix(#1493): build self.upstream lazily; stop reloading deps per populate key #1499 rebuild (load()) is skipped, so call_count == 0 is the correct assertion.
  • TestDeleteSchemaPathPruning — the prune-empty-parents while loop, the fs.ls not-empty break, the exception guard, and the parent != root and parent.startswith(root) store-root boundary all exist in GarbageCollector.delete_schema_path. Helpers (schema_references, _gc, GcObjectTest, and the prefix/connection_test/mock_stores fixtures) all resolve.
  • TestStoragePathGeneration — matches build_object_path: empty schema_prefix skips the section segment via the if prefix: guard → myschema/MyTable/id=42/…; with a partition → subject_id=1/myschema/…. The no-leading-/ sentinel is a nice touch.
  • env-isolation fixture — autouse delenv of the five DJ_* vars, running before test bodies so explicit setenv still wins. Correctly fixes the ambient-.env flakiness.

One suggestion — cite method names, not line numbers. The TestDeleteSchemaPathPruning docstrings point at gc.py:293-302 and gc.py:295, but the boundary while parent and parent != root and parent.startswith(root) is actually at gc.py:287 on current master — the refs have already drifted ~8 lines (the dependencies.py and storage.py refs happen to still be exact). Since these numbers rot on every edit above them, I'd suggest citing the method (e.g. "the pruning loop in delete_schema_path") rather than file:line throughout the new docstrings. Keeps the regression-sentinel intent without the maintenance burden.

Minor: _isolate_dj_env is duplicated verbatim in TestStoreEnv and TestBackendConfiguration — could be one shared fixture (module-level + usefixtures), purely DRY. And if other classes in test_settings.py build settings that read DJ_*, they'd have the same latent ambient-env issue — out of scope here, just worth noting.

Substance looks good.

@dimitri-yatsenko dimitri-yatsenko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the tests are correct and well-targeted (re-verified against current origin/master @55f3beef).

Correction to my earlier comment: I'd checked the line refs against a stale local tree and got the drift backwards. Against current master:

  • gc.py:295 (the while parent and parent != root … boundary) and the 293-302 range — exact.
  • storage.py:267-269 (if prefix: guard) — exact.
  • dependencies.pythis is the drifted one: the if self._loaded and known_schemas <= self._loaded_schemas: return short-circuits are at 258 (downstream) / 292 (upstream) now, not the cited 266-267/300-301 (~8 lines off, shifted by the #1508 MultiDiGraph merge).

Which only reinforces the earlier suggestion: cite the method (the pruning loop in delete_schema_path, the short-circuit in load_all_downstream/load_all_upstream) rather than file:line — the numbers demonstrably rot; they even fooled me via a stale checkout. Non-blocking — fine as a quick follow-up or to leave.

Substance is solid. ✅

@dimitri-yatsenko
dimitri-yatsenko merged commit 3b43a2a into datajoint:master Jul 20, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants