diff --git a/tests/STYLE.md b/tests/STYLE.md new file mode 100644 index 0000000..d6c7b39 --- /dev/null +++ b/tests/STYLE.md @@ -0,0 +1,116 @@ +# cc-ci test style guide + +Rules for writing and changing tests under `tests/`. Read this before any test edit — in particular +before a `/recipe-upgrade --with-tests` or `/ci-test-review` fix, where the temptation is to +make a red run green rather than to make the test right. + +The tests are the **independent gate** on recipe upgrades. Their value is entirely in being hard to +fool, so every rule below exists to keep them (a) honest and (b) alive across upgrades. + +--- + +## 1. Set up state through the application, not its database + +**Order of preference for any fixture that must create state:** + +1. **The app's public HTTP API.** +2. **The app's official CLI or release console** (`docker exec … `). +3. **Writing rows into its database — last resort only**, and only with a comment saying which of the + above were tried and why they did not work. + +Direct SQL couples the test to the app's *internal schema*, which upgrades are free to change. The +app's own interface is the thing it promises to keep working. + +> **Why this rule exists.** `tests/plausible/custom/test_event_tracking.py` used to register its test +> site with `INSERT INTO sites (...)`. That was sufficient for plausible v2. In v3 a site must belong +> to a **team**, and the app silently discards events for a teamless site — `POST /api/event` still +> returns **202** and the row is still in postgres, so the only visible symptom was that nothing ever +> reached ClickHouse. It read as a mysterious ingestion stall and held the recipe RED for six weeks. +> +> The fix was not to also INSERT a team row. It was to stop writing rows: the fixture now calls +> `Plausible.Sites.create/2` through the app's release console, and the app provisions whatever its +> data model currently needs. The same expression works unchanged on v2 (which has no `teams` table +> at all) **and** v3 — not because the test handles both, but because it stopped depending on the +> schema. + +When the ideal interface is unavailable, say so in the code. plausible's HTTP provisioning API +(`POST /api/v1/sites`) is gated behind a paid plan and answers `:upgrade_required` on CE, so the test +drops to option 2 and records that in a comment. + +## 2. Gate on version rather than writing dual-path fixtures + +If a behaviour genuinely only exists from version X, **gate the test on the version** instead of +branching inside it: + +```python +pytest.mark.skipif(app_version < (3,), reason="teams were introduced in v3") +``` + +Do **not** write a fixture that carefully supports both schemas. Version-portable code is harder to +read, harder to trust, and quietly rots once nobody runs the old path. + +Corollary: **old tests can simply be deleted** once the fleet has moved past that version. The older +version is only ever exercised through the *upgrade* tier (deploy base → upgrade → assert), so tests +that only make sense for a superseded version are dead weight, not coverage. + +Prefer §1 first: an app-level fixture often makes the version difference disappear, and then no gate +is needed at all. + +## 3. Never weaken an assertion to turn a run green + +There is a hard line between these two, and only the second is allowed as a way out of a red run: + +* **Weakening** — relaxing *what* is asserted: dropping a field check, accepting a wider status set, + asserting a 202 ack instead of the stored result, deleting the read-back. +* **Correcting the fixture or the wait** — fixing *how* the test sets up or how long it allows, with + the assertion untouched. + +If a test can only pass by asserting less, it has found a real regression. Report it; do not edit it. + +## 4. Assert real state, not acknowledgements + +An HTTP 202 means "accepted", not "done". Read the effect back out of the system that owns it — the +row in the analytics store, the file on disk, the record in the API — and assert on the values you +sent. plausible's ingestion returns 202 for events it goes on to discard entirely; a test that +stopped at the ack would have been permanently, silently green. + +## 5. Derive waits from the recipe's declared readiness, not a guess + +A per-recipe `recipe_meta.py` already declares `DEPLOY_TIMEOUT` / `HTTP_TIMEOUT` because someone +measured that app's boot profile. A custom test that hard-codes a shorter window contradicts it and +will flake or fail on a slower version. + +Remember the **tier order**: `custom` runs after `backup`/`restore`, which disrupts the datastore and +restarts the app. A window sized for a warm app is not sized for that. plausible's health check +allowed 60s; v3 boots through `sleep 10` → `createdb` → `migrate` → cache warmers first. + +## 6. Diagnose from the app's own telemetry before touching a test + +Before concluding a test is stale, find the app's account of what happened. It is usually definitive +and it stops you fixing the wrong thing. plausible records dropped events in ClickHouse's +`ingest_counters`: `dropped_not_found` with 0 rows before the fix, `buffered` with rows after — that +single counter identified the root cause after the HTTP status had suggested everything was fine. + +Prove the diagnosis both ways where you can: same input, broken state → symptom; corrected state → +no symptom. + +## 7. Fixtures must be idempotent + +A fixture may run against a warm canonical, a restored volume, or a re-run. Creating state must be +safe to repeat — look the object up first and reuse it, rather than assuming a clean database. + +## 8. Keep test identities obviously synthetic + +Use `ccci-`-prefixed names and `.example` / `.invalid` domains for anything a test creates, so state +it leaves behind is instantly attributable and can never be confused with real data. + +--- + +## Changing a test: the checklist + +1. Reproduce the failure and get the **app's own** explanation (§6). +2. Classify: recipe bug, or stale test? Only a stale test justifies a test edit. +3. Fix the **fixture, wait, or setup** — never the assertion (§3). +4. Prefer the app's interface over its database (§1); gate on version rather than branching (§2). +5. Verify green against the recipe PR head with the changed test, plus a regression sample. +6. Say in the commit and PR **what evidence** proves the diagnosis, not just what changed. diff --git a/tests/plausible/custom/test_event_tracking.py b/tests/plausible/custom/test_event_tracking.py index 38dda08..ade8373 100644 --- a/tests/plausible/custom/test_event_tracking.py +++ b/tests/plausible/custom/test_event_tracking.py @@ -14,10 +14,11 @@ Both assert real app state (the event reached the analytics store), not just the plausible only ingests events for *known* sites — the in-memory `sites_cache` gates ingestion and drops events for unregistered domains (empirically confirmed: an event for an unregistered domain -never appears in events_v2). From v3 (community-edition) a site is only "known" once it belongs to a -TEAM; a teamless site is dropped as `dropped_not_found` while the POST still acks 202, so the failure -looks like a silent ingestion stall. `_register_site` therefore provisions a team as well when the -schema has one. So each test first registers a site row in the metadata postgres, then +never appears in events_v2). Sites are therefore provisioned through plausible's OWN creation path +rather than by writing rows — under v3 a site must belong to a TEAM, and a teamless site is dropped as +`dropped_not_found` while the POST still acks 202, which reads as a silent ingestion stall. Letting the +app create the site sidesteps that entirely, and works unchanged on v2. So each test first provisions +the site, then POSTs repeatedly while polling ClickHouse: the sites_cache must refresh to admit the new site and the event write-buffer must flush to ClickHouse, so the first landing is not instantaneous. Re-POSTing the same event is safe — we assert the row count is >= 1. @@ -43,6 +44,10 @@ _UA = ( "(KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" ) +# Identity the harness provisions sites under. Ephemeral per-run deploy, never a real account. +_HARNESS_EMAIL = "cc-ci@ci.invalid" +_HARNESS_PW = "ccci-harness-passphrase-2026" + def _ch(domain: str, sql: str) -> str: """Run a ClickHouse query against the `plausible_events_db` service; return stdout (stripped).""" @@ -53,47 +58,59 @@ def _ch(domain: str, sql: str) -> str: ).strip() +# plausible's own provisioning path. Creating a site through the app (rather than INSERTing rows) +# means the app applies whatever its current data model requires — which is what makes this work +# unchanged across the v2→v3 jump, where sites gained a mandatory owning TEAM. Verified on cc-ci +# against BOTH v2.0.0 (no `teams` table at all) and v3.2.1: identical expression, site usable, events +# ingested. See tests/STYLE.md. +# +# The HTTP provisioning API (`POST /api/v1/sites`) would be the first choice, but it is gated behind +# a paid plan — on CE it answers `:upgrade_required` — so the app's release console is the closest +# public interface available here. +_PROVISION_SITE_EXS = """ +pw = "__PW__" +email = "__EMAIL__" +user = + case Plausible.Auth.find_user_by(email: email) do + nil -> + {:ok, u} = + Plausible.Auth.User.new(%{name: "cc-ci", email: email, password: pw, password_confirmation: pw}) + |> Plausible.Repo.insert() + u + u -> u + end +site = "__SITE__" +result = + case Plausible.Sites.get_by_domain(site) do + nil -> Plausible.Sites.create(user, %{"domain" => site, "timezone" => "UTC"}) + s -> {:ok, s} + end +case result do + {:ok, _} -> IO.puts("CCCI_SITE_OK " <> site) + other -> IO.puts("CCCI_SITE_ERR " <> inspect(other)) +end +""" + + def _register_site(domain: str, site: str) -> None: - """Register `site` in the metadata postgres so plausible will ingest events for it. + """Provision `site` via plausible's own site-creation path, so it is a site the app will ingest for. - Idempotent. Works against BOTH schema generations, because the upgrade tier deploys an older - base version before upgrading: + Idempotent: an existing domain is reused rather than re-created. - * v2 (`plausible/analytics`) — a row in `sites` is sufficient. - * v3 (`ghcr.io/plausible/community-edition`) — sites belong to a TEAM, and ingestion drops - events for a site whose team is missing. The POST still acks 202 and the row still exists in - postgres, so the only visible symptom is that nothing ever reaches ClickHouse; the reason is - recorded in ClickHouse's own `ingest_counters` as `dropped_not_found`. Verified on cc-ci - against v3.2.1: identical site row, no team → `dropped_not_found`; with a team linked → - `buffered` and the row appears in `events_v2`. - - The team block is guarded on the schema actually having teams, so this stays a no-op on v2 - rather than branching on a version string. + Do NOT reach into postgres to do this. A `sites` INSERT was enough under v2, but v3 requires the + site to belong to a TEAM and silently discards events for a teamless site — `POST /api/event` + still acks 202 and the row still exists, so the only symptom is that nothing reaches ClickHouse + (ClickHouse's own `ingest_counters` records it as `dropped_not_found`). That is what put this + recipe RED on build 1224. Going through the app removes the whole class of problem: it provisions + the team itself. """ - sql = ( - "INSERT INTO sites (domain, timezone, inserted_at, updated_at, native_stats_start_at) " - f"VALUES ('{site}','UTC', now(), now(), now()) ON CONFLICT (domain) DO NOTHING; " - "DO $ccci$ " - "BEGIN " - " IF EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'teams') " - " AND EXISTS (SELECT 1 FROM information_schema.columns " - " WHERE table_name = 'sites' AND column_name = 'team_id') THEN " - " INSERT INTO teams (name, inserted_at, updated_at, accept_traffic_until, setup_complete) " - " SELECT 'cc-ci', now(), now(), now() + interval '365 days', true " - " WHERE NOT EXISTS (SELECT 1 FROM teams WHERE name = 'cc-ci'); " - " UPDATE sites " - " SET team_id = COALESCE(team_id, (SELECT id FROM teams WHERE name = 'cc-ci' LIMIT 1)), " - " accept_traffic_until = COALESCE(accept_traffic_until, now() + interval '365 days') " - f" WHERE domain = '{site}'; " - " END IF; " - "END " - "$ccci$; " - f"SELECT domain FROM sites WHERE domain = '{site}';" + exs = ( + _PROVISION_SITE_EXS.replace("__PW__", _HARNESS_PW) + .replace("__EMAIL__", _HARNESS_EMAIL) + .replace("__SITE__", site) ) - out = lifecycle.exec_in_app( - domain, ["psql", "-q", "-U", "plausible", "-d", "plausible", "-tAc", sql], service="db" - ).strip() - assert out == site, f"site {site!r} not registered in postgres (got {out!r})" + out = lifecycle.exec_in_app(domain, ["/app/bin/plausible", "rpc", exs], service="app") + assert f"CCCI_SITE_OK {site}" in out, f"could not provision site {site!r} via the app: {out.strip()[-400:]}" def _post_event(base_domain: str, site: str, name: str, pathname: str) -> int: