Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions .github/workflows/e2e-bronze-to-api.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,14 @@ on:
# (clickhouse + mariadb + dbt + pytest) on the merge commit just burns
# ~30 minutes for an outcome we already proved. workflow_dispatch
# remains for manual reruns against main.
#
# Deliberately NO `paths:` filter: this check is meant to be a required
# status check on main, and a path-filtered required check never reports
# on PRs outside the filter — they hang on "Expected" forever. Running on
# every PR keeps the gate uniform; the serial suite costs ~4 min with a
# warm cargo cache.
pull_request:
branches: [main]
paths:
- "src/ingestion/**"
- "src/backend/services/analytics-api/**"
- "src/backend/libs/insight-clickhouse/**"
- "src/backend/Cargo.toml"
- "src/backend/Cargo.lock"
- ".github/workflows/e2e-bronze-to-api.yml"
workflow_dispatch:

env:
Expand Down Expand Up @@ -68,8 +67,14 @@ jobs:
wait

# ─── Run the suite inside the runner ────────────────────────────────
# Serial on purpose: the session rig is not xdist-safe yet (see
# conftest.py — per-worker analytics-api spawns race SeaORM migrations
# in the shared MariaDB, non-primary workers don't wait for ClickHouse
# migrations, and the shared dbt target/ dir is deleted by whichever
# worker finishes first). Wall-time cost is negligible: the image and
# cargo builds dominate the job.
- name: Run E2E suite
run: ./e2e.sh test -n auto --tb=short -q
run: ./e2e.sh test --tb=short -q

# ─── Diagnostics on failure ─────────────────────────────────────────
- name: Dump compose logs on failure
Expand Down
3 changes: 3 additions & 0 deletions src/ingestion/scripts/create-bronze-placeholders.sh
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,9 @@ CREATE TABLE IF NOT EXISTS silver.class_ai_dev_usage (
spec_lines Nullable(Float64),
session_count Nullable(Float64),
total_chat_messages Nullable(Float64),
cost_cents Nullable(UInt32),
prs_with_cc_count Nullable(UInt32),
prs_total_count Nullable(UInt32),
_version UInt64
) ENGINE = ReplacingMergeTree(_version) ORDER BY (email, day) COMMENT 'INSIGHT_PLACEHOLDER_v1';
SQL
Expand Down
8 changes: 4 additions & 4 deletions src/ingestion/tests/e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ See specs: [PRD](../../../../docs/domain/bronze-to-api-e2e/specs/PRD.md), [DESIG

## Prerequisites

Only one: **Docker Engine ≥ 24**. Everything else (Python 3.12, Rust 1.92, dbt-clickhouse, pytest, all deps) lives inside the runner image.
Only one: **Docker Engine ≥ 24**. Everything else (Python 3.12, Rust matching `rust-version` in `src/backend/Cargo.toml`, dbt-clickhouse, pytest, all deps) lives inside the runner image.

## Run (recommended — dockerized)

Expand All @@ -23,7 +23,7 @@ cd src/ingestion/tests/e2e
./e2e.sh build # build the runner image (one-time, ~3-5 min cold)
./e2e.sh test # full suite (includes people_smoke E2E)
./e2e.sh test -k people_smoke -v # one fixture
./e2e.sh test -n auto # parallel (pytest-xdist)
./e2e.sh test -n auto # ⚠️ parallel (pytest-xdist) — NOT supported yet: workers race on shared CH/MariaDB/dbt target
./e2e.sh shell # interactive bash inside the runner
./e2e.sh down # tear down compose stack + volumes
```
Expand All @@ -40,7 +40,7 @@ If you prefer to develop on the host (faster iteration on the test code itself),
python3.12 -m venv .venv
source .venv/bin/activate
pip install -e .
rustup update stable # ≥ 1.92 required for edition2024
rustup update stable # must satisfy rust-version in src/backend/Cargo.toml

pytest -k people_smoke -v # session-rig brings compose up automatically
```
Expand Down Expand Up @@ -83,5 +83,5 @@ These ports avoid conflict with `dev-up.sh` (which uses 8123 / 3306) and the dbt

## Notes for fixture authors

- Auth in `analytics-api` is a stub; requests work without a Bearer token. `insight_tenant_id` resolves to `00000000-0000-0000-0000-000000000000` (nil UUID) — your bronze CSV rows MUST use the same tenant.
- Auth in `analytics-api` requires no Bearer token, but its tenant middleware rejects requests without a non-nil tenant. The harness sends `X-Insight-Tenant-Id` with `e2e_lib.config.TEST_TENANT_ID` on every request and re-homes seeded metric definitions onto that tenant (`metric_seed.py`). The ClickHouse query path does not filter by tenant yet, so bronze CSV rows may use any tenant value.
- Metric definitions are auto-seeded by the analytics-api binary's SeaORM migrations. Look up the metric UUID with `GET /v1/metrics` once the session is up, or add overrides in `seed/metrics.yaml`.
10 changes: 6 additions & 4 deletions src/ingestion/tests/e2e/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,16 +125,18 @@ def analytics_api(ch_migrations_applied: SessionConfig):
"""Build + spawn the analytics-api binary. Its SeaORM migrations run on startup;
we then upsert any test-specific metrics from seed/metrics.yaml.

If `cargo build` fails (e.g. cargo < 1.92 lacks edition2024), every test
that requires this fixture is skipped — the rest of the framework still
runs against the data plane.
If `cargo build` fails, this is a hard FAIL — identical locally and in CI.
A skip here would make the whole transformation suite silently green while
testing nothing (e.g. when the runner's toolchain drifts behind the version
src/backend/Cargo.toml requires). If the binary can't build, the bronze→API
tests cannot run, so the only honest result is red.
"""
cfg = ch_migrations_applied
from e2e_lib.analytics_api import ApiSpawnError # local import to keep top clean
try:
binary = build(cfg)
except ApiSpawnError as e:
pytest.skip(f"analytics-api binary not buildable: {e}")
pytest.fail(f"analytics-api binary not buildable: {e}", pytrace=False)
port = find_free_port()
proc = AnalyticsApiProcess(cfg, binary, port)
proc.start()
Expand Down
78 changes: 62 additions & 16 deletions src/ingestion/tests/e2e/e2e_lib/analytics_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
sessions) and spawn the binary directly on the host (per DESIGN §4: a host
binary keeps target/ warm and avoids container I/O on the cargo hot path).

