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

#123: Poland's remaining R-checks as acceptance tests (R105, R119) #136

Merged
art merged 1 commit from issue-123-pl-acceptance-tests into main 2026-08-16 05:18:21 +00:00
Conversation 2 Commits 1 Files changed 2 +247 -6
art commented 2026-08-16 00:48:39 +00:00
Owner
Copy link

Closes #123

#123 — Poland: acceptance tests for the remaining R-checks

Part of the #115 plan: the manual R-checks still listed as candidates for Poland in
docs/country-requirements.md are now acceptance tests in conf/plmap.yaml, run on
every import of the layer and in -acceptance-only review runs. Config + doc only; no
import was needed — every test reads data already live in the DB.

What was added (11 tests, all 10 s bucket; measured 0–1 ms live)

R where check
R105 config-level every Poland staging table is nonzero after load (all 24, matching the staging: block exactly)
R119 country_mask, water_bodies, waterways, water_points, roads, ferries, railways, transit, stations, places no zoom table in the layer's declared range is empty

Design decisions worth the reviewer's eye

  • Two deliberate empties found and recorded, not fixed. Poland's R119 pass found
    map_water_bodies_3_pl and map_ferries_3_pl…map_ferries_6_pl empty. Both are the
    documented-knob case (R31's family), so each test asserts the populated range and names
    the empties in its comment:
    • water_bodies z3: the 4-px² area floor at z3's pixel scale drops every lake piece —
      the same property as every other country (fi 0, is 0 rows at z3). Poland's lone z3
      row is the #43 mislabel, which is exactly the bug the R58 fix removes; the test
      asserts z4–z14 so it survives that fix unchanged.
    • ferries z3–z6: min_length_pixels: 1 at those zooms is 19.6–2.4 km of ground per
      pixel, and BDOT10k SKPP carries crossings, not Baltic routes — the longest merged
      Polish ferry chain is ~1 km, so nothing survives until z7 (1.2 km/px, 1 row) and the
      133 routes only fully appear at z14.
  • The layers with zoom_filters all checked: none of waterways' importance cuts,
    roads' road_class cuts, railways' rail_class cuts, stations' importance cuts or
    places' rank caps empties a zoom; each test's comment records the floor count that
    proves it.

Recorded, not shipped — the checks that fail against the live data

Three of the issue's candidates are not in this PR because the live DB does not
satisfy them yet. Each is written down with its measurement and its exact fix, so the
fix PR can land it as a fail→pass pair (the #120 pattern; #121 did the same for R139).

  • R58 (issue #43, the standing data bug). The candidate test is "R58: no lake-class
    polygon is an order-of-magnitude area outlier that sits on the country mask's
    boundary", on water_bodies. Measured live it FAILS — correctly: map_water_bodies_14_pl
    still carries the 5 845 km² (3857) Morze Bałtyckie slab (29× the next-largest lake,
    Śniardwy at 200 km²), sharing 49 km of edge with the national boundary. It survives to
    z3 (it is the lone row there). The predicate: water_class = 'lake' AND area > 10× the second-largest lake AND ST_Intersects(ST_Boundary(poland_mask), geom) — the R58 check
    verbatim (order-of-magnitude outlier + seaward-edge cross-check). The fix is #43's
    option 2 ("exclude by geometry"); I commented #43 with the measurement and this
    predicate, so the fix PR is a ~5-line query change plus a cheap water_bodies reimport.
  • R63 (the delimited multi-value ref). The candidate test is the requirement's own
    check, count(*) = 0 FROM map_roads_14_pl WHERE ref ~ '[;,]'. Measured live it FAILS:
    114 rows still carry comma-delimited refs (75, 28 × 82, 964,768, 222, 211207G, 211208G, …). The #19 split handles the ; form (E77;S7;62 — issue #39) but not the
    comma form BDOT10k also delivers; the fix is extending
    regexp_replace(numerdrogi, ';', '|', 'g') to '[;,]' plus a roads reimport. Filed as
    a new issue (the fix + test land together).
  • R43 is genuinely not an acceptance test. The CASE/WHERE IN sync is a static
    property of the config text; no SQL can witness it without duplicating the very list
    the check is meant to keep singular (that duplicate would be the drift hazard). It is a
    query-comment review surface — recorded as such in the doc. The #123 pass did the
    review: every Polish CASE is total (single-kind BDOT10k files, R43's own allowance),
    and the one shared-column WHERE IN/CASE pair (water_points' rodzaj IN ('wodospad', 'źródło')) matches — źródło is the ELSE's referent, the least-specific class still
    correct for the file. Same shape as #121's R50/R51 and #122's R92 records.

Verification (live DB, no writes)

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

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

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

New-test highlights from the plmap run:

R105:   PASS tables=24, empty_tables=<nil>
R119:   PASS (country_mask 1, water_bodies z4–z14 11, waterways 12, water_points 7,
             roads 10, ferries z7–z14 8, railways 10, transit 6, stations 6, places 11;
             all empty_tables=<nil>)

Merge interaction

#122's PR (#134) is open and touches the same R119 paragraph in
docs/country-requirements.md; whoever merges second needs a small doc rebase to fold
the Sweden and Poland paragraphs together. Nothing else overlaps.

Nothing left undone that I know of: R58's and R63's tests ship with their fix PRs (both
specified above and tracked — #43 commented, new issue filed for the comma split), and
R43 is recorded as a review surface.

Closes #123 --- #123 — Poland: acceptance tests for the remaining R-checks Part of the #115 plan: the manual R-checks still listed as candidates for Poland in `docs/country-requirements.md` are now acceptance tests in `conf/plmap.yaml`, run on every import of the layer and in `-acceptance-only` review runs. Config + doc only; no import was needed — every test reads data already live in the DB. ## What was added (11 tests, all 10 s bucket; measured 0–1 ms live) | R | where | check | |---|---|---| | R105 | config-level | every Poland staging table is nonzero after load (all 24, matching the `staging:` block exactly) | | R119 | country_mask, water_bodies, waterways, water_points, roads, ferries, railways, transit, stations, places | no zoom table in the layer's declared range is empty | ## Design decisions worth the reviewer's eye - **Two deliberate empties found and recorded, not fixed.** Poland's R119 pass found `map_water_bodies_3_pl` and `map_ferries_3_pl`…`map_ferries_6_pl` empty. Both are the documented-knob case (R31's family), so each test asserts the populated range and names the empties in its comment: - water_bodies z3: the 4-px² area floor at z3's pixel scale drops every lake piece — the same property as every other country (fi 0, is 0 rows at z3). Poland's lone z3 row is the #43 mislabel, which is exactly the bug the R58 fix removes; the test asserts z4–z14 so it survives that fix unchanged. - ferries z3–z6: `min_length_pixels: 1` at those zooms is 19.6–2.4 km of ground per pixel, and BDOT10k SKPP carries crossings, not Baltic routes — the longest merged Polish ferry chain is ~1 km, so nothing survives until z7 (1.2 km/px, 1 row) and the 133 routes only fully appear at z14. - **The layers with zoom_filters all checked**: none of waterways' importance cuts, roads' road_class cuts, railways' rail_class cuts, stations' importance cuts or places' rank caps empties a zoom; each test's comment records the floor count that proves it. ## Recorded, not shipped — the checks that fail against the live data Three of the issue's candidates are **not in this PR** because the live DB does not satisfy them yet. Each is written down with its measurement and its exact fix, so the fix PR can land it as a fail→pass pair (the #120 pattern; #121 did the same for R139). - **R58 (issue #43, the standing data bug).** The candidate test is "R58: no lake-class polygon is an order-of-magnitude area outlier that sits on the country mask's boundary", on `water_bodies`. Measured live it FAILS — correctly: `map_water_bodies_14_pl` still carries the 5 845 km² (3857) `Morze Bałtyckie` slab (29× the next-largest lake, Śniardwy at 200 km²), sharing 49 km of edge with the national boundary. It survives to z3 (it is the lone row there). The predicate: `water_class = 'lake' AND area > 10× the second-largest lake AND ST_Intersects(ST_Boundary(poland_mask), geom)` — the R58 check verbatim (order-of-magnitude outlier + seaward-edge cross-check). The fix is #43's option 2 ("exclude by geometry"); I commented #43 with the measurement and this predicate, so the fix PR is a ~5-line query change plus a cheap water_bodies reimport. - **R63 (the delimited multi-value ref).** The candidate test is the requirement's own check, `count(*) = 0 FROM map_roads_14_pl WHERE ref ~ '[;,]'`. Measured live it FAILS: 114 rows still carry comma-delimited refs (`75, 28` × 82, `964,768`, `222, 211207G, 211208G`, …). The #19 split handles the `;` form (`E77;S7;62` — issue #39) but not the comma form BDOT10k also delivers; the fix is extending `regexp_replace(numerdrogi, ';', '|', 'g')` to `'[;,]'` plus a roads reimport. Filed as a new issue (the fix + test land together). - **R43 is genuinely not an acceptance test.** The CASE/`WHERE IN` sync is a static property of the config text; no SQL can witness it without duplicating the very list the check is meant to keep singular (that duplicate would be the drift hazard). It is a query-comment review surface — recorded as such in the doc. The #123 pass did the review: every Polish `CASE` is total (single-kind BDOT10k files, R43's own allowance), and the one shared-column `WHERE IN`/CASE pair (water_points' `rodzaj IN ('wodospad', 'źródło')`) matches — `źródło` is the `ELSE`'s referent, the least-specific class still correct for the file. Same shape as #121's R50/R51 and #122's R92 records. ## Verification (live DB, no writes) ``` $ make build && make test ok github.com/art4711/ismap/importer ok github.com/art4711/ismap/mapsrv $ ./importer/importer -acceptance-only conf/plmap.yaml acceptance: 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run $ make acceptance conf/ismap.yaml 26 passed / 0 failed / 0 skipped / 0 not run conf/semap.yaml 12 passed / 0 failed / 0 skipped / 1 not run (R35, 100 s, needs -acceptance all) conf/plmap.yaml 24 passed / 0 failed / 0 skipped / 0 not run conf/fimap.yaml 12 passed conf/world.yaml 9 passed ``` New-test highlights from the plmap run: ``` R105: PASS tables=24, empty_tables=<nil> R119: PASS (country_mask 1, water_bodies z4–z14 11, waterways 12, water_points 7, roads 10, ferries z7–z14 8, railways 10, transit 6, stations 6, places 11; all empty_tables=<nil>) ``` ## Merge interaction #122's PR (#134) is open and touches the same R119 paragraph in `docs/country-requirements.md`; whoever merges second needs a small doc rebase to fold the Sweden and Poland paragraphs together. Nothing else overlaps. Nothing left undone that I know of: R58's and R63's tests ship with their fix PRs (both specified above and tracked — #43 commented, new issue filed for the comma split), and R43 is recorded as a review surface. 
Config + doc only, the #121/#122 shape: the manual R-checks still listed as
candidates in the #115 plan for Poland become acceptance tests in
conf/plmap.yaml, run on every import of the layer and in -acceptance-only
review passes. No import was needed — every test reads data already live in
the DB, and the plmap run goes 13 -> 24 tests, all passing.

What ships:
- R105 (config-level): every Poland staging table is nonzero after load.
  The requires list is exactly the 24-name staging: block — the doc's trap
  (naming a table the config does not declare silently disables the check)
  is why.
- R119 (10 shared layers: country_mask, water_bodies, waterways,
  water_points, roads, ferries, railways, transit, stations, places): no
  zoom table in the declared range is empty, pg_relation_size so it costs
  nothing. The layers with zoom_filters record that none empties a zoom;
  two deliberate empties are recorded, not fixed:
    * water_bodies z3 — the 4-px² floor drops every lake piece (the same
      property as every other country); today's lone row is the #43
      mislabel, so the test asserts z4–z14 and survives the R58 fix.
    * ferries z3–z6 — min_length_pixels 1 is 19.6–2.4 km/px at those zooms
      and BDOT10k SKPP carries crossings, not Baltic routes (longest merged
      chain ~1 km); z7 is the effective floor.

Recorded, not shipped (the checks fail against the live DB; each ships
with its fix, the #120 pattern):
- R58: measured 2026-08-16 the 5 845 km² (3857) Morze Bałtyckie lake slab
  is still live (29x the next-largest lake, 49 km of edge shared with the
  national boundary) — the candidate test and exact predicate are commented
  on #43 for the fix PR.
- R63: 114 rows of map_roads_14_pl still carry comma-delimited refs (the
  #19 split handles ';' but not ','); filed as #135 with the one-character
  fix and the test.
- R43: not an acceptance test — the CASE/WHERE-IN sync is a static text
  property with no SQL witness that does not duplicate the very list the
  check keeps singular; recorded as a query-comment review surface (the
  #123 pass confirmed every Polish CASE is total, R43's own allowance).

Verified against the live DB (no writes):
  make build && make test  -> ok (importer, mapsrv)
  importer/importer -acceptance-only conf/plmap.yaml
    -> 24 test(s): 24 passed, 0 failed, 0 skipped, 0 not run
  make acceptance: ismap 26, semap 12+1 not-run (R35, pre-existing),
    plmap 24, fimap 12, world 9 — all passing.
art-bot commented 2026-08-16 04:14:42 +00:00
Collaborator
Copy link

Review

Verified: go test ./... green, and the plmap suite runs 24/24 against the live database. Two checks I made against the config and the data rather than taking the PR body's word for it, both clean:

  • R105 covers the staging block exactly — 24 tables declared, 24 in requires:, no extras and no omissions. The comment's argument for why that exactness matters (requires: disables the test rather than strengthening it when it names a table the config doesn't declare) is the right reasoning and worth having written down.
  • Both deliberate empties are real — map_ferries_{3,4,5,6}_pl are 0 rows with z7 at 1, and every R119 table list matches its layer's declared zoom range with only the documented gaps missing.

The ferries test is the right shape for a documented knob

Worth calling out because the parallel PR does it differently: this test asserts only that z7–z14 are populated, and says nothing about z3–z6. So when someone eventually tunes min_length_pixels and the coarse zooms fill in, the test stays green — it records the current floor without freezing it. #124's R47 check takes the opposite approach (an upper bound that fires when the documented fix lands), and I think yours is the better default: a test that goes red when a bug is fixed trains people to edit tests reflexively.

Minor: Poland's z3 is not actually empty

The test title reads R119: no water_bodies zoom table is empty (z4–z14; z3 is the deliberate empty), but Poland's z3 has a row:

map_water_bodies_3_se = 0   map_water_bodies_3_fi = 0   map_water_bodies_3_pl = 1

The exclusion from the list is still right — one row at z3 is the same "everything below the area floor" story, and the shared-contract reasoning in the comment already records pl 1 rows accurately. It is only the title that overstates it for Poland specifically. Something like z3 is below the area floor would match both the data and the comment.

Merge order

docs/country-requirements.md conflicts with #124/#137 once either lands — additive on both sides, so mechanical, but it needs a human at the merge.

## Review Verified: `go test ./...` green, and the plmap suite runs 24/24 against the live database. Two checks I made against the config and the data rather than taking the PR body's word for it, both clean: - **R105 covers the staging block exactly** — 24 tables declared, 24 in `requires:`, no extras and no omissions. The comment's argument for why that exactness matters (`requires:` *disables* the test rather than strengthening it when it names a table the config doesn't declare) is the right reasoning and worth having written down. - **Both deliberate empties are real** — `map_ferries_{3,4,5,6}_pl` are 0 rows with z7 at 1, and every R119 table list matches its layer's declared zoom range with only the documented gaps missing. ### The ferries test is the right shape for a documented knob Worth calling out because the parallel PR does it differently: this test asserts only that z7–z14 are populated, and says nothing about z3–z6. So when someone eventually tunes `min_length_pixels` and the coarse zooms fill in, the test stays green — it records the current floor without freezing it. #124's R47 check takes the opposite approach (an upper bound that fires when the documented fix lands), and I think yours is the better default: a test that goes red when a bug is *fixed* trains people to edit tests reflexively. ### Minor: Poland's z3 is not actually empty The test title reads `R119: no water_bodies zoom table is empty (z4–z14; z3 is the deliberate empty)`, but Poland's z3 has a row: ``` map_water_bodies_3_se = 0 map_water_bodies_3_fi = 0 map_water_bodies_3_pl = 1 ``` The exclusion from the list is still right — one row at z3 is the same "everything below the area floor" story, and the shared-contract reasoning in the comment already records `pl 1 rows` accurately. It is only the title that overstates it for Poland specifically. Something like `z3 is below the area floor` would match both the data and the comment. ### Merge order `docs/country-requirements.md` conflicts with #124/#137 once either lands — additive on both sides, so mechanical, but it needs a human at the merge.
art-bot force-pushed issue-123-pl-acceptance-tests from 5e9513cff4 to 305e1f3113 2026-08-16 04:31:56 +00:00 Compare
art-bot changed target branch from main to issue-122-se-acceptance-tests 2026-08-16 04:44:59 +00:00
art-bot commented 2026-08-16 04:45:32 +00:00
Collaborator
Copy link

Rebased onto issue-122-se-acceptance-tests (#134):

main ← #130 (#113) ← #134 (#122) ← #136 (this) ← #137 (#124)

Force-pushed 5e9513c → 305e1f3. One conflict resolved, in docs/country-requirements.md §14: Sweden's and Poland's R119 paragraphs both appended to the same spot and both ended in the fragment that joins to "manual form:". Kept both paragraphs, dropped the duplicated connector from Sweden's.

Re-verified after the rebase: go test ./... green, plmap 24/24 against the live DB.

**Rebased onto `issue-122-se-acceptance-tests`** (#134): ``` main ← #130 (#113) ← #134 (#122) ← #136 (this) ← #137 (#124) ``` Force-pushed `5e9513c → 305e1f3`. One conflict resolved, in `docs/country-requirements.md` §14: Sweden's and Poland's R119 paragraphs both appended to the same spot and both ended in the fragment that joins to "manual form:". Kept both paragraphs, dropped the duplicated connector from Sweden's. Re-verified after the rebase: `go test ./...` green, plmap 24/24 against the live DB.
art changed target branch from issue-122-se-acceptance-tests to main 2026-08-16 05:18:04 +00:00
art merged commit 305e1f3113 into main 2026-08-16 05:18:21 +00:00
art deleted branch issue-123-pl-acceptance-tests 2026-08-16 05:18:21 +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!136
No description provided.