art/ismap
1
0
Fork 0
Code Issues 56 Pull requests 1 Projects Releases Packages Wiki Activity Actions

#121: Iceland's remaining R-checks as acceptance tests (R34, R49, R105, R119, R120) #132

Merged
art merged 2 commits from issue-121-is-acceptance-tests into main 2026-08-15 19:16:56 +00:00
Conversation 1 Commits 2 Files changed 2 +241 -8
art commented 2026-08-15 17:30:31 +00:00
Owner
Copy link

Closes #121

#121 — Iceland: acceptance tests for the remaining R-checks

Config-only change (conf/ismap.yaml): the manual R-checks still listed as
candidates in #115's plan are now acceptance tests, run on every import of the
layer and in -acceptance-only review runs. No import was needed — this is
config, and every test reads data already live in the DB.

What was added (13 tests, all 10 s bucket; measured 0–0.3 s live)

R where check
R34 cover maxz land area (EPSG:3057, sea excluded) within 1 % of LMÍ's national-territory polygons (mork)
R49 waterways z14 importance histogram is a monotone Horton ladder
R105 config-level every Iceland staging table is nonzero after load
R119 contours, peaks, buildings, airports, airport_points no zoom table in the layer's range is empty
R120 the same five layers no NULLs in the columns the cascade reads

Notes for the reviewer:

  • R34's reference. The issue's wording is "the authoritative national area".
    I used LMÍ's own territory polygons (the mork staging table — already in the
    DB, feeding country_borders and country_mask) rather than a hardcoded
    figure, which is the same two-products cross-check docs/country-finland.md
    documents for Finland (MML outline vs SYKE CORINE). Live: cover 103 497 km² vs
    mork 102 714 km² = +0.76 %; the official 103 000 km² sits between the two. The
    sea polygon is the computed box-minus-coastline mask, not national territory,
    so the query excludes it — the comment says so.
  • R49 only ported half the cluster. Zero-NULL importance is already the shared
    contract's R46 test, which runs for every country. R50/R51 are properties of
    the polygon-adjacency graph the importer builds, not of anything the output
    tables witness, so they cannot be an acceptance test — the monotone shape is
    the portable part, and its comment says exactly that. The CASE's first arm
    guards the empty-partition case (an Iceland partition with no rows would
    otherwise pass the shape check vacuously).
  • R119 with zoom_filters. These five layers do have zoom_filters, so R31's
    "deliberate empty" case applies in principle; per layer the comment records
    that none of its filters empties a zoom. count(*) = N turns a missing zoom
    table into a failure rather than a vacuous pass over the tables that exist;
    requires: guards the whole layer on its maxz table so a half-built DB skips.
  • airport_points is in. The issue's list names contours, peaks, buildings,
    airports ("R13's Iceland-only set"); airport_points is the airports label layer
    of that same set, with its own tables, so it gets the same two tests.
  • R139 is deliberately not here. #120's "0 invalid geometries at maxz" test
    ships with #120 itself (PR #131, still open; its fix is already live in the DB
    and the issue's comment records the dependency). Do not re-add it — this PR
    only references it.
  • Merge interaction with #131. Both PRs add an acceptance_tests: block to
    the cover layer (R139 there, R34 here) — whoever merges second needs a
    two-line rebase to fold the blocks together. Nothing else overlaps.

Verification (live DB, no writes)

$ make test
ok  github.com/art4711/ismap/importer
ok  github.com/art4711/ismap/mapsrv

$ ./importer/importer -acceptance-only conf/ismap.yaml
acceptance: 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run

$ make acceptance
conf/ismap.yaml   acceptance: 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run
conf/semap.yaml   acceptance: 13 test(s): 12 passed, 0 failed, 0 skipped, 1 not run (needs -acceptance all)
conf/plmap.yaml   acceptance: 13 test(s): 13 passed, 0 failed, 0 skipped, 0 not run
conf/fimap.yaml   acceptance: 12 test(s): 12 passed, 0 failed, 0 skipped, 0 not run
conf/world.yaml   acceptance: 9 test(s): 9 passed, 0 failed, 0 skipped, 0 not run

(semap's single not-run is the pre-existing R35 at 100 s — needs
-acceptance all, unrelated to this change.)

New-test highlights from the run:

R34:  PASS cover_km2=103497, territory_km2=102714, pct=0.76
R49:  PASS histogram=1:50656 > 2:17319 > 3:9056 > 4:3406 > 5:1476
R105: PASS tables=5, empty_tables=<nil>
R119: PASS (contours tables=5, peaks tables=4, buildings tables=3,
            airports tables=4, airport_points tables=6; all empty_tables=<nil>)
R120: PASS (all NULL counts 0)