The auth middleware in analytics-api is currently a stub — the binary requires
no Bearer token (auth happens at the API Gateway, which we bypass). All
requests resolve to tenant_id = nil UUID.
analytics-api requires no Bearer token (auth happens at the API Gateway, which
we bypass), but its tenant middleware rejects requests without a resolvable
non-nil tenant. The harness therefore sends `X-Insight-Tenant-Id` with
`config.TEST_TENANT_ID` on every request — including /health polling — and
`metric_seed.seed_test_metrics` re-homes the seeded metric definitions onto
that tenant.
"""

from __future__ import annotations
Expand All @@ -24,14 +27,41 @@

import httpx

from e2e_lib.config import SessionConfig
from e2e_lib.config import SessionConfig, TENANT_HEADER, TEST_TENANT_ID
from e2e_lib.fixture_loader import Fixture

LOG = logging.getLogger("e2e.api")


MIN_CARGO_MAJOR = 1
MIN_CARGO_MINOR = 92 # src/backend/Cargo.toml requires `edition = "2024"`
def _required_cargo_version(repo_root: Path) -> tuple[int, int] | None:
"""Read the required toolchain version from the single source of truth:
`[workspace.package].rust-version` in src/backend/Cargo.toml.

A hardcoded constant here silently drifts behind the real requirement (it
was pinned at 1.92 while the crates moved to 1.95), which let a broken build
masquerade as "version OK". Reading Cargo.toml keeps the precheck honest.

