#115: Formalized acceptance tests. #119
No reviewers
Labels
No labels
blocked
claude-wip
map-report
map:is
map:se
needs-review
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
art/ismap!119
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-115-acceptance-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#115: Formalized acceptance tests.
Closes #115.
Review round 2 (2026-08-15)
Two commits on top of the original submission: the mechanism's partial-database
contract is now enforced by the machinery rather than by SQL style, and the rule that
future checks go through this mechanism is written where the work actually gets
directed.
The finding that mattered
The claim that a test skips rather than fails on a partially-populated database held
only for tests that named a partition table directly. Measured against the live DB,
the four shapes in the tree behaved three different ways for the same situation:
map_cover_13_se(names a partition)string_agg(…) = '…'count(*) FILTER (…) > 0falsepg_inheritscatalogue query-acceptance-only conf/semap.yamlagainst a database holding only Iceland would havereported failures for Sweden's station ladder and Poland's ferry partition. With
#121–#125 about to add forty more tests, whichever shape gets copied would decide the
behaviour.
requires:is the fix: a test declares the tables it needs, with<cc>expandingto the owning layer's country code, so one line in a country config means that
country's own partition. Absent table → skipped, and named in the log. Startup
validation rejects a
requires:that is not a table name, or a<cc>on a test withno country to substitute. This also replaces the Tornio test's hand-written
CASE WHEN NOT EXISTS (…) THEN NULLguard withrequires: [map_roads_14_se, map_roads_14_fi].The catalogue queries can't use it — the parent tables exist from the importer's first
run, and it's the children that vary — so they aggregate instead:
bool_andover anempty set is NULL, which is already "not applicable". They now name the offending
partitions through a
FILTERcolumn, which the per-row form gave for free.The rest of the review
13 test(s): 13 passed, 0 failed, 0 skipped, 0 not run). An all-skipped run and an all-passed run both exited 0 and looked identical.In
-acceptance-only, zero passes is now itself a non-zero exit — that mode existsto produce a verdict, and "everything skipped" is not one.
make acceptance/make acceptance-all. AGENTS.md says everything runs throughthe Makefile; this was the one thing that didn't.
make testnow runs the importer'stests too — it had been mapsrv-only since before this branch.
had
geomas a key column, so a btree on a geometry column would have passed.acceptanceVerdict) with the pgxrow-draining split out, so all ten branches have unit tests — including that a
failure reports the first failing row, not the first row.
formatAcceptanceDetailnolonger sizes a slice with
len(values)-1; gofmt.Making the mechanism the default path
The rule existed only in
docs/importer-cli.md— a flag reference nobody reads beforeverifying — while everything that directs the work still described the hand-rolled path:
AGENTS.md's first convention,
work-issue's step 5 (the file a subagent executes),AGENTS.md's Tornio paragraph, and
docs/country-requirements.md's 72*Check:*lines,one of which was literally the query R119's test replaces.
doing twice is an acceptance test, not a query you retype. Explore in psql freely; the
check that decided the work is correct gets written down. A PR that verifies a layer by
pasting SQL into its body, when that SQL could have been a test, isn't finished — and a
check that genuinely can't be expressed as one is a gap to file, not a licence to go
back to psql.
work-issuestep 5 gets the operational form: turn the checks that mattered intotests in the same commit, run
make acceptance, paste the tally into the note insteadof ad-hoc SQL output.
work-issuesexpects to see that tally when verifying aworker's self-report.
docs/country-requirements.mdgains a preamble saying the*Check:*lines aremigrating into the importer and that a line still written as bare SQL is a candidate,
not a decision to keep it manual. Eleven requirements now name their test (R6, R14,
R22, R35, R41, R46, R75, R83, R85, R119, R123–R126); where only half is covered, the
line says which half and what the other half needs.
docs/importer-cli.mdrewritten around the mechanism rather than the flags.README documents
make acceptance(code vs. data). architecture.md gainsacceptance.go. country-finland.md notes the #17 criterion is now literally a test.Verification
make build,make test,gofmt,go vetclean.R35 carve scan 18.2 s, Stockholm z5 tile 1 841 744 bytes).
make acceptancecorrectlydefers the one 100s test and finishes in ~7 s.
requires:on a missing partition skips, acatalogue query over a nonexistent layer skips, a nothing-verified review run exits 1
with the reason, a 1s test against
pg_sleep(30)fails at 1.001 s and leaves nobackend behind (checked
pg_stat_activitywith 26 s of sleep left), a mistyped columnstill fails rather than skipping, and
<cc>on a config-level test is rejected atstartup.
-layer country_mask conf/ismap.yamlimport ran its acceptance test after theswap and printed its tally.
Original submission
What this is
The importer's first built-in acceptance-test machinery: layers and shared-layer
contracts can declare
acceptance_tests:in config — a sentence describing what isbeing tested (citing the R-number from docs/country-requirements.md where it
restates one), a SQL query whose first column is the pass/fail boolean, and a
timeout from the issue's fixed ladder {1, 10, 100, 1000} seconds. The 1s and 10s
tests run on every import of the layer (and after each -regeneralize window); the
100s and 1000s ones run with
-acceptance all.-acceptance-onlyruns theconfigured tests against the live database and exits — no fetching, no staging, no
writes — which is the review path: a review (or a scheduled re-check) now runs the
importer's own tests instead of hand-rolled SQL. A third home, the config-level
key, carries "global" tests not tied to any layer (the review comment's ask): the
shared index-coverage and MVT-size checks live there, once, in conf/common.yaml,
and run on every import and in every review pass.
Where the tests live
conf/common.yaml— on the shared-layer contracts (shared_layers:entries),so a cross-country invariant is declared once. The contract's tests query the
shared parent tables and run on any country's import of that layer.
conf/<cc>map.yaml— on a country's own layers, querying its own partitions.The first tests encode the checks the recent reviews actually hand-rolled:
covercovercountry_borderswaterwaysroadsplacesplacesmap_*table's geom column has a valid gist indexcountry_maskplacescovercoverplacesstationswater_pointsferriesstationsplacesstationsplacesDesign decisions (the issue's open questions)
acceptance_tests:), plus a config-level keyfor tests not tied to any layer (the review comment's "global tests"): a test
is about the thing it names, and the key's home (shared contract, country
layer, or config top level) says which tables it may touch. Shared and
config-level tests live in conf/common.yaml and are inherited by every country
config through the existing
include:merge — no new inheritance machinery.10s is where the meaningful per-import checks live (a partition listing, a
mid-size validity scan), and ten seconds per layer is cheap next to the import
itself. 100s/1000s need
-acceptance all.layer's atomic swap, so the data is live either way; a failing test fails the
whole run at the end with a summary naming every failed test. A partial DB never
fails on a test for data that is not there: "relation does not exist" (42P01)
and an all-NULL first column are logged as SKIPs. The all-NULL form is how the
Tornio test says "the other country has not imported yet".
the server when it fires, so a runaway test cannot pin a worker.
iteration loop for generalization parameters, and the tests are the loop's
verification.
Verification
go test ./...in importer/ and mapsrv/;make build.names, uniqueness), the fast/all timeout policy, the failure summary format.
TestRealConfigsValidatenow validates the acceptance_tests blocks of everyreal config.
(
-acceptance-only -acceptance allfor all five configs): 47 test runs, 0failures. Sweden's R35 carve scan measured 18.5 s (comfortably inside its 100s
bucket).
(
-layer country_mask conf/ismap.yaml) ran its acceptance test after the swap.configs; failure exits non-zero with the summary.
map_cover_13_iscontains 1 invalid geometry (an
estuarywith a ring self-intersection near22.4°W/64.6°N). It is filed as #120 and is exactly the kind
of thing the -acceptance-only review pass now catches on every run.
Not done
crossing, not just Tornio) is not yet a test; the Tornio form is the issue #17
criterion and stays the cheap canary. The pairing query is a 1000s-bucket
candidate when someone wants it.
chopped into follow-up issues rather than grown here: #121 (Iceland), #122
(Sweden), #123 (Poland), #124 (Finland) and #125 (global/MVT) carry the
per-country and config-level candidates already listed in
docs/country-requirements.md. The machinery is in place and the pattern is
established; adding tests is per-issue work now, which is the point.
Layers and shared-layer contracts can now declare `acceptance_tests:` in config: a name (a sentence describing what is tested, citing the R-number from docs/country-requirements.md where it restates one), a SQL query whose first column is the pass/fail boolean, and a timeout from the issue's fixed ladder {1, 10, 100, 1000} seconds. The 1s and 10s tests run after every import of the layer (and after each -regeneralize window); 100s and 1000s tests need `-acceptance all`. `-acceptance-only` runs the configured tests against the live database and exits — no fetching, no import, no writes — which is the review path the issue asks for: a review runs the importer's own tests instead of hand-rolled SQL. Where tests live: the shared contracts in conf/common.yaml (cross-country invariants on the shared parent tables, run on any country's import of the layer) and the per-country configs (a country's own partitions). The first set encodes the checks the recent reviews actually hand-rolled: Sweden's R35 carve validity (100s bucket, ~20s live) and z3 wetland, all four countries' R85 top-five place ranks, the R75 station ladders, Poland's R6 ferry/water-point partitions, and the shared contracts' R22/R83/R119/R126 checks (including the Tornio-Haparanda border test from issue #17). Decisions on the issue's open questions: 10s tests run by default (the issue leaves "1 (and maybe 10)" open; 10s is where the meaningful per-import checks live); a failing test does not roll back the layer (tests run after the atomic swap) but fails the whole run at the end with a summary naming every failure; a partial DB cannot fail on missing data — "relation does not exist" and an all-NULL verdict are logged as skips, the NULL form being how the Tornio test says "the other country has not imported yet". Timeout enforcement is a per-test context deadline; pgx cancels the query on the server. Verified: go test in importer/ and mapsrv/, make build; new unit tests for parse, validation, the fast/all policy and the failure summary; TestRealConfigsValidate now validates every config's acceptance_tests blocks; all five configs' full suites run live against the maps DB (-acceptance-only -acceptance all: 47 test runs, 0 failures); the import hook verified with a real single-layer import (-layer country_mask conf/ismap.yaml ran its test after the swap); failure, skip and not-applicable paths exercised with throwaway configs. Writing the tests surfaced one genuine finding: Iceland's map_cover_13_is contains 1 invalid geometry (an estuary with a ring self-intersection) — filed separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com># #115: Formalized acceptance tests.to #115: Formalized acceptance tests.art referenced this pull request2026-08-15 09:28:25 +00:00
Review of the two review commits (
708e601machinery,8293e3cdocs) — verified against the live DB, not just read.What holds up, verified:
count(*) FILTER > 0→ false/fail, catalogue → 0 rows/fail). The commit's table is accurate.requires:with<cc>fixes it structurally — declared, validated at startup (regex +<cc>-only-where-there-is-a-country), parameterisedto_regclass(no interpolation). Reproduced the skip live.bool_andover empty = NULL → skip) works, and thestring_agg ... FILTERcolumns do name the offenders on failure.-acceptance-onlyzero-passes-exits-1: verified (a no-tests config exits 1 with the reason).make acceptance/make acceptance-all/make testall work; the tally numbers are exactly as claimed (57 runs, 0 failures, R35 18.5 s, z5 tile 1 841 744 B).One defect found and fixed (
45f3c9a): the skip rule inverted the commit's own "typo fails, missing object skips" rule for functions. Every 42883 skipped, so a test callingbool_and(integer)(or misspelling any function) was silently skipped on every import — demonstrated live before the fix. 42883 now skips only when the message names azxy_*function (the only 42883 a partial DB produces:zxy_mapbefore world.yaml ran); a function typo fails like a column typo. Unit test added.Three nits also fixed in
45f3c9a: the tally printed "(needs -acceptance all)" even when notRun = 0 (an-acceptance allrun read as if it left tests behind); the gist test's detail showedtables=<nil>on a clean run; the "nothing was verified" message didn't name the fast-mode-on-slow-tests cause.Two nits left, not worth commits: the commit message says "Eleven requirements now name their test" — it is fourteen (R6, R14, R22, R35, R41, R46, R75, R83, R85, R119, R123–R126); and in
make acceptancethe loop stops at the first failing config, so later configs' tallies are not printed — worth knowing when reading a failing run, not worth changing.Verdict: the review is sound, the machinery is better for it, and the branch is green — 57 test runs, 0 failures,
make testandmake buildclean. Ready for the maintainer.