AbsaOSS / AbsaOSS/EventGate

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

Aperta
#210 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
type:tech-debt
Lingua principale
Python
Stelle
4
Fork
0
Merge medio
1g 5h
PR unite (30g)
8

Descrizione

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

```python
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](https://github.com/AbsaOSS/EventGate/pull/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:

```bash
--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.

```python
@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 `400`s 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//*.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.

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.