umbragit/UmbraTrans
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

27 of 63 database tests were skipping, and the summary said nothing

Browse Source 
The PostgreSQL adapter suite refuses to run against a database that already holds rows,
because it truncates indiscriminately and cannot do so selectively - audit_events is
append-only, with a trigger that raises on delete. The refusal is correct. Two separate
faults meant it was firing when it should not have.

The guard sampled the live tables at the moment it was first requested. pytest runs files
alphabetically and several files before it create organizations and tenant schemas, so by
the time it looked, this session's own rows read as pre-existing data. It skipped 27 of
63 tests on a scratch database that had been empty moments earlier, and '567 passed'
carried no hint of it. The sample is now taken in pytest_sessionstart, before collection.
A session-scoped autouse fixture was tried first and is not early enough - it is
instantiated lazily and does not precede the other session fixtures that write.

Nothing cleaned up after the session. Ten files open their own connection to the DSN
outside the postgres fixture, each inserted organization provisions a tenant schema, and
no test dropped it: 78 schemas per run, which after about fourteen runs exhausted
max_locks_per_transaction and errored the integration tests with a message reading like
a PostgreSQL defect.

And the fix for that nearly caused the disaster the guard exists to prevent. The first
version of the cleanup truncated unconditionally on teardown, so pointing the suite at a
populated database destroyed it while every test that could have touched it skipped
itself. Found by planting an organization, watching the suite refuse, and finding the
organization gone afterwards. It now leaves an occupied database exactly as found, and
says so - through pytest's terminal reporter, because a plain write to stderr is
swallowed by file-descriptor capture and was seen by nobody.

test_scratch_database_safety.py covers all three, and each was confirmed to fail when the
protection is removed. Three mistakes in writing it are recorded in the file: a leading
test file that cleaned up after itself and so reproduced nothing; assertions that passed
because '-rs' was missing and pytest had printed no skip reasons; and a check that
stopped at the warnings header, which pytest prints its summary line after.

598 passed, 4 skipped. The 4 are environmental: two pages with no password field, two
needing passwordless sudo for device permissions.
This commit is contained in:
umbra audio
2026-10-03 21:56:18 -07:00
parent b0cdec63a1
commit 8d1c6a6406
3 changed files with 531 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

224
umbratrans_web/tests/conftest.py
View File

