From b026fe18725f0735dae0a758d129671ed23fdf55 Mon Sep 17 00:00:00 2001 From: Phil Leggetter Date: Tue, 15 Sep 2026 10:07:22 +0100 Subject: [PATCH] docs: record four testing practices the v2.6.0 release earned Each of these cost real time in v2.6.0 and none was written down anywhere. A fix is not done until it has been reverted and the test watched to fail. An audit of the release found eight fixes that could be deleted with a green suite, two tests failing for the wrong reason, and one rewritten to assert the regression it existed to catch. It is the only technique used on that release with no false negatives, and it costs a minute per fix. Prove a filter with a negative control. The API answers 200 and returns a full result set for a parameter it does not support, so a dropped filter is indistinguishable from a working one unless a deliberately bogus value is also tested. This was the single most repeated defect class in the release -- metrics filters, MCP arguments, comma-separated ids -- and one negative control catches all of them. Test each call site separately. The delivery-group data loss existed in four code paths; the third was found only after the first two were considered fixed. Redaction that matches only double-quoted values misses the config file, because the CLI writes single-quoted TOML. Two separate agents printed a key that way in one day, both believing they had redacted it. Also documents the acceptance suite's 240 req/min ceiling: two concurrent runs exhaust it and every job fails at its timeout with zero assertion failures, which reads as a code failure and is not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BnrKWZQASV7bFJ4oGWwmo9 --- AGENTS.md | 8 ++++++++ test/acceptance/README.md | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 8874d21a..0e322ccf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -409,6 +409,12 @@ if apiErr, ok := err.(*hookdeck.APIError); ok { - **Always run tests** when changing code. Run **`go test ./...`** from the repo root (see **§5 Running `go test` after code changes**). For CLI-facing changes, run acceptance tests with the correct **`-tags=...`** per `test/acceptance/README.md`. **Agents:** **prompt for out-of-sandbox / full permissions**, then run `go test` with **`required_permissions: ["all"]`** (and network if needed) so the module cache works—**do not** push the user to run tests manually unless elevation is refused. - **Create tests for new functionality.** Add unit tests for validation and business logic; add acceptance tests for flows that use the CLI as a user or agent would (success and failure paths). Acceptance tests must pass or fail—no skipping to avoid failures. +- **A fix is not done until you have reverted it and watched the test fail.** Apply the test, confirm it passes, then undo the fix and confirm the *right* test fails, then restore. A test written alongside a fix frequently passes for an unrelated reason: an audit of one release found eight fixes that could be deleted with the suite still green, two tests that failed for the wrong reason, and one that had been rewritten to assert the regression it was meant to catch. Revert one fix at a time, restore immediately, and never leave the tree non-compiling between steps. + +- **Prove a filter with a negative control. "No error" is not evidence it worked.** The API silently ignores query parameters it does not support, so a dropped filter returns HTTP 200 and a full result set that looks filtered. Assert both directions: a real value returns the expected subset, and a deliberately bogus value returns zero. If both return the same rows, the filter is being discarded. The same applies to any flag whose effect you cannot see in the output — read the resource back (`get --output json`) rather than trusting a success message. + +- **When a fix has several call sites, test each one separately.** A single passing test does not pin the others. One release fixed the same defect in four code paths; the third was found only after the first two were considered done. + ### Acceptance Test Setup Acceptance tests require a Hookdeck API key. See [`test/acceptance/README.md`](test/acceptance/README.md) for full details. Quick setup: create `test/acceptance/.env` with `HOOKDECK_CLI_TESTING_API_KEY=`. The `.env` file is git-ignored and must never be committed. @@ -579,6 +585,8 @@ Summary for code and docs work: - **Guest** — `listen` without login may call `POST /cli/guest`; separate from `--cli-key` onboarding. - **`project list`** — Requires a user-associated CLI client key (`hookdeck login` or `hookdeck login --cli-key`). CI keys from `hookdeck ci` and raw Project API keys cannot list or switch projects (acceptance: `HOOKDECK_CLI_TESTING_CLI_KEY`). +- **Redacting keys: the CLI writes single-quoted TOML.** A redaction pattern that only matches double-quoted values will print the key from `config.toml` verbatim. Match both quote styles, or avoid reading the file at all — compare by hash, and refer to variables by name. Never echo a key value, including into a log you expect only yourself to read. + --- ## Agent skills diff --git a/test/acceptance/README.md b/test/acceptance/README.md index 0720548c..d0e33f18 100644 --- a/test/acceptance/README.md +++ b/test/acceptance/README.md @@ -114,6 +114,14 @@ go test -short ./test/acceptance/... ``` Use the same `-tags` as "Run all" if you want to skip the full acceptance set. All acceptance tests are skipped when `-short` is used, allowing fast unit test runs. +## Rate limits + +The API allows **240 requests per minute**. A full slice makes thousands of calls, so the suite runs close to that ceiling by design. + +**Do not run two acceptance runs against the same projects at once.** Two concurrent runs exhaust the limit, every job hits its `-timeout` with HTTP 429s in the log, and the result looks exactly like a code failure: jobs fail with **zero assertion failures**. If you see all slices failing at the timeout and no `--- FAIL` lines, check for a second run before looking at the code. + +Note a direct push to a branch triggers a `pull_request` run for any PR it heads, so pushing and dispatching a manual run together produces exactly this collision. + ## Parallelisation Tests are partitioned by **feature build tags** so CI and local runs can execute three matrix slices in parallel (each slice uses its own Hookdeck project and config file).