Expand unit test coverage and fix the issues found - #1041
Merged
Conversation
…d ViewModels Fills the largest gaps in unit test coverage across shared/ and app/: compass and routing utils, CalloutController and the manual-callout builders, the head-tracking calibration/provider layer, and all previously-untested ViewModels. Adds Mockito to app/build.gradle.kts to make the Context-bound onboarding/settings ViewModels testable. Also fixes two bugs the new tests surfaced: - dijkstraOnWaysWithLoops returned on first edge relaxation reaching `end` instead of once `end` is popped off the priority queue with a guaranteed- minimal distance, so it could return a non-optimal (too-long) shortest path. - CalloutController.startCallout let an exception from the callout body (e.g. a button pressed before GeoEngine.start() has finished) escape the launched coroutine, cancelling SoundscapeService's root Job and silently disabling every future callout button press for the rest of the service's lifetime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iew, and geocoders Continues the coverage sweep: road-naming confection, the Street Preview state machine, route/beacon playback, the beacon style preview controller, route-share JSON, and the Photon/Fused/Multi geocoder orchestration classes. Also fixes three bugs the new tests surfaced: - routeToShareJson() built JSON via raw string templates with no escaping, so a `"` or `\` in a route/marker name, description, or address produced invalid JSON. Now built via kotlinx.serialization's JSON DSL. - StreetPreview.go() computed the heading of a newly-reached junction from the first way chosen at the previous junction rather than the way actually last followed to get there, so a route that bends through a pass-through node reported a physically wrong facing direction - throwing off which choice reads as "straight ahead" at the next junction. - RoutePlayer.moveToNext() always returned true, even at the last waypoint, unlike moveToPrevious() which correctly returns false at the start boundary. RoutePlayer.startBeacon() also hardcoded currentRouteFlow's beaconOnly to true instead of using the already-computed distance-based value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…overwrite
PhotonGeocoder.getAddressFromLocationName used `feature.properties?.get("name").toString()`
for name/featureType/featureClass - the `?.` only guarded the map lookup, so a
genuinely absent property became the literal string "null" instead of staying
null, which MvtFeature.getText() then treated as a real name (surfacing "Null"
to the user instead of falling back to a class-based description). Switched to
`?.toString()` throughout, and extracted the now-shared MvtFeature-building
logic into one place also used by getAddressFromLngLat, which previously never
computed featureName at all.
FusedGeocoder.getAddressFromLocationName merges a nearby Photon street-number
match's name into the platform result unconditionally. PhotonGeocoder itself
never populates `.name` (only `.featureName`) unless a caller's processor has
already run LocationDescription.process() on the result, so a PhotonGeocoder
used without that specific wiring would silently blank out a good platform
name during the merge. Guarded the merge to only overwrite when the photon
name is non-blank.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r, and small utils Continues the coverage sweep: offline map download/extract orchestration, the shared speakCallout rendering logic, tile source resolution, a ResourceMapper spot-check suite, and a bundle of small string/byte-format/ share-text/route-name utilities. Added ktor-client-mock to shared's commonTest to make OfflineMapManager's HTTP-backed refresh() testable. Also fixes five bugs the new tests surfaced: - ResourceMapper's OSM-tag lookup table had a duplicate "fuel" entry; the later put() silently overwrote the earlier, intended "Gas Station" mapping with the generic "Fuel Station" one. Removed the duplicate. - TileSourceResolver.resolveTileSourceUrl(location = null) returned offlineExtractPaths[0] - whatever order the filesystem happened to list extracts in - contradicting its own doc comment's "best (largest) local extract" guarantee. Now picks the largest validated extract, matching the location != null case. - OfflineMapManager.startDownload()'s retry loop had no terminal state after exhausting all 10 attempts on repeated HTTP 503s: downloadState was left stuck on Caching forever with no indication the download had failed. Now sets an Error state and cleans up the temp file. - FormatBytes.formatBytes()'s unit-selection loop compared the raw (pre-rounding) value, so e.g. 999_500 bytes (999.5 kB, which rounds to "1000" for display) never advanced to MB, showing "1000 kB" instead of "1.0 MB". Added a post-rounding recheck. - ShareLocationText.buildShareLocationText() chained four .replace() calls over the same growing string, so a location name containing literal placeholder text (e.g. named "%2$s") could get swept up and rewritten by a later substitution meant only for template placeholders. Switched to a single regex pass against the original template. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…classes Continues the coverage sweep: street-address description/house-number logic, named-way search disambiguation, search-string Unicode normalization, GPX recording, and the Ktor-backed Photon/vector-tile network clients. Also fixes seven bugs the new tests surfaced: - GpxRecorder.storeLocation()'s buffer eviction called List.drop(1), which returns a new list rather than mutating the MutableList - the result was discarded, so the buffer grew unbounded instead of being capped. Now uses removeAt(0). - GpxRecorder.generateGpx() stamped every <trkpt> with a single timestamp computed once at generation time, instead of each location's own recorded timestampMilliseconds - exported GPX tracks had meaningless timing data. - TileSearch.generateWithoutSettlement() and addLastWords() both had a post-decrement loop-break check (`--count == 0`) that can never fire once the counter starts at (or is driven to) 0, leaving the settlement-stripped string unchanged instead of empty, and a stray leading space respectively. - StreetDescription.getStreetNumber() computed which side of the street a query point is on using the *opposite* direction convention from the one addHouse() used when recording each house's side - so a house recorded on the left was looked up as being on the right, and vice versa. - StreetDescription.distanceAlongStreet() compared the original, unchanged `distance` parameter against each way's length instead of the remaining distance, causing it to run off the end of a multi-segment street and return null instead of interpolating. - StreetDescription.createDescription() added the last way's length twice when recording the trailing descriptive intersection, inflating the recorded distance to the street's final junction/dead-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
While investigating why StreetDescription.assignHouseNumberModes() was returning MIXED for almost every real street tested, found several compounding issues: - MvtToGeoJson.kt's addToStreetNumberMap() read a housenumber feature's street name from `properties["street"]`, but the tag-parsing loop stores a parsed "street" tag in a dedicated `street` field instead - it's never copied into `properties`. So any POI/building feature that carries both a name and a housenumber (e.g. a car park tagged with both an amenity name and a full address) was always misfiled into the "unknown street" bucket, regardless of having a perfectly good street tag. This let it become a candidate for a nearby, unrelated street's description. - StreetDescription's own POI-matching code had the identical bug, reading `properties["street"]` instead of the `street` field - so even the house's own address tag check (added as defense-in-depth here) wasn't actually filtering on anything. - The unknown-street house-number search only checked whether a candidate was nearest among a street's *own* segments, never comparing against other real named streets nearby - so an untagged house genuinely closer to a different street (e.g. across a shared junction) could still be attached to the wrong one. - assignHouseNumberModes()'s odd/even counts were computed before checkSortedNumberConsistency() had a chance to filter out numbering outliers, so that filtering had no effect on the mode decision it exists to inform. Together these caused real streets to spuriously classify as MIXED even when their numbering was almost entirely consistent. Verified against real Glasgow-area map data: 6 of 7 streets in SearchTest.testStreetDescription now correctly detect odd/even sides, up from 1 of 7 before this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davecraig
had a problem deploying
to
development
August 26, 2026 14:09 — with 
GitHub Actions
Failure
GitHub Actions
Failure
davecraig
had a problem deploying
to
development
August 26, 2026 14:09 — with
GitHub Actions
Failure
GitHub Actions
Failure
…bering assignHouseNumberModes() required 100% purity on each side (every number odd, or every number even) before classifying a street as ODD/EVEN, falling back to MIXED over even a single confidently-tagged exception. Real address data is rarely that clean - e.g. Sauchiehall Street in Glasgow has 41 even house numbers against just 2 odd ones on one side, both genuinely tagged street=Sauchiehall Street in OSM (likely historic renumbering/redevelopment, not misattribution). assignHouseNumberModes() now tolerates up to 10% opposite-parity exceptions per side (isDominantParity()) before giving up and calling a side MIXED. Since a side can now be classified as ODD/EVEN despite carrying a known exception, createDescription() strips those exceptions out of leftSortedNumbers/rightSortedNumbers afterwards (removeParityExceptions()), so they can't corrupt floor/ceiling interpolation in getStreetNumber() / getLocationFromStreetNumber() for genuinely unknown addresses nearby. assignHouseNumberModes() itself stays side-effect-free. Verified against real Glasgow-area map data: all 7 streets in SearchTest.testStreetDescription now correctly detect their odd/even sides, up from 1 of 7 at the start of this investigation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davecraig
had a problem deploying
to
development
August 26, 2026 14:59 — with
GitHub Actions
Failure
GitHub Actions
Failure
… flakiness The test drives production code that hardcodes Dispatchers.Default, so it waits on real thread scheduling rather than virtual time. It timed out waiting for StateFlow emissions on a shared CI runner (32978550205) and passed with no code changes on the very next run, confirming this was scheduling jitter rather than a logic bug. Widening the bound doesn't slow the common case since the loop exits as soon as its condition is met. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The test used java.io.File and java.lang.System, which don't exist on
Kotlin/Native, breaking compileTestKotlinIosSimulatorArm64 on every CI run
since this test was added. Switched to the same Okio Path/systemFileSystem
idiom OfflineMapManager itself and TileSourceResolverTest already use, so
the test now actually runs on iOS instead of just being silently skipped
there. Also replaced a GzipSink.use{} call whose Closeable type doesn't
resolve to kotlin.AutoCloseable on Native with an explicit close().
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davecraig
requested a deployment
to
development
August 26, 2026 15:17 — with
GitHub Actions
Abandoned
GitHub Actions
Abandoned
davecraig
requested a deployment
to
development
August 26, 2026 15:17 — with
GitHub Actions
Abandoned
GitHub Actions
Abandoned
AddressFormatterTest, RoundaboutsTest, and CalloutHistoryTest only exercise code that already lives in shared/commonMain, so they run there directly (and now also compile for iOS) instead of only in the app module's JVM unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davecraig
had a problem deploying
to
development
August 27, 2026 07:32 — with
GitHub Actions
Failure
GitHub Actions
Failure
davecraig
had a problem deploying
to
development
August 27, 2026 07:32 — with
GitHub Actions
Failure
GitHub Actions
Failure
AddressFormatterTest and CalloutHistoryTest moved from app/src/test (JVM-only) to shared/commonTest in d72dab6, which also compiles for iOS Kotlin/Native where org.junit isn't available. Switch to kotlin.test equivalents, including replacing bare stdlib assert() (opt-in-gated on Native) with assertTrue/assertEquals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…signal myLocation_calledBeforeGeoEngineStarted_resetsActiveFlow_andScopeSurvives already had its withTimeout bound doubled once (97c3ebc) for CI flakiness and still recurred. Rather than widen it again, replace the delay(5)-based polling loop with a Channel signalled from the flow collector, so the wait suspends until notified instead of depending on polling cadence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lots of new unit tests and quite a lot of issues found and fixed.