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

Bắt đầu trong .github/workflows/check_python.yml để kiểm tra cách detect hiện đang điều khiển unit-tests và integration-tests. Sau đó kiểm tra tests/unit/utils/test_conf_path.py, tests/integration/conftest.py và pyproject.toml để tìm các lỗi cụ thể về đường dẫn Windows và khả dụng của Docker, cùng với các thiếu sót trong cấu hình pytest. Chạy bộ kiểm thử unit, sau đó thêm hoặc xác minh các kiểm thử contract và observability trong tests/contract/ và tests/unit/observability/ theo các tệp được đề xuất. Công việc được hoàn tất khi CI không còn bỏ qua các thay đổi cấu hình, việc thu thập kiểm thử integration được bỏ qua một cách sạch sẽ khi không có Docker và các kiểm thử mới thực thi tính nhất quán của contract route/schema/sql/auth.

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 #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, #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 #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 #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

• #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.
• #189 — aggregated Postgres writer for the status change topic (the module with the weakest unit coverage, G1).
• #193 — service logging improvements.