cc-ci
eecc4aaa51
continuous-integration/drone/push Build is failing
The first cut of this fix added a team INSERT next to the existing sites INSERT. That fixed the symptom and kept the cause: a fixture that knows the apps table layout breaks whenever the app changes it, which is exactly what happened here. _register_site now calls Plausible.Sites.create/2 through the app release console, so the app provisions whatever its data model currently requires - including the team it introduced in v3. Verified against BOTH versions on cc-ci: the identical expression works on v2.0.0, which has no teams table at all, and on v3.2.1. No version gate is needed because the fixture no longer depends on the schema. The HTTP provisioning API (POST /api/v1/sites) would have been first choice, but it is gated behind a paid plan and answers :upgrade_required on CE. That is recorded in the code so the next person does not re-derive it. tests/STYLE.md writes the rule down, along with the others this failure exercised: gate on version rather than supporting both schemas (old-version tests can just be deleted - the older version is only exercised through the upgrade tier); correct the fixture or the wait but never the assertion; assert stored state rather than a 202 ack; size waits from the recipes declared readiness; and read the apps own telemetry before deciding a test is stale. Full cold suite against the recipe PR head: level 5 of 5, GREEN.
117 lines
6.1 KiB
Markdown
117 lines
6.1 KiB
Markdown
# 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 <recipe> --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 … <app-cli>`).
|
|
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.
|