Test suite: close breaking-change gaps (CI gating, cross-platform runs, contract & log-contract layers)

[oto-macenauer-absa]

Description of Technical Debt

The automated test suite is broad (235 unit tests, 45 integration tests, 94% line coverage on src/) and the integration layer is genuinely good — real Postgres 16 and Kafka via testcontainers, moto for S3/Secrets Manager/EventBridge, an in-process mock JWT provider, and the Lambda handlers invoked through real API Gateway proxy events.

What it does not currently guarantee is the thing we actually want from it: that a breaking change to existing functionality fails CI. There are three structural holes and a set of narrower coverage gaps.

The suite as measured today (local run, master):

Layer	Tests	Result
Unit (tests/unit/)	237	235 passed, 2 failed on Windows
Integration (tests/integration/)	45	45 passed in 26 s
Coverage on src/	—	94% (lowest file: writer_postgres.py at 79%)

Structural holes

H1 — CI runs no tests at all when non-Python files change.
.github/workflows/check_python.yml gates every job (including unit-tests and integration-tests) behind the detect job, which only looks for changed *.py and requirements*.txt files. A PR that touches only conf/topic_schemas/*.json, conf/access.json, conf/config.json, conf/topic_keys.json, api.yaml, src/**/sql/*.sql, or Dockerfile hits the noop job and merges green.

Those are precisely the highest-risk breaking-change surfaces:

• tightening required in a topic schema → existing producers start getting 400
• editing src/writers/sql/inserts.sql named parameters → WriterPostgres breaks at runtime
• editing access.json → silent 403 for a tenant

H2 — Two unit tests fail on Windows, so make qa cannot pass locally.
tests/unit/utils/test_conf_path.py hardcodes the POSIX separator:

assert conf_dir.endswith("pkg/conf")                    # line 82
assert conf_dir.endswith("pkg_invalid_current/conf")    # line 121

On Windows resolve_conf_dir() returns ...\pkg\conf, so both fail. CI is ubuntu-latest and never sees it. Fix by comparing Path objects or os.path.join("pkg", "conf").

H3 — The integration suite hard-errors instead of skipping when Docker is unavailable.
tests/integration/conftest.py::_prepull_images is scope="session", autouse=True and calls docker.from_env(timeout=300) unconditionally. With no Docker daemon this raises DockerException during collection and the entire suite errors out — make qa gives a stack trace rather than a skip. Contributors without Docker have no usable local QA path.

Coverage gaps

G1 — WriterPostgres._upsert_status_change has no unit test.
writer_postgres.py lines 171–189 (the whole event_type → created_at/started_at/finished_at mapping for JobCreatedEvent, JobCreatedAndStartedEvent, JobStartedEvent, JobFinishedEvent) is exercised only by tests/integration/test_status_change_writer.py. Combined with H1 and H3 that means the newest writer (added in #189) has no regression net on a config-only PR or on a machine without Docker.

G2 — Nothing enforces api.yaml ↔ ROUTE_MAP consistency.
Both currently list the same 8 routes, but they are maintained by hand on both sides. /docs, /stats/{topic_name} and /terminate were each added twice, manually. Drift is undetected until a consumer reads the spec.

G3 — Nothing enforces topic JSON Schema ↔ Postgres writer field access.
_insert_dlchange, _insert_run and _insert_test use direct subscript access for required fields (message["catalog_id"], message["job_ref"], job["status"], …). If a field is dropped from required in the topic schema, validation passes and the writer then raises KeyError → 500. This is a config-only change, so per H1 it also runs no tests.

G4 — The Postgres DDL exists only inside test code.
tests/integration/schemas/postgres_schema.py::SCHEMA_SQL is the only schema definition in the repository; the authoritative production DDL lives elsewhere. Integration tests can pass against a table shape that no longer matches production.

G5 — Nothing verifies the deployable artifact.
The Dockerfile flattens src/, conf/ and api.yaml into ${LAMBDA_TASK_ROOT}. conf_path.py has a dedicated resolution branch (scenario 3) for exactly that layout, but it is only tested against synthetic temp directories. No test builds the image and confirms src.event_gate_lambda.lambda_handler imports and answers /health.

G6 — Coverage gates are inconsistent and far below actual.

• CI: pytest --cov=. -v tests/unit/ --cov-fail-under=80
• Makefile: pytest tests/unit/ --cov=src --cov-fail-under=90
• Actual: 94%

The effective ratchet permits a 14-point regression. There is also no per-file floor, so writer_postgres.py at 79% hides behind the aggregate, and no coverage report is published on the PR.

G7 — No pytest configuration.
pyproject.toml has no [tool.pytest.ini_options]: no testpaths, no registered markers (so there is no -m "not integration" escape hatch), and no filterwarnings. The suite already emits, in four integration modules:

PytestRemovedIn10Warning: Class-scoped fixture defined as instance method is deprecated.

That is a silent break waiting for the next pytest major bump.

G8 — Auth negative tests are thin.
Covered: expired token, wrong signing key, missing token, unauthorized sub, case-insensitive sub match. Not covered: alg=none, an HS256 token signed with the RSA public key (algorithm confusion), a token with no sub claim, a non-string sub, and Authorization values with embedded whitespace/newlines. decode_jwt pins algorithms=["RS256"] so these should all be rejected — which is exactly why they deserve regression tests.

G9 — Repository hygiene.

• .coverage is tracked in git despite being listed in .gitignore (PR feat(logging): structured logs with request correlation via Powertools #204 happens to delete it).
• tests/integration/.tmp_conf/ is written by lambda_handler_factory and only removed if it ends up empty; it is not in .gitignore.

Interaction with PR #204 (structured logging)

#204 (feature/193-improve-service-logging) is well tested for its own surface and should not be blocked by this issue. It adds tests/unit/utils/test_observability.py (13 tests, including correlation-id validation, cold-start flipping and request-scoped-key leakage between warm invocations), tests/unit/utils/test_logging_levels.py, 9 new test_utils.py tests for the rewritten dispatch_request, and a TestCorrelationId class in the integration health tests.

Two things it changes are worth folding into this work rather than leaving implicit:

1. dispatch_request now catches bare Exception at the boundary instead of a tuple of six exception types. That is the right call, but it means any handler bug now silently becomes a logged 500 instead of surfacing. The log-contract layer proposed below is what keeps that from becoming a blind spot.
2. HandlerTopic.handle_request gained JSON-body validation (400 for a non-JSON or non-object body) and resolve_request_topic gained a 400 for a missing topic_name path parameter. These are new externally visible status codes that api.yaml does not document — G2 would catch that.

Impact of Technical Debt

• A config-only PR can break production with a fully green CI run. This is the concrete, current risk (H1).
• Contributors on Windows cannot run make qa (H2), and contributors without Docker get a crash rather than a skip (H3) — both push verification onto CI, which per H1 may not run.
• The newest writer (status_change, Add aggregated Postgres writer for status change topic #189) has the weakest unit coverage of any module (G1).
• Schema/spec/SQL drift is invisible until runtime (G2, G3, G4).
• 94% line coverage overstates confidence: the gate is set at 80, and line coverage says nothing about assertion strength.

Category

Testing / Test Coverage

Priority

Medium - Should be addressed soon

Proposed Solution

Ordered by value per unit of effort.

1. Make CI actually run (H1) — highest value, smallest change

In check_python.yml, split the concerns:

• Keep the detect gate for pylint-analysis, black-check and mypy-check (they genuinely only care about *.py).
• Run unit-tests unconditionally on every PR. It takes ~10 s.
• Extend the detection glob for integration-tests to include the behavioural surfaces:

--jq '.[].filename | select(
  endswith(".py") or endswith(".sql") or endswith(".yaml") or
  (startswith("conf/")) or (startswith("requirements")) or
  . == "Dockerfile" or . == "api.yaml"
)'

2. Fix cross-platform and no-Docker execution (H2, H3)

• test_conf_path.py: replace endswith("pkg/conf") with Path(conf_dir) == module_dir / "conf".
• tests/integration/conftest.py: probe the daemon once and skip cleanly.

@pytest.fixture(scope="session", autouse=True)
def _prepull_images() -> None:
    try:
        client = docker.from_env(timeout=300)
        client.ping()
    except docker.errors.DockerException as exc:
        pytest.skip(f"Docker is not available, skipping integration tests: {exc}", allow_module_level=True)
    ...

Add tests/integration/.tmp_conf/ to .gitignore and git rm --cached .coverage.

3. Add a contract test layer — new, fast, no Docker required

A new tests/contract/ package that runs on every PR in well under a second and covers exactly the drift H1 leaves open:

• test_api_spec_matches_routes.py — parse api.yaml paths, assert set equality against event_gate_lambda.ROUTE_MAP ∪ event_stats_lambda.ROUTE_MAP; assert every status code a handler can return is documented (this immediately catches the new 400s from feat(logging): structured logs with request correlation via Powertools #204).
• test_schema_matches_writer.py — for each topic, assert every key WriterPostgres accesses via subscript is present in the schema's required array.
• test_sql_params_match_writer.py — extract %(name)s placeholders from src/writers/sql/inserts.sql and src/readers/sql/stats.sql, assert they equal the dict keys passed by the writer/reader.
• test_config_consistency.py — assert constants.TOPIC_* ≡ files in conf/topic_schemas/ ≡ keys in access.json, and that every topic_keys.json key is a known topic and every value is a field defined in that topic's schema.

4. Add a log-contract test layer — protects PR #204's value

Under tests/unit/observability/, drive the handler with caplog/captured stdout and assert:

• every emitted record is valid JSON with service, level, message, correlation_id, cold_start, resource;
• no record ever contains a raw bearer token, a password, or a Secrets Manager payload;
• no request-scoped key (user, topic) survives from a POST /topics/{topic} into a following GET /health on the same warm container — the integration-level counterpart to the unit test feat(logging): structured logs with request correlation via Powertools #204 already has;
• LOG_LEVEL=TRACE produces redacted, size-capped payloads (safe_serialize_for_log is well unit-tested; this checks the wiring).

5. Close the coverage gaps

• Parametrized unit tests for _upsert_status_change over all four event_type values plus an unknown one, asserting the exact created_at/started_at/finished_at triple (G1).
• Auth negative tests: alg=none, HS256-signed-with-public-key, missing sub, non-string sub (G8).
• Align both gates on --cov=src --cov-fail-under=93, add --cov-report=xml plus a PR coverage comment, and add a per-file floor so no module drops below ~85% (G6).
• Add [tool.pytest.ini_options] with testpaths, registered unit/integration/contract markers, --strict-markers, and filterwarnings = ["error::DeprecationWarning", ...]; fix the class-scoped-fixture deprecation it surfaces (G7).

6. Optional / follow-up layers

• Container smoke job — docker build the Lambda image, run it under the AWS Runtime Interface Emulator, and assert /health and /topics respond. Catches packaging and conf_path flattening breaks that no unit test can (G5).
• Golden payload corpus — tests/fixtures/payloads/<topic>/*.json of historically accepted messages, replayed through validation and the writers. Any schema tightening then fails a test instead of a producer in production. This is the direct answer to "catch breaking changes over existing functionality".
• Vendor the authoritative DDL into db/schema.sql and have the integration fixture load that file instead of SCHEMA_SQL (G4).
• Mutation testing (mutmut) over src/handlers/ and src/utils/, nightly and non-blocking. 94% line coverage says nothing about assertion strength; this measures it.

Runner and local-environment considerations

• Unit + contract tests: no Docker, no network, run everywhere in ~10 s. Safe to make unconditional on PRs.
• Integration tests: keep ubuntu-latest; the existing parallel pre-pull with backoff already handles registry flakiness, and the 15-minute timeout is generous against a 26 s local run. Do not add pytest-xdist — the container fixtures are session-scoped and would need reworking first.
• After the H2/H3 fixes, make qa passes on Windows, macOS and Linux, with or without Docker.

Effort Estimate

~3–4 days total: ~0.5 day for H1–H3 and the hygiene fixes, ~1.5 days for the contract and log-contract layers, ~1 day for the coverage gaps and pytest/CI configuration. The container smoke job, golden corpus, DDL vendoring and mutation testing are a separate follow-up of similar size.

Dependencies / Related

• feat(logging): structured logs with request correlation via Powertools #204 — structured logging via Powertools (in review). Not blocked by this issue; the log-contract layer in step 4 and the api.yaml status-code check in step 3 are the natural follow-ups to it.
• Add aggregated Postgres writer for status change topic #189 — aggregated Postgres writer for the status change topic (the module with the weakest unit coverage, G1).
• Improve service logging #193 — service logging improvements.