Returns None if it can't be determined — the `cargo build` itself remains
the hard gate (it fails loudly), so the precheck is only for a nicer message.
"""
cargo_toml = repo_root / "src/backend/Cargo.toml"
try:
import tomllib

data = tomllib.loads(cargo_toml.read_text(encoding="utf-8"))
except (OSError, ValueError, ImportError):
return None
ver = (
data.get("workspace", {}).get("package", {}).get("rust-version")
or data.get("package", {}).get("rust-version")
)
if not ver:
return None
nums = str(ver).split(".")
try:
return int(nums[0]), int(nums[1])
except (IndexError, ValueError):
return None


def _cargo_version_at_least(cargo: str, *, major: int, minor: int) -> tuple[bool, str]:
Expand Down Expand Up @@ -139,13 +169,16 @@ def build(cfg: SessionConfig) -> Path:
"cargo executable not found on PATH or in standard rustup locations. "
"Install via `rustup` and ensure ~/.cargo/bin is on PATH (or set CARGO_HOME)."
)
ok, version = _cargo_version_at_least(cargo, major=MIN_CARGO_MAJOR, minor=MIN_CARGO_MINOR)
if not ok:
raise ApiSpawnError(
f"cargo {version} is too old — src/backend/Cargo.toml requires "
f"edition2024 (cargo ≥ {MIN_CARGO_MAJOR}.{MIN_CARGO_MINOR}). "
f"Run `rustup update stable` and retry."
)
version = "?"
required = _required_cargo_version(cfg.repo_root)
if required is not None:
ok, version = _cargo_version_at_least(cargo, major=required[0], minor=required[1])
if not ok:
raise ApiSpawnError(
f"cargo {version} is too old — src/backend/Cargo.toml requires "
f"rust-version ≥ {required[0]}.{required[1]}. "
f"Run `rustup update stable` and retry."
)
LOG.info("cargo build --release -p analytics-api (cargo=%s, version=%s)", cargo, version)
try:
result = subprocess.run(
Expand Down Expand Up @@ -232,8 +265,17 @@ def is_running(self) -> bool:
return self._proc is not None and self._proc.poll() is None

def client(self) -> httpx.Client:
"""Return an httpx.Client bound to this process's base URL."""
return httpx.Client(base_url=self.base_url, timeout=30.0)
"""Return an httpx.Client bound to this process's base URL.

Every request carries `X-Insight-Tenant-Id`: the tenant middleware sits
in front of all routes (including `/health`) and rejects requests with
no resolvable tenant, so the header is mandatory, not per-endpoint.
"""
return httpx.Client(
base_url=self.base_url,
timeout=30.0,
headers={TENANT_HEADER: str(TEST_TENANT_ID)},
)

def call_fixture(self, fixture: Fixture) -> ApiResponse:
"""Build a request from the fixture's spec.yaml, execute it, return ApiResponse.
Expand Down Expand Up @@ -263,7 +305,11 @@ def _wait_healthy(self, *, timeout_s: float) -> None:
f"{stdout[-2000:]}"
)
try:
with httpx.Client(base_url=self.base_url, timeout=2.0) as c:
with httpx.Client(
base_url=self.base_url,
timeout=2.0,
headers={TENANT_HEADER: str(TEST_TENANT_ID)},
) as c:
r = c.get("/health")
if r.status_code == 200:
LOG.info("analytics-api is healthy at %s", self.base_url)
Expand Down
15 changes: 15 additions & 0 deletions src/ingestion/tests/e2e/e2e_lib/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import os
import secrets
import string
import uuid
from dataclasses import dataclass, field
from pathlib import Path

Expand All @@ -19,6 +20,20 @@
_REPO_ROOT = Path(__file__).resolve().parents[5]


# Header analytics-api's tenant middleware reads to resolve the request tenant
# (auth.rs::TENANT_HEADER). The harness sends it on EVERY request.
TENANT_HEADER = "X-Insight-Tenant-Id"

# Session tenant for the whole e2e run. analytics-api's tenant middleware
# rejects the nil UUID (a non-identity value must not pin tenant context), so
# the harness cannot use 0000…0. Instead it seeds metric definitions under this
# non-nil tenant and sends it as `X-Insight-Tenant-Id` on every request. The
# ClickHouse query path does not filter by tenant yet (MVP — handlers.rs), so
# fixture data carries whatever tenant it likes; only the `metrics`-table lookup
# is tenant-scoped, and that is what we align here.
TEST_TENANT_ID = uuid.UUID("11111111-1111-1111-1111-111111111111")


def _random_password(length: int = 24) -> str:
alphabet = string.ascii_letters + string.digits
return "".join(secrets.choice(alphabet) for _ in range(length))
Expand Down
53 changes: 38 additions & 15 deletions src/ingestion/tests/e2e/e2e_lib/metric_seed.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,38 +18,61 @@
import yaml

from e2e_lib import mariadb
from e2e_lib.config import SessionConfig
from e2e_lib.config import SessionConfig, TEST_TENANT_ID

LOG = logging.getLogger("e2e.metric_seed")

# All test metrics live under the nil tenant — matches the auth-stub
# context in analytics-api/src/auth.rs.
# All e2e metric definitions live under TEST_TENANT_ID. The analytics-api tenant
# middleware rejects the nil UUID, and `find_enabled_metric` filters the
# `metrics` table by tenant, so both the prod metrics seeded by the binary's
# migrations (under the nil tenant) and our overrides must sit under the tenant
# the harness sends as `X-Insight-Tenant-Id`.
# SeaORM stores `.uuid()` columns as BINARY(16) in MariaDB, so we pass raw
# bytes — pymysql interprets a str as utf-8 (36 chars) and overflows.
TEST_TENANT = TEST_TENANT_ID.bytes
NIL_TENANT = uuid.UUID("00000000-0000-0000-0000-000000000000").bytes


def seed_test_metrics(cfg: SessionConfig, seed_path: Path | None = None) -> int:
"""Read seed/metrics.yaml and upsert into MariaDB.metrics. Returns row count."""
seed_path = seed_path or (cfg.repo_root / "src/ingestion/tests/e2e/seed/metrics.yaml")
if not seed_path.is_file():
LOG.debug("no seed file at %s — skipping", seed_path)
return 0
"""Align MariaDB.metrics with the e2e tenant, then upsert seed overrides.