@@ -9,9 +9,11 @@ or otherwise.
from __future__ import annotations
import os
import re
import secrets
import string
import sys
from collections.abc import Iterator
from dataclasses import dataclass, replace
from typing import Any
@@ -168,6 +170,228 @@ class ApiHarness:
self._client.cookies.clear()
#: Tables whose rows are created per test and must not survive the session. Every one
#: of the first six is referenced by ``organizations``, so it has to be emptied before
#: the organizations themselves can go. ``users`` is here for the same reason from the
#: other direction: the tenant tables hold foreign keys into it.
_SESSION_TABLES = (
"public.artifacts",
"public.audit_events",
"public.authors",
"public.compute_nodes",
"public.jobs",
"public.node_pairing_codes",
"public.publishers",
"public.user_roles",
"public.users",
"public.organizations",
)
#: What the scratch database held when the session began, sampled in
#: ``pytest_sessionstart``. ``None`` means "not sampled", which is the case whenever
#: the disposal is not acknowledged - see :func:`scratch_database_session`.
_SESSION_START: dict[str, Any] | None = None
def pytest_sessionstart(session: pytest.Session) -> None:
"""Sample the scratch database before collection, when nothing has touched it.
This is a hook rather than a fixture for a reason that cost two wrong runs to
find. A session-scoped autouse fixture is instantiated lazily, on first request,
and pytest does not guarantee it precedes the other session-scoped fixtures that
create organizations - so the sample taken there saw six schemas that this same
session had already created and reported the database as occupied. The guard that
reads this value then skipped 27 of the 63 adapter tests, exactly as it had been
skipping them before. ``pytest_sessionstart`` is the only moment that is
unambiguously before everything, including collection.
"""
global _SESSION_START # noqa: PLW0603 - one value for the whole session, by design
dsn = os.environ.get("UMBRATRANS_TEST_DSN", "").strip()
acknowledged = os.environ.get("UMBRATRANS_TEST_DATABASE_IS_DISPOSABLE", "").strip() in {
"1",
"true",
"yes",
"on",
}
if not dsn or not acknowledged or "umbracontrol" in dsn:
return
try:
import psycopg
from psycopg.rows import dict_row
except ImportError: # pragma: no cover - keeps a non-database run collectable
return
connection = psycopg.connect(dsn, row_factory=dict_row)
try:
counts = _occupancy(connection)
connection.rollback()
finally:
# Closed immediately. A connection held open for the session would keep its
# transaction's locks on every table it read, and the first TRUNCATE in the
# suite would then block forever waiting for ACCESS EXCLUSIVE. That is a hang
# rather than a failure, which is why this is a short-lived connection and not
# one kept for the session.
connection.close()
_SESSION_START = {"enabled": True, "occupied": {n: c for n, c in counts.items() if c}}
@pytest.fixture(scope="session", autouse=True)
def scratch_database_session(request: pytest.FixtureRequest):
"""Empty the scratch database when the session ends, whoever created what.
The ``postgres`` fixture in ``test_postgres_repository.py`` already truncates three
tables when it finishes. That covers the tests which request it and nothing else,
and ten files under this directory open their own connection to
``UMBRATRANS_TEST_DSN`` and insert organizations without going through that
fixture. Each inserted organization provisions a tenant schema from a trigger, and
no test drops it.
Two consequences, both observed rather than predicted:
* A run left 78 organizations and 78 tenant schemas behind. After about fourteen
runs that is enough schemas that dropping them in one transaction fails with
"out of shared memory ... max_locks_per_transaction", and the PostgreSQL
integration tests error out with a message that reads like a database defect.
* Worse, and silent: ``require_empty_database`` refuses to run the adapter suite
against a database that already holds rows. So the debris from one file made 27
of the 63 adapter tests *skip* on the next run, and the summary still read
"567 passed". A green suite that never opened a database is worse than a red
one, because it looks like evidence.
TRUNCATE rather than DELETE throughout: ``audit_events`` and ``job_events`` carry
triggers that raise on mutation, so DELETE is refused and truncation is the only
way to remove them.
Gated exactly as the adapter suite gates itself - the DSN must be set, the
disposal must be acknowledged explicitly, and ``umbracontrol`` is refused outright.
Without both environment variables this fixture does nothing at all, so a run
without them cannot touch a database.
It also *reports* what the database held when the session began, which the adapter
suite's guard reads instead of re-querying mid-session. That guard exists to refuse
to run against a database holding somebody's data, and reading the live tables
made it read this session's own rows as somebody else's.
"""
import os
dsn = os.environ.get("UMBRATRANS_TEST_DSN", "").strip()
acknowledged = os.environ.get("UMBRATRANS_TEST_DATABASE_IS_DISPOSABLE", "").strip() in {
"1",
"true",
"yes",
"on",
}
if not dsn or not acknowledged or "umbracontrol" in dsn:
yield {"enabled": False, "occupied": {}}
return
# Imported here so that a run without these installed still collects the tests
# that never touch PostgreSQL, rather than erroring during fixture setup.
import psycopg
from psycopg.rows import dict_row
# The sample was taken in pytest_sessionstart, before collection. Reading it from
# there rather than re-sampling is the whole point: see that hook's docstring.
at_start = _SESSION_START if _SESSION_START is not None else {"enabled": True, "occupied": {}}
if at_start.get("occupied"):
# The database held data before this session began, so this run is not entitled
# to change a single row of it - and the adapter suite's guard has already
# skipped for exactly that reason.
#
# This branch is load-bearing in the strongest sense available, and it was
# absent at first. An earlier version truncated unconditionally on teardown,
# which meant that pointing the suite at a populated database destroyed that
# data *while every test that could have touched it skipped itself*. The guard
# was doing its job - 27 skips, refusal, nothing destroyed by any test - and
# the autouse teardown was undoing it one fixture later. Verified by planting
# an organization, watching the suite refuse, and finding the organization
# gone afterwards.
#
# So: leave it exactly as found, and say so, because a run that did nothing
# should not look like a run that succeeded.
#
# Through the terminal reporter rather than sys.stderr: pytest captures at file
# descriptor level for the whole run and discards the buffer unless a test
# fails, so a plain write is swallowed and the message is seen by nobody. It was
# confirmed absent from stderr under default settings and present under
# --capture=no, which is the worst possible combination - the code ran and
# stayed silent.
reporter = request.config.pluginmanager.get_plugin("terminalreporter")
line = (
"umbratrans: left the database untouched - it already held data "
f"({', '.join(sorted(at_start['occupied']))}). Nothing was cleaned up."
)
if reporter is not None:
reporter.write_line("")
reporter.write_line(line, red=True, bold=True)
else: # pragma: no cover - only when the terminal plugin is disabled
sys.stderr.write(f"\n {line}\n")
yield at_start
return
connection = psycopg.connect(dsn, row_factory=dict_row)
try:
yield at_start
schemas = connection.execute(
"SELECT nspname FROM pg_namespace WHERE nspname LIKE 'tenant\\_%' ORDER BY nspname"
).fetchall()
# One schema per transaction, and load-bearing. Dropping several hundred in a
# single transaction exhausts the lock table and fails with "out of shared
# memory"; the message names max_locks_per_transaction but the fix is the
# batching, not the setting.
for schema in schemas:
connection.execute(f'DROP SCHEMA IF EXISTS "{schema["nspname"]}" CASCADE')
connection.commit()
connection.execute(f"TRUNCATE {', '.join(_SESSION_TABLES)} CASCADE")
connection.commit()
finally:
connection.close()
def _occupancy(connection: Any) -> dict[str, int]:
"""Row counts for every table that decides whether a database is a test database.
Covers the platform tables *and* the four tenant tables, which since migration
0005 live one schema per organization. A database can hold nothing in ``public``
and still hold somebody's books, so a check that looked only at ``public`` would
pass on a database about to be emptied.
The tenant tables are found by listing the schemas rather than by asking for a
fixed set, so a schema left over from an older migration, or one whose name does
not match the current convention, is still seen.
"""
counts: dict[str, int] = {}
for name in ("users", "organizations", "audit_events", "user_roles", "sessions", "jobs", "artifacts"):
row = connection.execute(f"SELECT count(*) AS total FROM public.{name}").fetchone()
counts[name] = int(row["total"])
schemas = connection.execute(
"SELECT nspname FROM pg_namespace WHERE nspname LIKE 'tenant\\_%' ORDER BY nspname"
).fetchall()
counts["tenant_schemas"] = len(schemas)
for schema in schemas:
name = schema["nspname"]
tables = connection.execute(
"SELECT tablename FROM pg_tables WHERE schemaname = %s ORDER BY tablename", (name,)
).fetchall()
for table in tables:
row = connection.execute(
f'SELECT count(*) AS total FROM "{name}"."{table["tablename"]}"'
).fetchone()
counts[f"{name}.{table['tablename']}"] = int(row["total"])
return counts
@pytest.fixture
def settings() -> Settings:
return suite_settings()

