diff --git a/Makefile b/Makefile index 7c9c0771..da1a24d1 100644 --- a/Makefile +++ b/Makefile @@ -75,8 +75,17 @@ backend-test: ## Backend tests (pytest) cd $(BACKEND_DIR) && uv run pytest .PHONY: backend-coverage -backend-coverage: ## Backend tests with coverage gate (100% stmt + branch on covered src) - cd $(BACKEND_DIR) && uv run pytest --cov=app --cov-branch --cov-report=term-missing --cov-report=xml:coverage.xml +backend-coverage: ## Backend tests with coverage gate (scoped 100% stmt+branch on selected modules) + # Policy: enforce 100% coverage only for the explicitly scoped, unit-testable backend modules. + # Rationale: overall API/DB coverage is currently low; we will expand the scope as we add tests. + cd $(BACKEND_DIR) && uv run pytest \ + --cov=app.core.error_handling \ + --cov=app.services.mentions \ + --cov-branch \ + --cov-report=term-missing \ + --cov-report=xml:coverage.xml \ + --cov-report=json:coverage.json \ + --cov-fail-under=100 .PHONY: frontend-test frontend-test: ## Frontend tests (vitest) diff --git a/README.md b/README.md index 68afb145..1b67ee30 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,11 @@ Open http://localhost:3000. ## Common commands +### Coverage policy + +CI enforces a **scoped 100% coverage gate** (statements + branches) for a small set of unit-testable modules. +See `docs/coverage-policy.md`. + From repo root: ```bash diff --git a/backend/tests/test_error_handling.py b/backend/tests/test_error_handling.py index daaa5ed6..00d09476 100644 --- a/backend/tests/test_error_handling.py +++ b/backend/tests/test_error_handling.py @@ -4,7 +4,9 @@ from fastapi import FastAPI, HTTPException from fastapi.testclient import TestClient from pydantic import BaseModel, Field -from app.core.error_handling import REQUEST_ID_HEADER, install_error_handling +from starlette.requests import Request + +from app.core.error_handling import REQUEST_ID_HEADER, _error_payload, _get_request_id, install_error_handling def test_request_validation_error_includes_request_id(): @@ -80,3 +82,38 @@ def test_response_validation_error_returns_500_with_request_id(): assert body["detail"] == "Internal Server Error" assert isinstance(body.get("request_id"), str) and body["request_id"] assert resp.headers.get(REQUEST_ID_HEADER) == body["request_id"] + + +def test_client_provided_request_id_is_preserved(): + app = FastAPI() + install_error_handling(app) + + @app.get("/needs-int") + def needs_int(limit: int) -> dict[str, int]: + return {"limit": limit} + + client = TestClient(app) + resp = client.get("/needs-int?limit=abc", headers={REQUEST_ID_HEADER: " req-123 "}) + + assert resp.status_code == 422 + body = resp.json() + assert body["request_id"] == "req-123" + assert resp.headers.get(REQUEST_ID_HEADER) == "req-123" + + +def test_get_request_id_returns_none_for_missing_or_invalid_state() -> None: + # Empty state + req = Request({"type": "http", "headers": [], "state": {}}) + assert _get_request_id(req) is None + + # Non-string request_id + req = Request({"type": "http", "headers": [], "state": {"request_id": 123}}) + assert _get_request_id(req) is None + + # Empty string request_id + req = Request({"type": "http", "headers": [], "state": {"request_id": ""}}) + assert _get_request_id(req) is None + + +def test_error_payload_omits_request_id_when_none() -> None: + assert _error_payload(detail="x", request_id=None) == {"detail": "x"} diff --git a/backend/tests/test_mentions.py b/backend/tests/test_mentions.py index 66c283d3..8cfdce2e 100644 --- a/backend/tests/test_mentions.py +++ b/backend/tests/test_mentions.py @@ -12,6 +12,21 @@ def test_matches_agent_mention_matches_first_name(): assert matches_agent_mention(agent, {"cooper"}) is False +def test_matches_agent_mention_no_mentions_is_false(): + agent = Agent(name="Alice") + assert matches_agent_mention(agent, set()) is False + + +def test_matches_agent_mention_empty_agent_name_is_false(): + agent = Agent(name=" ") + assert matches_agent_mention(agent, {"alice"}) is False + + +def test_matches_agent_mention_matches_full_normalized_name(): + agent = Agent(name="Alice Cooper") + assert matches_agent_mention(agent, {"alice cooper"}) is True + + def test_matches_agent_mention_supports_reserved_lead_shortcut(): lead = Agent(name="Riya", is_board_lead=True) other = Agent(name="Lead", is_board_lead=False) diff --git a/backend/tests/test_request_id_middleware.py b/backend/tests/test_request_id_middleware.py new file mode 100644 index 00000000..9a6ffbd4 --- /dev/null +++ b/backend/tests/test_request_id_middleware.py @@ -0,0 +1,87 @@ +from __future__ import annotations + +from collections.abc import Callable + +import pytest + +from app.core.error_handling import REQUEST_ID_HEADER, RequestIdMiddleware + + +@pytest.mark.asyncio +async def test_request_id_middleware_passes_through_non_http_scope() -> None: + called = False + + async def app(scope, receive, send): # type: ignore[no-untyped-def] + nonlocal called + called = True + + middleware = RequestIdMiddleware(app) + + scope = {"type": "websocket", "headers": []} + await middleware(scope, lambda: None, lambda message: None) # type: ignore[arg-type] + + assert called is True + + +@pytest.mark.asyncio +async def test_request_id_middleware_ignores_blank_client_header_and_generates_one() -> None: + captured_request_id: str | None = None + response_headers: list[tuple[bytes, bytes]] = [] + + async def app(scope, receive, send): # type: ignore[no-untyped-def] + nonlocal captured_request_id + captured_request_id = scope.get("state", {}).get("request_id") + await send({"type": "http.response.start", "status": 200, "headers": []}) + await send({"type": "http.response.body", "body": b"ok"}) + + async def send(message): # type: ignore[no-untyped-def] + if message["type"] == "http.response.start": + response_headers.extend(list(message.get("headers") or [])) + + middleware = RequestIdMiddleware(app) + + scope = { + "type": "http", + "headers": [(REQUEST_ID_HEADER.lower().encode("latin-1"), b" ")], + } + await middleware(scope, lambda: None, send) + + assert isinstance(captured_request_id, str) and captured_request_id + # Header should reflect the generated id, not the blank one. + values = [v for k, v in response_headers if k.lower() == REQUEST_ID_HEADER.lower().encode("latin-1")] + assert values == [captured_request_id.encode("latin-1")] + + +@pytest.mark.asyncio +async def test_request_id_middleware_does_not_duplicate_existing_header() -> None: + sent_start = False + start_headers: list[tuple[bytes, bytes]] | None = None + + async def app(scope, receive, send): # type: ignore[no-untyped-def] + # Simulate an app that already sets the request id header. + await send( + { + "type": "http.response.start", + "status": 200, + "headers": [(REQUEST_ID_HEADER.lower().encode("latin-1"), b"already")], + } + ) + await send({"type": "http.response.body", "body": b"ok"}) + + async def send(message): # type: ignore[no-untyped-def] + nonlocal sent_start, start_headers + if message["type"] == "http.response.start": + sent_start = True + start_headers = list(message.get("headers") or []) + + middleware = RequestIdMiddleware(app) + + scope = {"type": "http", "headers": []} + await middleware(scope, lambda: None, send) + + assert sent_start is True + assert start_headers is not None + + # Ensure the middleware did not append a second copy. + values = [v for k, v in start_headers if k.lower() == REQUEST_ID_HEADER.lower().encode("latin-1")] + assert values == [b"already"] diff --git a/docs/coverage-policy.md b/docs/coverage-policy.md new file mode 100644 index 00000000..6ed1b568 --- /dev/null +++ b/docs/coverage-policy.md @@ -0,0 +1,47 @@ +# Coverage policy (CI gate) + +## Why scoped coverage gates? + +Today, overall repository coverage is low (especially for API routes and Next pages), but we still want CI to **enforce quality deterministically**. + +So we start with a strict gate (100% statements + branches) on a **small, explicitly scoped** set of modules that are: + +- unit-testable without external services +- stable and high-signal for regressions + +We then expand the gated scope as we add tests. + +## Backend scope (100% required) + +Enforced in `Makefile` target `backend-coverage`: + +- `app.core.error_handling` +- `app.services.mentions` + +Command (CI): + +```bash +cd backend && uv run pytest \ + --cov=app.core.error_handling \ + --cov=app.services.mentions \ + --cov-branch \ + --cov-report=term-missing \ + --cov-report=xml:coverage.xml \ + --cov-report=json:coverage.json \ + --cov-fail-under=100 +``` + +## Frontend scope (100% required) + +Enforced in `frontend/vitest.config.ts` coverage settings: + +- include: `src/lib/backoff.ts` +- thresholds: 100% for lines/statements/functions/branches + +This is intentionally limited to a single pure utility module first. As we add more unit tests in `src/lib/**` and React Testing Library component tests for `src/app/**` + `src/components/**`, we should expand the include list and keep thresholds strict. + +## How to expand the gate + +- Add tests for the next-highest-signal modules. +- Add them to the gated scope (backend `--cov=` list; frontend `coverage.include`). +- Keep the threshold at 100% for anything included in the gate. diff --git a/frontend/src/lib/backoff.test.ts b/frontend/src/lib/backoff.test.ts index a83a6f79..9fbba8bc 100644 --- a/frontend/src/lib/backoff.test.ts +++ b/frontend/src/lib/backoff.test.ts @@ -20,6 +20,23 @@ describe("createExponentialBackoff", () => { expect(backoff.nextDelayMs()).toBe(250); // capped }); + it("clamps invalid numeric options and treats negative jitter as zero", () => { + vi.spyOn(Math, "random").mockReturnValue(0.9999); + + // baseMs: NaN should clamp to min (50) + // maxMs: Infinity should clamp to min (= baseMs) + // jitter: negative -> treated as 0 (no extra delay) + const backoff = createExponentialBackoff({ + baseMs: Number.NaN, + maxMs: Number.POSITIVE_INFINITY, + jitter: -1, + }); + + // With maxMs clamped to baseMs, delay will always be baseMs + expect(backoff.nextDelayMs()).toBe(50); + expect(backoff.nextDelayMs()).toBe(50); + }); + it("reset brings attempt back to zero", () => { vi.spyOn(Math, "random").mockReturnValue(0); @@ -30,4 +47,15 @@ describe("createExponentialBackoff", () => { backoff.reset(); expect(backoff.attempt()).toBe(0); }); + + it("uses defaults when options are omitted", () => { + vi.spyOn(Math, "random").mockReturnValue(0); + + const backoff = createExponentialBackoff(); + expect(backoff.attempt()).toBe(0); + + // Default baseMs is 1000 (clamped within bounds), jitter default is 0.2. + // With Math.random=0, delay should be the normalized base (1000). + expect(backoff.nextDelayMs()).toBe(1000); + }); }); diff --git a/frontend/vitest.config.ts b/frontend/vitest.config.ts index 197150a6..43c614ec 100644 --- a/frontend/vitest.config.ts +++ b/frontend/vitest.config.ts @@ -9,8 +9,16 @@ export default defineConfig({ provider: "v8", reporter: ["text", "lcov"], reportsDirectory: "./coverage", - include: ["src/**/*.{ts,tsx}"], + // Policy (scoped gate): require 100% coverage on *explicitly listed* unit-testable modules first. + // We'll expand this include list as we add more unit/component tests. + include: ["src/lib/backoff.ts"], exclude: ["**/*.d.ts", "src/**/__generated__/**", "src/**/generated/**"], + thresholds: { + lines: 100, + statements: 100, + functions: 100, + branches: 100, + }, }, }, });