Nothing left undone that I know of: the R46–R51 cluster's graph-internal halves
(R50/R51) and #122's area-collapse-per-zoom measurement are the documented
remainders, both already filed elsewhere.

Closes #121 --- #121 — Iceland: acceptance tests for the remaining R-checks Config-only change (`conf/ismap.yaml`): the manual R-checks still listed as candidates in #115's plan are now acceptance tests, run on every import of the layer and in `-acceptance-only` review runs. No import was needed — this is config, and every test reads data already live in the DB. ## What was added (13 tests, all 10 s bucket; measured 0–0.3 s live) | R | where | check | |---|---|---| | R34 | `cover` | maxz land area (EPSG:3057, sea excluded) within 1 % of LMÍ's national-territory polygons (mork) | | R49 | `waterways` | z14 importance histogram is a monotone Horton ladder | | R105 | config-level | every Iceland staging table is nonzero after load | | R119 | contours, peaks, buildings, airports, airport_points | no zoom table in the layer's range is empty | | R120 | the same five layers | no NULLs in the columns the cascade reads | Notes for the reviewer: - **R34's reference.** The issue's wording is "the authoritative national area". I used LMÍ's own territory polygons (the `mork` staging table — already in the DB, feeding `country_borders` and `country_mask`) rather than a hardcoded figure, which is the same two-products cross-check `docs/country-finland.md` documents for Finland (MML outline vs SYKE CORINE). Live: cover 103 497 km² vs mork 102 714 km² = +0.76 %; the official 103 000 km² sits between the two. The sea polygon is the computed box-minus-coastline mask, not national territory, so the query excludes it — the comment says so. - **R49 only ported half the cluster.** Zero-NULL importance is already the shared contract's R46 test, which runs for every country. R50/R51 are properties of the polygon-adjacency graph the *importer* builds, not of anything the output tables witness, so they cannot be an acceptance test — the monotone shape is the portable part, and its comment says exactly that. The CASE's first arm guards the empty-partition case (an Iceland partition with no rows would otherwise pass the shape check vacuously). - **R119 with zoom_filters.** These five layers *do* have zoom_filters, so R31's "deliberate empty" case applies in principle; per layer the comment records that none of its filters empties a zoom. `count(*) = N` turns a *missing* zoom table into a failure rather than a vacuous pass over the tables that exist; `requires:` guards the whole layer on its maxz table so a half-built DB skips. - **airport_points is in.** The issue's list names contours, peaks, buildings, airports ("R13's Iceland-only set"); airport_points is the airports label layer of that same set, with its own tables, so it gets the same two tests. - **R139 is deliberately not here.** #120's "0 invalid geometries at maxz" test ships with #120 itself (PR #131, still open; its fix is already live in the DB and the issue's comment records the dependency). Do not re-add it — this PR only references it. - **Merge interaction with #131.** Both PRs add an `acceptance_tests:` block to the `cover` layer (R139 there, R34 here) — whoever merges second needs a two-line rebase to fold the blocks together. Nothing else overlaps. ## Verification (live DB, no writes) ``` $ make test ok github.com/art4711/ismap/importer ok github.com/art4711/ismap/mapsrv $ ./importer/importer -acceptance-only conf/ismap.yaml acceptance: 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run $ make acceptance conf/ismap.yaml acceptance: 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run conf/semap.yaml acceptance: 13 test(s): 12 passed, 0 failed, 0 skipped, 1 not run (needs -acceptance all) conf/plmap.yaml acceptance: 13 test(s): 13 passed, 0 failed, 0 skipped, 0 not run conf/fimap.yaml acceptance: 12 test(s): 12 passed, 0 failed, 0 skipped, 0 not run conf/world.yaml acceptance: 9 test(s): 9 passed, 0 failed, 0 skipped, 0 not run ``` (semap's single not-run is the pre-existing R35 at 100 s — needs `-acceptance all`, unrelated to this change.) New-test highlights from the run: ``` R34: PASS cover_km2=103497, territory_km2=102714, pct=0.76 R49: PASS histogram=1:50656 > 2:17319 > 3:9056 > 4:3406 > 5:1476 R105: PASS tables=5, empty_tables=<nil> R119: PASS (contours tables=5, peaks tables=4, buildings tables=3, airports tables=4, airport_points tables=6; all empty_tables=<nil>) R120: PASS (all NULL counts 0) ``` Nothing left undone that I know of: the R46–R51 cluster's graph-internal halves (R50/R51) and #122's area-collapse-per-zoom measurement are the documented remainders, both already filed elsewhere. 
Part of the #115 plan: conf/ismap.yaml now carries acceptance tests for the
remaining manual R-checks the #119 first set deliberately left out. Config-only;
no import needed, the live DB already holds the data every test reads.

Thirteen new tests, all in the 10 s bucket (measured 0–0.3 s live):