raw = yaml.safe_load(seed_path.read_text(encoding="utf-8"))
overrides = (raw or {}).get("overrides") or []
if not overrides:
LOG.debug("seed file %s has no overrides — skipping", seed_path)
return 0
Runs after the analytics-api binary's SeaORM migrations have seeded the prod
metric catalog (under the nil tenant). Returns the number of override rows.
"""
seed_path = seed_path or (cfg.repo_root / "src/ingestion/tests/e2e/seed/metrics.yaml")
overrides: list[dict] = []
if seed_path.is_file():
raw = yaml.safe_load(seed_path.read_text(encoding="utf-8"))
overrides = (raw or {}).get("overrides") or []

with mariadb.connection(cfg) as conn:
with conn.cursor() as cur:
moved = _retenant_seeded_metrics(cur)
for row in overrides:
_upsert_metric(cur, row)
LOG.info("upserted %d test metric(s) from %s", len(overrides), seed_path.name)
LOG.info(
"re-tenanted %d migration-seeded metric(s) onto %s; upserted %d override(s)",
moved,
TEST_TENANT_ID,
len(overrides),
)
return len(overrides)


def _retenant_seeded_metrics(cur) -> int:
"""Move metrics the binary seeded under the nil tenant onto TEST_TENANT.

The query path's `find_enabled_metric` is tenant-scoped, so prod metrics
seeded under 0000…0 are invisible to a request that resolves to TEST_TENANT.
Re-homing them in the test DB (NOT in the migration source) keeps the fix
inside the harness and out of prod seeding. Idempotent."""
cur.execute(
"UPDATE metrics SET insight_tenant_id = %s WHERE insight_tenant_id = %s",
(TEST_TENANT, NIL_TENANT),
)
return cur.rowcount


def _upsert_metric(cur, row: dict) -> None:
required = {"id", "name", "query_ref"}
missing = required - row.keys()
Expand All @@ -70,7 +93,7 @@ def _upsert_metric(cur, row: dict) -> None:
""",
(
metric_id_bytes,
NIL_TENANT,
TEST_TENANT,
row["name"],
row.get("description", ""),
row["query_ref"],
Expand Down
31 changes: 19 additions & 12 deletions src/ingestion/tests/e2e/meta/test_ci_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,19 @@ def test_yaml_parses(workflow: dict) -> None:
assert "jobs" in workflow


def test_required_paths_in_filter(workflow: dict) -> None:
"""PR-touch path filter MUST cover ingestion, analytics-api, insight-clickhouse lib."""
def test_runs_on_every_pr(workflow: dict) -> None:
"""The suite MUST run on every PR — no `paths:` filter. It is meant to be
a required status check on main, and a path-filtered required check never
reports on PRs outside the filter, leaving them stuck on "Expected"."""
# PyYAML coerces `on:` (a YAML truthy key) to the boolean True. Accept either.
on = workflow.get("on") or workflow.get(True)
assert on, "workflow has no `on:` triggers"
pr_paths = set(on.get("pull_request", {}).get("paths", []))
for required in (
"src/ingestion/**",
"src/backend/services/analytics-api/**",
"src/backend/libs/insight-clickhouse/**",
):
assert required in pr_paths, f"PR path filter missing {required!r}"
pr = on.get("pull_request") or {}
assert "paths" not in pr and "paths-ignore" not in pr, (
"pull_request must not be path-filtered — as a required check it would "
"hang on 'Expected' for PRs outside the filter"
)
assert pr.get("branches") == ["main"]


def test_uses_local_runner_image(workflow: dict) -> None:
Expand All @@ -60,11 +61,17 @@ def test_ci_env_set_to_true(workflow: dict) -> None:
assert env.get("CI") == "true", "workflow must export CI=true to enforce snapshot guard"


def test_pytest_runs_with_xdist(workflow: dict) -> None:
"""The pytest invocation MUST use -n auto so the suite parallelizes."""
def test_pytest_runs_serial(workflow: dict) -> None:
"""The pytest invocation MUST NOT use xdist: the session rig is not
xdist-safe yet (per-worker analytics-api spawns race SeaORM migrations in
the shared MariaDB, non-primary workers don't wait for CH migrations, and
the shared dbt target/ dir is deleted by whichever worker finishes first —
see conftest.py). Flip this test back once worker isolation lands."""
job = next(iter(workflow["jobs"].values()))
test_step = next(s for s in job["steps"] if s.get("name") == "Run E2E suite")
assert "-n auto" in test_step["run"]
assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), (
"CI must run the e2e suite serially until the rig is xdist-safe"
)
Comment on lines +72 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Serialization guard is bypassable with attached -n form.

