Test audit of #39: lollipop/self-crossing snapping, fix a real self-crossing bug #116

Merged

robert merged 1 commit from area/navigation-snapping-test-audit into main 2026-09-05 22:42:27 +02:00

Conversation 0 Commits 1 Files changed 2 +171 -10

robert commented 2026-09-05 22:39:37 +02:00

Owner

Copy link

Audits issue #39's acceptance criteria against #31's RouteSnapper (merged tonight as PR #114) and adds the real, missing test scope. Does not close #39 — two criteria are genuinely out of this issue's honest scope (see below) and get dependency edges instead.

Criteria already covered by RouteSnapperTest (#31)

Verified non-superficial (tight tolerances, not "some value came back"):

• Out-and-back not jumping to the return leg — "an out-and-back route does not jump from the outbound leg to the geometrically coincident return leg" asserts offsetMeters < 1.0 and correct segmentStartIndex/monotonic distance across the turnaround.
• Re-acquisition after a GPS gap — "the search window widens after a GPS gap..." asserts the post-gap distance to within plusOrMinus 5.0 of the true 400 m value, which is what actually proves the widened window fired rather than merely "some progress was made".
• Rejoining further along the route — "the search window widens to re-acquire the route after an off-route excursion" asserts the post-rejoin distance to within plusOrMinus 5.0 of the true 450 m value.

No new tests added for these three; writing near-duplicates would not have added real coverage.

Criterion genuinely missing: lollipop and self-crossing routes

Not covered by #31 (only a plain out-and-back was tested). Auditing it surfaced a real, previously-undiscovered bug, not just an untested case:

On a route that crosses itself, RouteSnapper's forward-only windowed search can contain both the rider's true, nearby position on the polyline and a much later revisit of the same physical point (the crossing itself). Because the underlying locateAlongPolyline simply picks whichever segment in the window has the globally smallest offset, a few meters of completely ordinary GPS noise can make the far, chronologically-wrong segment look marginally closer than the true one — and the search cursor jumps onto it, irrecoverably, since forward-only search never looks back.

Reproduced concretely: an 8 m half-size figure-eight (~226 m total route length) with only 3 m of perpendicular noise near the crossing snapped the cursor from ~113 m into the ride straight to ~223 m — permanently — with the reported offset then growing on every subsequent fix even though the rider was genuinely still on-route, riding the second loop.

Fix (RouteSnapper.kt): a two-phase search in snap(). On the plain, un-widened path (no GPS gap, previous fix looked on-route), try a tight, plausibility-gated window first (IMMEDIATE_SEARCH_HORIZON_METERS = 60 m, maxOffsetMeters = the existing REACQUISITION_OFFSET_THRESHOLD_METERS = 50 m). Only when that tight search finds nothing plausible does it fall back to the exact original full search, completely unchanged (same horizon logic, same maxOffsetMeters = Double.MAX_VALUE, same honest-large-offset reporting).

This guarantees zero behavioural change on every already-covered path:

• Verified: the full RouteSnapperTest suite, including the real-komoot GPX regression fixture, passes unchanged with the fix applied.
• Verified: the new figure-eight test genuinely fails against the pre-fix code (git-stash-checked) and passes after — a real regression test, not a decorative one.

Also added a lollipop fixture (stem out, loop via a different, physically distinct path, stem back) confirming that shape already tracks correctly without needing the fix, since its coincident points are well separated in cumulative route distance at a realistic loop size — an honest "this part already works" finding, not assumed.

Both new tests and the fix live in:

• companion/core/src/main/kotlin/de/butzei/pedalpebble/core/route/nav/RouteSnapper.kt
• companion/core/src/test/kotlin/de/butzei/pedalpebble/core/route/nav/RouteSnapperTest.kt

Criteria explicitly NOT closed here

Off-route enter/exit hysteresis at the thresholds belongs to #32 ("Off-route detection with hysteresis and auto-recovery"), a separate open issue. RouteSnapper deliberately does not implement hysteresis — it only reports a raw offsetMeters for #32 to build on (see RouteSnap's own KDoc). There is no OffRouteDetector.kt anywhere in the repo yet (confirmed by search). Fabricating a test against a class that doesn't exist would be worse than leaving this honestly untested. Dependency edge added: #39 depends on #32.

Distance-to-turn and ETA against hand-computed values belongs to #33 ("Next turn, then-turn, remaining distance and ETA"), a separate open issue targeting a NavEngine.kt that does not exist yet (confirmed by search — the nav/ package currently contains only RouteSnapper.kt). Cue.distanceAlongRouteMeters (from #26/#63's cue-sheet enrichment) and RouteSnap.distanceAlongRouteMeters (from #31) both already exist independently and a distance-to-turn figure is a straightforward subtraction of the two, but no code anywhere actually does that combination yet, and no rolling-speed-based ETA exists at all. Dependency edge added: #39 depends on #33.

Issue #39 is left open — the hysteresis/ETA scope is real and honestly untested by design, not overlooked. Leaving the closing decision to Robert per standing instructions.

Verification

Real ./gradlew :companion:core:test --rerun-tasks run (all core tests, not just RouteSnapperTest) — green. ./gradlew :companion:core:koverVerify — green, coverage floor intact.

https://claude.ai/code/session_01DAoXbRmJUf2uxNYBfdAXPt

Audits issue #39's acceptance criteria against #31's `RouteSnapper` (merged tonight as PR #114) and adds the real, missing test scope. Does **not** close #39 — two criteria are genuinely out of this issue's honest scope (see below) and get dependency edges instead. ## Criteria already covered by `RouteSnapperTest` (#31) Verified non-superficial (tight tolerances, not "some value came back"): - **Out-and-back not jumping to the return leg** — `"an out-and-back route does not jump from the outbound leg to the geometrically coincident return leg"` asserts `offsetMeters < 1.0` and correct `segmentStartIndex`/monotonic distance across the turnaround. - **Re-acquisition after a GPS gap** — `"the search window widens after a GPS gap..."` asserts the post-gap distance to within `plusOrMinus 5.0` of the true 400 m value, which is what actually proves the widened window fired rather than merely "some progress was made". - **Rejoining further along the route** — `"the search window widens to re-acquire the route after an off-route excursion"` asserts the post-rejoin distance to within `plusOrMinus 5.0` of the true 450 m value. No new tests added for these three; writing near-duplicates would not have added real coverage. ## Criterion genuinely missing: lollipop and self-crossing routes Not covered by #31 (only a plain out-and-back was tested). Auditing it surfaced **a real, previously-undiscovered bug**, not just an untested case: On a route that crosses itself, `RouteSnapper`'s forward-only windowed search can contain both the rider's true, nearby position on the polyline *and* a much later revisit of the same physical point (the crossing itself). Because the underlying `locateAlongPolyline` simply picks whichever segment in the window has the globally smallest offset, a few meters of *completely ordinary* GPS noise can make the far, chronologically-wrong segment look marginally closer than the true one — and the search cursor jumps onto it, irrecoverably, since forward-only search never looks back. Reproduced concretely: an 8 m half-size figure-eight (~226 m total route length) with only 3 m of perpendicular noise near the crossing snapped the cursor from ~113 m into the ride straight to ~223 m — permanently — with the reported offset then *growing on every subsequent fix* even though the rider was genuinely still on-route, riding the second loop. **Fix** (`RouteSnapper.kt`): a two-phase search in `snap()`. On the plain, un-widened path (no GPS gap, previous fix looked on-route), try a tight, plausibility-gated window first (`IMMEDIATE_SEARCH_HORIZON_METERS` = 60 m, `maxOffsetMeters` = the existing `REACQUISITION_OFFSET_THRESHOLD_METERS` = 50 m). Only when that tight search finds nothing plausible does it fall back to the **exact original full search, completely unchanged** (same horizon logic, same `maxOffsetMeters = Double.MAX_VALUE`, same honest-large-offset reporting). This guarantees zero behavioural change on every already-covered path: - Verified: the full `RouteSnapperTest` suite, including the real-komoot GPX regression fixture, passes unchanged with the fix applied. - Verified: the new figure-eight test genuinely **fails** against the pre-fix code (git-stash-checked) and passes after — a real regression test, not a decorative one. Also added a lollipop fixture (stem out, loop via a different, physically distinct path, stem back) confirming that shape already tracks correctly *without* needing the fix, since its coincident points are well separated in cumulative route distance at a realistic loop size — an honest "this part already works" finding, not assumed. Both new tests and the fix live in: - `companion/core/src/main/kotlin/de/butzei/pedalpebble/core/route/nav/RouteSnapper.kt` - `companion/core/src/test/kotlin/de/butzei/pedalpebble/core/route/nav/RouteSnapperTest.kt` ## Criteria explicitly NOT closed here **Off-route enter/exit hysteresis at the thresholds** belongs to #32 ("Off-route detection with hysteresis and auto-recovery"), a separate open issue. `RouteSnapper` deliberately does not implement hysteresis — it only reports a raw `offsetMeters` for #32 to build on (see `RouteSnap`'s own KDoc). There is no `OffRouteDetector.kt` anywhere in the repo yet (confirmed by search). Fabricating a test against a class that doesn't exist would be worse than leaving this honestly untested. **Dependency edge added: #39 depends on #32.** **Distance-to-turn and ETA against hand-computed values** belongs to #33 ("Next turn, then-turn, remaining distance and ETA"), a separate open issue targeting a `NavEngine.kt` that does not exist yet (confirmed by search — the `nav/` package currently contains only `RouteSnapper.kt`). `Cue.distanceAlongRouteMeters` (from #26/#63's cue-sheet enrichment) and `RouteSnap.distanceAlongRouteMeters` (from #31) both already exist independently and a distance-to-turn figure is a straightforward subtraction of the two, but no code anywhere actually does that combination yet, and no rolling-speed-based ETA exists at all. **Dependency edge added: #39 depends on #33.** Issue #39 is left **open** — the hysteresis/ETA scope is real and honestly untested by design, not overlooked. Leaving the closing decision to Robert per standing instructions. ## Verification Real `./gradlew :companion:core:test --rerun-tasks` run (all core tests, not just `RouteSnapperTest`) — green. `./gradlew :companion:core:koverVerify` — green, coverage floor intact. https://claude.ai/code/session_01DAoXbRmJUf2uxNYBfdAXPt

robert added 1 commit 2026-09-05 22:39:38 +02:00

Test audit of #39: lollipop/self-crossing snapping, fix a real self-crossing bug ... edf8dfb66a

Audits issue #39's acceptance criteria against #31's RouteSnapper (merged as
PR #114) and adds the real, missing test scope.

Criteria already covered by RouteSnapperTest (#31), verified non-superficial
(tight tolerances, not just "some value came back"):
- Out-and-back not jumping to the return leg
- Re-acquisition after a GPS gap
- Rejoining further along the route after an off-route excursion

Criterion genuinely missing and added here: lollipop and self-crossing
routes. Auditing this surfaced a real bug, not just an untested case: on a
route that crosses itself, the forward-only windowed search can contain both
the rider's true nearby position and a much later revisit of the same
physical point (the crossing). A few meters of ordinary GPS noise can make
the far, chronologically-wrong segment look marginally closer, and the
search cursor jumps onto it irrecoverably (forward-only search never looks
back). Reproduced concretely: an 8 m half-size figure-eight (~226 m total)
with 3 m of perpendicular noise near the crossing snapped the cursor from
~113 m into the ride straight to ~223 m, permanently, with the reported
offset then growing on every subsequent fix even though the rider was
genuinely still on-route on the second loop.

Fixed in RouteSnapper.snap() with a two-phase search: try a tight,
plausibility-gated window first on the un-widened path (no GPS gap, last fix
looked on-route); only fall back to the original full search - completely
unchanged - when that tight search finds nothing plausible. This guarantees
zero behavioural change on every already-covered path (verified: full
RouteSnapperTest suite, including the real-komoot GPX regression fixture,
passes unchanged), and the new figure-eight test is confirmed to fail
against the pre-fix code (git-stash-verified) and pass after.

Also added a lollipop fixture (stem out, loop via a different path, stem
back) confirming that shape already tracks correctly without the fix, since
its coincident points are well separated in cumulative route distance.

Two criteria are out of this issue's honest scope and not closed here:

- Off-route enter/exit hysteresis at the thresholds belongs to #32 (Off-route
  detection with hysteresis and auto-recovery), a separate open issue.
  RouteSnapper deliberately does not implement hysteresis - it only reports
  a raw offsetMeters for #32 to build on. There is no OffRouteDetector.kt
  anywhere in the repo yet (confirmed by search). Dependency edge added:
  #39 depends on #32.

- Distance-to-turn and ETA against hand-computed values belongs to #33
  (Next turn, then-turn, remaining distance and ETA), a separate open issue
  targeting a NavEngine.kt that does not exist yet (confirmed by search).
  Cue.distanceAlongRouteMeters and RouteSnap.distanceAlongRouteMeters both
  exist independently, but no NavEngine combines them into a distance-to-turn
  figure, and no rolling-speed-based ETA exists anywhere. Dependency edge
  added: #39 depends on #33.

Issue #39 is left open, per instructions - the hysteresis/ETA scope remains
real and untested by design, not overlooked.

Claude-Session: https://claude.ai/code/session_01DAoXbRmJUf2uxNYBfdAXPt

robert referenced this pull request 2026-09-05 22:40:01 +02:00

Unit tests: snapping on out-and-back routes, off-route hysteresis #39

robert merged commit 0e844106eb into main 2026-09-05 22:42:27 +02:00

robert referenced this pull request from a commit 2026-09-05 22:42:28 +02:00

Merge pull request 'Test audit of #39: lollipop/self-crossing snapping, fix a real self-crossing bug' (#116) from area/navigation-snapping-test-audit into main

robert referenced this pull request 2026-09-05 22:42:38 +02:00

Unit tests: snapping on out-and-back routes, off-route hysteresis #39

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:companion

Android companion app

  

area:docs

Documentation

  

area:shared

Message contract shared by both sides

  

area:tooling

Build tooling, SDK, dev scripts

  

area:watchapp

Pebble C watchapp

  

blocker

Blocks a large part of the project until resolved

  

kind:chore

Setup, scaffolding, maintenance

  

kind:feature

New functionality

  

kind:spike

Time-boxed investigation to de-risk a decision

  

kind:test

Tests and fixtures

No labels

area:companion

area:docs

area:shared

area:tooling

area:watchapp

blocker

kind:chore

kind:feature

kind:spike

kind:test

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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

robert/PedalPebble!116

No description provided.