- R34 (cover): maxz cover land area in EPSG:3057 vs LMÍ's own national-territory
  polygons (the mork staging table) — 103 497 km² vs 102 714 km² = +0.76 %, inside
  the ~1 % R34 allows. Same two-products cross-check Finland documents; the
  official 103 000 km² sits between the two.
- R49 (waterways): the z14 importance histogram is a monotone Horton ladder
  (1:50656 > 2:17319 > 3:9056 > 4:3406 > 5:1476). The NULL half of R46 stays the
  shared contract's test; R50/R51 are properties of the importer's internal
  adjacency graph, not of the output tables, so the shape check is the portable
  half.
- R105 (config-level): every Iceland staging table is nonzero after load —
  catches the polygon-only shapefile reader silently skipping rows.
- R119 (contours, peaks, buildings, airports, airport_points): no zoom table in a
  layer's declared range is empty (count(*) = N AND bool_and(pg_relation_size > 0)
  so a *missing* zoom table fails rather than passing vacuously). R31's deliberate-
  empty case is noted per layer: none of these filters empties a zoom.
- R120 (same five layers): no NULLs in the columns the cascade reads — elevation /
  interval_class (contours), elevation (peaks), building_type / name (buildings),
  name / airport_class (airports and airport_points).

airport_points is in because the issue's "R13's Iceland-only set" includes it
(the airports label layer), and its tables are as guardable as the other four.

Not duplicated here: #120's R139 ("0 invalid geometries at maxz") ships with #120
itself (PR #131) — that issue's comment records the dependency.

Verification (live DB, no writes):
  make test                 # config + Go tests, all green
  importer/importer -acceptance-only conf/ismap.yaml
    acceptance: 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run
  make acceptance           # all five configs
    ismap:  24 passed / semap: 12 passed + R35 not run (100 s, needs -acceptance all)
    plmap:  13 passed / fimap: 12 passed / world: 9 passed
art-bot force-pushed issue-121-is-acceptance-tests from 18345a1e75 to 49e6a4cd7b 2026-08-15 18:46:02 +00:00 Compare
art-bot commented 2026-08-15 18:51:01 +00:00
Collaborator
Copy link

Review pass (art-bot). Two things changed about how this lands, not just what it contains.

R105 was disabled, not strict. It listed is_lcv_lmi_landcoverunit in requires:, and that table is added by #120's staging: block, not this one. requires: skips a test whose tables are missing, so against a database built from this branch's config alone the check would not have run — not for the landcover table, and not for the four that are there either. It passed in review only because the live DB still held #120's import.

This branch is now stacked on issue-120-is-cover-validity. That makes the list true and resolves the collision the two branches had — both inserted an acceptance_tests: key into the cover layer at the same line, so whichever merged second would have conflicted. The fold is done here: R139, R120 and R34 under one key. Merge #131 first; once it is on main this branch's diff is its own 208 lines again.

Also added, per docs/country-requirements.md's own rule that a check automated in a commit is recorded there in the same commit: *Check:* lines for R34, R49 and R105. R49's now also says why R50/R51 cannot have an acceptance test, which was in the config comment but not in the document that sends people looking.

Live: 26 acceptance tests, 26 passed.

Review pass (art-bot). Two things changed about how this lands, not just what it contains. **R105 was disabled, not strict.** It listed `is_lcv_lmi_landcoverunit` in `requires:`, and that table is added by #120's `staging:` block, not this one. `requires:` *skips* a test whose tables are missing, so against a database built from this branch's config alone the check would not have run — not for the landcover table, and not for the four that are there either. It passed in review only because the live DB still held #120's import. **This branch is now stacked on `issue-120-is-cover-validity`.** That makes the list true and resolves the collision the two branches had — both inserted an `acceptance_tests:` key into the `cover` layer at the same line, so whichever merged second would have conflicted. The fold is done here: R139, R120 and R34 under one key. **Merge #131 first**; once it is on main this branch's diff is its own 208 lines again. Also added, per docs/country-requirements.md's own rule that a check automated in a commit is recorded there in the same commit: `*Check:*` lines for R34, R49 and R105. R49's now also says why R50/R51 cannot have an acceptance test, which was in the config comment but not in the document that sends people looking. Live: 26 acceptance tests, 26 passed.
art merged commit 49e6a4cd7b into main 2026-08-15 19:16:56 +00:00
art deleted branch issue-121-is-acceptance-tests 2026-08-15 19:16:56 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
issue is stuck: missing decision, credential, or data (work-issue blocked path)

  
claude-wip
night-shift

  
map-report
map data feedback (mapsrv)

  
map:is
map data feedback (mapsrv)

  
map:se
map data feedback (mapsrv)

  
needs-review
night-shift

No labels
blocked
claude-wip
map-report
map:is
map:se
needs-review
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
art/ismap!132
No description provided.