This assertion only blocks "-n " and terminal "-n", so an attached form like -nauto would pass while still enabling parallelism. Tighten the check to reject any -n token variant.

Suggested fix
-    assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), (
+    run_cmd = test_step["run"]
+    assert " -n " not in f" {run_cmd} "
+    assert " -nauto" not in f" {run_cmd} "
+    assert "\n-n " not in run_cmd
+    assert not run_cmd.rstrip().endswith("-n"), (
         "CI must run the e2e suite serially until the rig is xdist-safe"
     )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert "-n " not in test_step["run"] and not test_step["run"].rstrip().endswith("-n"), (
"CI must run the e2e suite serially until the rig is xdist-safe"
)
run_cmd = test_step["run"]
assert " -n " not in f" {run_cmd} "
assert " -nauto" not in f" {run_cmd} "
assert "\n-n " not in run_cmd
assert not run_cmd.rstrip().endswith("-n"), (
"CI must run the e2e suite serially until the rig is xdist-safe"
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ingestion/tests/e2e/meta/test_ci_workflow.py` around lines 71 - 73, The
current assertion only checks for "-n " and trailing "-n" and misses attached
forms like "-nauto"; update the guard to split the command string (use
shlex.split on test_step["run"]) and assert that no token startswith("-n") to
reject any -n variant. Replace the existing assertion that references
test_step["run"] with a check using the tokenized command and ensure the failure
message remains descriptive (CI must run the e2e suite serially until the rig is
xdist-safe).



def test_compose_logs_dumped_on_failure(workflow: dict) -> None:
Expand Down
4 changes: 2 additions & 2 deletions src/ingestion/tests/e2e/meta/test_session_smoke.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ def test_migrations_create_insight_database(
def test_analytics_api_health(analytics_api: AnalyticsApiProcess) -> None:
"""analytics-api responds 200 on /health.

Requires `cargo ≥ 1.92` (edition2024). Older toolchains fail at build time
with `feature 'edition2024' is required` — run `rustup update stable`.
Requires a cargo/rustc satisfying `rust-version` in src/backend/Cargo.toml.
An older toolchain now FAILS (not skips) — run `rustup update stable`.
"""
with analytics_api.client() as c:
r = c.get("/health")
Expand Down
Loading
Loading