25
umbratrans_web/tests/test_postgres_repository.py
View File

@@ -739,7 +739,7 @@ integration = pytest.mark.skipif(
@pytest.fixture(scope="session")
def require_empty_database():
def require_empty_database(scratch_database_session):
"""Refuse to run against a database that holds anything.
This suite empties ``users``, ``organizations`` and ``audit_events`` when it
@@ -765,19 +765,34 @@ def require_empty_database():
schema per organization, so a database that holds nothing in ``public`` can
still hold somebody's books. Checking only the platform tables would have made
this guard pass on a database it would then have emptied.
The occupancy is read from ``scratch_database_session``, which samples the database
before any test has run, rather than from the live tables here. Sampling live was
wrong in a way nothing in the summary line showed: pytest runs the files
alphabetically, and nine of the files before this one create organizations of their
own, so by the time this guard was first requested the tables were populated *by
this session*. It read that as evidence the database was not a test database and
skipped 27 of the 63 tests, on a scratch database that had been empty moments
before. The refusal is the right rule; sampling before the suite has touched
anything is the only moment at which the answer means what the rule intends.
"""
dsn = _integration_dsn()
if dsn is None or not _destructive_run_acknowledged() or "umbracontrol" in dsn:
yield
return
counts = _occupancy(dsn)
populated = {name: count for name, count in counts.items() if count}
if not scratch_database_session["enabled"]:
pytest.skip("the scratch-database session fixture is not active for this DSN")
populated = scratch_database_session["occupied"]
if populated:
detail = ", ".join(f"{name}={count}" for name, count in sorted(populated.items()))
pytest.skip(
"refusing to run the adapter suite against a database that already holds data "
f"({detail}). This suite empties those tables when it finishes and cannot do so "
"selectively, because audit_events is append-only. Use a scratch database."
f"at the start of the session ({detail}). This suite empties those tables when it "
"finishes and cannot do so selectively, because audit_events is append-only. Use a "
"scratch database."
)
yield
_drop_tenant_schemas(dsn)

287
umbratrans_web/tests/test_scratch_database_safety.py Normal file
View File

@@ -0,0 +1,287 @@
"""A run must never change a database it refused to run against.
`require_empty_database` exists to stop the suite emptying somebody's data. It works by
skipping: point the suite at a populated database and 27 of the 63 adapter tests decline
to run, because the suite truncates indiscriminately and cannot do so selectively -
`audit_events` is append-only, with a trigger that raises on delete.
The trap this file covers is what happens *after* those tests skip. The refusal only
protects the database if nothing else in the session then writes to it, and for a while
something did: the session-wide cleanup fixture truncated on teardown unconditionally.
So the suite refused to touch the data, every test that could have harmed it skipped
itself, and one fixture later the data was gone anyway. The observable result was a run
that skipped the dangerous tests and destroyed the database - the worst of both, and it
reported success.
Found by planting an organization, watching the suite refuse, and finding the
organization gone afterwards. The fix is a branch that leaves an occupied database
exactly as found. This is the test that says it must stay that way.
It runs pytest in a subprocess because the property is about what a whole session does
to a database, which no in-process fixture assertion can observe. It asserts on the
database afterwards rather than on the source, because a source check would be satisfied
by a comment.
"""
from __future__ import annotations
import os
import re
import subprocess
import sys
from pathlib import Path
from typing import Any
import pytest
TESTS = Path(__file__).resolve().parent
REPO = TESTS.parents[1]
DSN_ENV = "UMBRATRANS_TEST_DSN"
ACK_ENV = "UMBRATRANS_TEST_DATABASE_IS_DISPOSABLE"
#: A slug nothing else in the suite uses, so a leftover row cannot be mistaken for a
#: surviving one.
PLANTED_SLUG = "test-planted-organization-must-survive"
_TENANT_SCHEMAS = "SELECT nspname FROM pg_namespace WHERE nspname LIKE 'tenant\\_%' ORDER BY nspname"
pytestmark = pytest.mark.skipif(
not os.environ.get(DSN_ENV)
or os.environ.get(ACK_ENV, "").strip() not in {"1", "true", "yes", "on"}
or "umbracontrol" in os.environ.get(DSN_ENV, ""),
reason=f"needs {DSN_ENV} and an acknowledged disposable {ACK_ENV}",
)
def _connect() -> Any:
import psycopg
from psycopg.rows import dict_row
return psycopg.connect(os.environ[DSN_ENV], row_factory=dict_row, autocommit=True)
def _schemas(connection: Any) -> set[str]:
return {row["nspname"] for row in connection.execute(_TENANT_SCHEMAS).fetchall()}
def _plant(connection: Any) -> set[str]:
"""Insert an organization, returning the tenant schemas its trigger created."""
before = _schemas(connection)
connection.execute("DELETE FROM public.organizations WHERE slug = %s", (PLANTED_SLUG,))
connection.execute(
"INSERT INTO public.organizations (id, name, slug, status, created_at) "
"VALUES (gen_random_uuid(), %s, %s, 'active', now())",
("Test Planted Organization", PLANTED_SLUG),
)
created = _schemas(connection) - before
present = connection.execute(
"SELECT count(*) AS total FROM public.organizations WHERE slug = %s", (PLANTED_SLUG,)
).fetchone()["total"]
assert present == 1, "the organization was not planted, so nothing is being proved"
assert created, "the plant created no tenant schema, so it is not the shape of data at risk"
return created
def _unplant(connection: Any, created: set[str]) -> None:
"""Undo the plant completely - the row *and* the schemas its trigger made.
Dropping the row alone leaves the schema, and schemas are what accumulate into the
lock exhaustion this whole exercise came from. A test that leaked two schemas per run
would be the very defect it exists to catch.
"""
for name in created:
connection.execute(f'DROP SCHEMA IF EXISTS "{name}" CASCADE')
connection.execute("DELETE FROM public.organizations WHERE slug = %s", (PLANTED_SLUG,))
def _run_adapter_suite() -> subprocess.CompletedProcess[str]:
"""A whole session, in a subprocess, against whatever the database holds now."""
return subprocess.run( # noqa: S603 - fixed argv, no shell
[
sys.executable,
"-m",
"pytest",
"umbratrans_web/tests/test_postgres_repository.py",
"-q",
"--no-header",
"-p",
"no:cacheprovider",
],
cwd=REPO,
capture_output=True,
text=True,
timeout=300,
check=False,
)
@pytest.fixture
def planted() -> Any:
connection = _connect()
created: set[str] = set()
try:
created = _plant(connection)
yield connection
finally:
_unplant(connection, created)
connection.close()
def test_a_planted_organization_survives_a_run(planted: Any) -> None:
"""The refusal must be real, not merely reported."""
result = _run_adapter_suite()
survived = planted.execute(
"SELECT count(*) AS total FROM public.organizations WHERE slug = %s", (PLANTED_SLUG,)
).fetchone()["total"]
assert survived == 1, (
"the suite destroyed an organization that was present when the session began. It "
"refused to run the destructive tests and then removed the data during teardown "
f"anyway. Subprocess output was:\n{result.stdout[-2000:]}"
)
def test_the_destructive_tests_actually_declined(planted: Any) -> None:
"""The survival above only means something if the tests really did refuse.
Without this, a suite that emptied the database without ever reaching its own guard
would pass the test above for the wrong reason - and a test that passes because the
mechanism it is meant to check never ran is the failure this project keeps making.
"""
result = _run_adapter_suite()
assert result.returncode == 0, result.stdout[-2000:]
assert "27 skipped" in result.stdout, (
"expected the adapter suite to skip its destructive tests against a populated "
f"database, and it did not. Subprocess output was:\n{result.stdout[-2000:]}"
)
def test_the_refusal_is_reported_rather_than_silent(planted: Any) -> None:
"""A run that changed nothing must not look like a run that succeeded.
Asserted on the subprocess output rather than on the exit code: pytest exits zero
when tests are skipped, which is precisely why the earlier version of this went
unnoticed. The message also has to survive pytest's file-descriptor capture - a
plain write to stderr is swallowed and never shown, which was the first attempt.
"""
result = _run_adapter_suite()
assert "left the database untouched" in result.stdout, (
"a run that declined to touch the database did not say so. Without this line a "
"refusal is indistinguishable in the output from a normal passing run:\n"
f"{result.stdout[-2000:]}"
)
@pytest.fixture
def empty_disposable_database() -> Any:
"""Empty the database so the subprocess below starts from nothing.
This test is the awkward one. The property only exists in a session that begins with
an empty database, and the enclosing session will not be empty when this runs - the
files before it have created organizations, which is the very condition being tested.
So the test clears the database itself first.
Creating a private database instead would be cleaner, and was tried: `umbra` has no
CREATE DATABASE on this server, so `CREATE DATABASE umbratrans_iso_probe` fails with
permission denied and there is no way to run these subprocesses against a database of
their own.
Clearing it is safe because the disposal is acknowledged - the gate is the same two
environment variables the whole suite runs behind - and because the subprocess is the
only thing that needs the emptiness. Anything running afterwards creates its own
organizations; none of this suite carries state in the database between files, which
is why a test file that needed this could not simply rely on ordering.
"""
connection = _connect()
for name in _schemas(connection):
connection.execute(f'DROP SCHEMA IF EXISTS "{name}" CASCADE')
connection.execute(
"TRUNCATE public.artifacts, public.audit_events, public.authors, public.compute_nodes, "
"public.jobs, public.node_pairing_codes, public.publishers, public.user_roles, "
"public.users, public.organizations CASCADE"
)
try:
yield connection
finally:
connection.close()
def test_tests_that_make_organizations_do_not_disable_the_adapter_suite(
empty_disposable_database: Any,
) -> None:
"""The database tests must actually run, in the order the suite runs them.
This is the quieter half of the same defect, and the more expensive one.
`require_empty_database` samples occupancy to decide whether the database is safe to
truncate. It used to read the live tables when that decision was made, which is after
pytest has already run the files ordered before it - and several of those create
organizations and tenant schemas. It read this session's own rows as pre-existing data
and skipped 27 of the 63 adapter tests, on a scratch database that had been empty
moments earlier. The summary line read "567 passed" and nothing else said otherwise.
So the sample has to be taken before the session touches anything. Running
`test_job_project_tenancy.py` first reproduces the ordering; with the guard sampling
late, the adapter tests skip here exactly as they did in the full suite, and with it
sampling at session start they do not.
`test_rls_tenancy.py` was the first choice for the leading file and was wrong: it
cleans up after itself, so it reproduced nothing and the test passed against the
broken guard. The file has to be one that leaves its organizations behind.
Asserted as "no skips", because the failure mode is a suite that looks green while
not testing the database, and a test that only checked the exit code would pass in
exactly that case.
"""
result = subprocess.run( # noqa: S603 - fixed argv, no shell
[
sys.executable,
"-m",
"pytest",
"umbratrans_web/tests/test_job_project_tenancy.py",
"umbratrans_web/tests/test_postgres_repository.py",
"-q",
"--no-header",
"-p",
"no:cacheprovider",
"-rs",
],
cwd=REPO,
capture_output=True,
text=True,
timeout=300,
check=False,
)
output = result.stdout[-3000:]
assert result.returncode == 0, output
# -rs above is not optional. Without it pytest prints no skip reasons, so the first
# assertion below would pass against a guard that skipped everything it was meant to
# catch - which is what happened the first time this was written.
assert "refusing to run the adapter suite" not in output, (
"the adapter suite skipped itself because it sampled the database after earlier "
"files had already created organizations in this same session. The sample has to "
f"be taken at session start, before collection. Output was:\n{output}"
)
# Read from the summary line rather than by searching for the word "skipped": pytest
# prints that summary *after* the warnings section, so an assertion that stops at the
# warnings header never sees it. That mistake hid the same failure a second time.
summary = output.strip().splitlines()[-1] if output.strip() else ""
skipped = re.search(r"(\d+) skipped", summary)
assert skipped is None, (
"expected no skips in this ordering, and the summary reports some. This test file "
f"had been passing against the very defect it was written for. Summary was: "
f"{summary!r}\nFull output was:\n{output}"
)