AbsaOSS / AbsaOSS/EventGate

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

Đang mở
#210 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
type:tech-debt
Ngôn ngữ chính
Python
Star
4
Fork
0
Merge trung bình
1 ngày 5 giờ
Pull request đã merge (30 ngày)
8

Mô tả

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

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.