From 714d89a46dd21def0cbaf96dc564a72bdcb49481 Mon Sep 17 00:00:00 2001 From: Milton Barrera Date: Sun, 8 Mar 2026 20:40:04 -0600 Subject: [PATCH 1/5] adding docs folder with instructions to test the app --- docs/BUGS.md | 725 ++++++++++++++++++++++++++++++++++++++++++++++++ docs/TESTING.md | 403 +++++++++++++++++++++++++++ 2 files changed, 1128 insertions(+) create mode 100644 docs/BUGS.md create mode 100644 docs/TESTING.md diff --git a/docs/BUGS.md b/docs/BUGS.md new file mode 100644 index 00000000..4af0a04c --- /dev/null +++ b/docs/BUGS.md @@ -0,0 +1,725 @@ +# OSMTracker Android — Bug Report Catalogue + +This document describes **9 confirmed bugs** found in the `DataHelper` and `GPSLogger` +modules during a static-analysis + unit-test audit (branch `new-test-cases`). + +For each bug you will find: +- A precise description and the affected code location. +- The severity and the risk if left unfixed. +- The unit test(s) that cover it and **a detailed explanation of why those tests pass + today even though the bug is still present**. +- A ready-to-paste GitHub issue description. + +--- + +## How can a test pass when a bug exists? + +There are two complementary strategies used in this catalogue: + +| Strategy | When used | What it means | +|----------|-----------|---------------| +| **Assert the broken behaviour** | Crash bugs (NPE, missing guard) | `assertThrows(NullPointerException.class, ...)` — the test *expects* the crash. It passes today because the crash *does* happen. When the bug is fixed the assertion must be removed. | +| **Assert the absence of the correct behaviour** | Resource/threading bugs | e.g. `verify(cursor, never()).close()` — the test asserts that `close()` is *never* called. It passes today because the bug means `close()` is indeed never called. When the bug is fixed, the `never()` must change to a plain `verify()`. | + +This technique is sometimes called **characterisation testing**: tests lock in the current +(broken) behaviour so that any future change to the code immediately shows up as a test +result change, prompting the developer to either confirm the fix or update the test. + +--- + +## Bug B1 — `wayPoint()`: NPE before null guard + +### Location +`DataHelper.java`, lines 213–214 (inside `wayPoint()`): +```java +Log.d(TAG, "Tracking waypoint … nbSatellites=" + location.getExtras().getInt("satellites") …); +// ↑ This line runs BEFORE the guard on line 220: +if (location != null) { … } +``` + +### Description +The method accesses `location.getExtras().getInt("satellites")` in a log statement at the +very start of the method body, **before** any null check on `location` or on its extras +bundle. This means: + +- Passing `location = null` (which the comment on line 218 acknowledges can happen) throws + `NullPointerException` immediately. +- Passing a `Location` whose extras bundle was never initialised (common when constructing + a `Location` programmatically) also throws `NullPointerException` because + `getExtras()` returns `null`. + +The `if (location != null)` guard on line 220 was intended to protect the insertion logic, +but the crash happens before it is ever reached. + +### Severity: CRITICAL — app crash + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `E1_wayPoint_throwsNPEWhenLocationIsNull` | `assertThrows(NullPointerException.class, …)` | The NPE is thrown on line 214; `assertThrows` catches it and the test is green. | +| `E2_wayPoint_throwsNPEWhenLocationExtrasIsNull` | `assertThrows(NullPointerException.class, …)` | `getExtras()` returns `null`; `.getInt(…)` on null throws NPE; `assertThrows` catches it. | + +When the bug is fixed (null-check the location and its extras before the log line), both +`assertThrows` calls will no longer see an exception and the tests will fail — that is the +intended signal to update the assertions. + +### Fix hint +Move the `if (location != null)` guard to wrap the entire method body, including the log +statement, and add a separate guard for `location.getExtras() != null`. + +--- + +### GitHub issue body + +**Title:** `wayPoint()` crashes with NPE when `location` is null or has no extras bundle + +``` +## Bug description +`DataHelper.wayPoint()` calls `location.getExtras().getInt("satellites")` in a log +statement (line 214) **before** the `if (location != null)` guard on line 220. + +Passing a null `location` — or a `Location` object whose extras bundle was never +initialised — throws `NullPointerException` immediately, crashing any operation that +tries to record a waypoint under those conditions. + +## Reproduction +```java +dataHelper.wayPoint(trackId, null, "name", null, uuid, 0, 0, 0); +// → NullPointerException at DataHelper.java:214 + +Location loc = new Location("gps"); // no setExtras() call +dataHelper.wayPoint(trackId, loc, "name", null, uuid, 0, 0, 0); +// → NullPointerException at DataHelper.java:214 +``` + +## Expected behaviour +The method should silently return (or log a warning) when `location` is null, +consistent with the developer comment already on line 218: +> "location should not be null, but sometime is." + +## Affected file +`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 213–220 + +## Labels +`bug`, `crash`, `null-safety` +``` + +--- + +## Bug B2 — `trackNote()`: NPE, no null guard at all + +### Location +`DataHelper.java`, lines 326–328 (inside `trackNote()`): +```java +Log.d(TAG, "Tracking note … nbSatellites=" + location.getExtras().getInt("satellites") …); +// No null check on location or its extras anywhere in this method +``` + +### Description +Unlike `wayPoint()`, which at least has a (misplaced) `if (location != null)` guard, +`trackNote()` has **no null protection whatsoever**. The log statement on line 327 +dereferences `location.getExtras()` unconditionally. Any caller that passes a `Location` +object without a pre-initialised extras bundle will trigger an immediate NPE. + +### Severity: CRITICAL — app crash + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `F1_trackNote_throwsNPEWhenLocationExtrasIsNull` | `assertThrows(NullPointerException.class, …)` | `getExtras()` returns `null`; chaining `.getInt(…)` throws NPE; `assertThrows` catches it. | + +### Fix hint +Add a null check on `location` (and `location.getExtras()`) before the log statement, +mirroring the fix required for B1. + +--- + +### GitHub issue body + +**Title:** `trackNote()` crashes with NPE — no null guard on `location` or its extras + +``` +## Bug description +`DataHelper.trackNote()` calls `location.getExtras().getInt("satellites")` in its first +log statement (line 327) without any null check on `location` or on its extras bundle. +Unlike `wayPoint()`, there is no guard anywhere in this method. + +## Reproduction +```java +Location loc = new Location("gps"); // no setExtras() +dataHelper.trackNote(trackId, loc, "note", uuid); +// → NullPointerException at DataHelper.java:327 +``` + +## Expected behaviour +The method should record the note even when the location has no extras bundle. The +satellite count should be treated as 0 or omitted from the log. + +## Affected file +`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 325–328 + +## Labels +`bug`, `crash`, `null-safety` +``` + +--- + +## Bug B3 — `getSegmentIdFor()`: Cursor never closed — resource leak + +### Location +`DataHelper.java`, lines 419–429 (inside `getSegmentIdFor()`): +```java +public static long getSegmentIdFor(long trackId, ContentResolver cr) { + Cursor ca = cr.query(…); // cursor opened here + + if (!ca.moveToFirst()) { + return 0; // ← cursor NOT closed before early return + } + + return Track.build(trackId, ca, cr, true).getMaxSegId(); + // ← cursor NOT closed in the success path either +} +``` + +### Description +The `Cursor` returned by `ContentResolver.query()` is opened at the top of the method but +is **never closed** in either code path — neither the early-return path (track not found) +nor the success path. This method is called every time GPS tracking starts (and potentially +on every segment boundary), so each call leaks a database cursor. Over time this exhausts +the cursor window pool, causing `SQLiteCantOpenDatabaseException` or +`CursorWindowAllocationException` on long-running tracking sessions. + +### Severity: HIGH — resource leak, potential crash after extended use + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `A2_getSegmentIdFor_doesNotCloseCursorInEarlyReturnPath_documentedBug` | `verify(mockCursor, never()).close()` | The mock cursor's `close()` is genuinely never called, so `never()` is satisfied. | + +The assertion uses `never()` deliberately: it documents that `close()` is *absent*. When +the bug is fixed and `close()` is added, this `never()` assertion will fail — the +developer must then change it to `verify(mockCursor).close()`. + +### Fix hint +Wrap the method body in a `try-finally` block, or use try-with-resources if targeting +API 16+ (which this project does). + +--- + +### GitHub issue body + +**Title:** `getSegmentIdFor()`: Cursor opened but never closed — resource leak on every track start + +``` +## Bug description +`DataHelper.getSegmentIdFor()` opens a `Cursor` via `ContentResolver.query()` but never +calls `cursor.close()` in either code path (early return when the track is not found, or +the normal return after calling `Track.build()`). + +This method is invoked every time GPS tracking starts. Each invocation leaks one cursor +handle. On long recording sessions the cursor pool can be exhausted, causing +`CursorWindowAllocationException` or `SQLiteCantOpenDatabaseException`. + +## Affected file +`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 419–429 + +## Suggested fix +```java +public static long getSegmentIdFor(long trackId, ContentResolver cr) { + Cursor ca = cr.query(…); + if (ca == null) return 0; + try { + if (!ca.moveToFirst()) return 0; + return Track.build(trackId, ca, cr, true).getMaxSegId(); + } finally { + ca.close(); + } +} +``` + +## Labels +`bug`, `database`, `resource-leak` +``` + +--- + +## Bug B4 — `getTrackById()`: No null or empty-cursor guard + +### Location +`DataHelper.java`, lines 584–595 (inside `getTrackById()`): +```java +public Track getTrackById(long trackId) { + Cursor c = context.getContentResolver().query(…); + Log.d(TAG, "Count: " + c.getCount()); // NPE if c is null + c.moveToFirst(); // no check on return value + Track track = Track.build(trackId, c, contentResolver, true); // reads invalid position + c.close(); + return track; +} +``` +Even the developer left a `//TODO: Fix this method` comment above it. + +### Description +Two distinct failure scenarios: + +1. **Null cursor**: `ContentResolver.query()` is permitted to return `null` (e.g., if the + provider crashes or is unavailable). Calling `c.getCount()` on a null reference throws + NPE immediately. +2. **Empty cursor**: When the given `trackId` does not exist in the database, the cursor + is valid but empty. `moveToFirst()` returns `false` (the cursor stays at position −1), + but its return value is never checked. The subsequent `Track.build()` call reads column + values from an invalid cursor position, throwing + `CursorIndexOutOfBoundsException`. + +### Severity: HIGH — crash on invalid or missing track ID + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `B1_getTrackById_throwsNPEWhenCursorIsNull` | `assertThrows(NullPointerException.class, …)` | Mock ContentResolver returns null; `c.getCount()` throws NPE; `assertThrows` catches it. | +| `B2_getTrackById_throwsForNonExistentId` | `assertThrows(RuntimeException.class, …)` | Empty cursor; `Track.build()` reads position −1; `CursorIndexOutOfBoundsException` is thrown and caught. | + +### Fix hint +Check `c != null` and `c.moveToFirst()` before calling `Track.build()`. Return `null` (or +throw a meaningful checked exception) on failure, and always close the cursor. + +--- + +### GitHub issue body + +**Title:** `getTrackById()` crashes on null cursor or non-existent track ID + +``` +## Bug description +`DataHelper.getTrackById()` has two crash paths (acknowledged by the TODO comment above +the method): + +1. If `ContentResolver.query()` returns `null`, `c.getCount()` throws NPE. +2. If the track does not exist, `moveToFirst()` is called but its `false` return is + ignored. `Track.build()` then reads from an invalid cursor position, throwing + `CursorIndexOutOfBoundsException`. + +## Reproduction +```java +// Case 1: provider returns null +dataHelper.getTrackById(1L); // with mocked null cursor → NPE + +// Case 2: ID does not exist +dataHelper.getTrackById(99999L); // → CursorIndexOutOfBoundsException +``` + +## Affected file +`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 582–595 + +## Labels +`bug`, `crash`, `database` +``` + +--- + +## Bug B5 — `getWayPointById()` / `getTrackPointById()`: Same null/empty-cursor pattern + +### Location +`DataHelper.java`: +- `getWayPointById()`, lines 618–630 +- `getTrackPointById()`, lines 652–664 + +### Description +Both methods share the same structural flaw as B4: +- No null check on the cursor returned by `ContentResolver.query()`. +- No check on the return value of `moveToFirst()` before passing the cursor to the + `WayPoint(Cursor)` / `TrackPoint(Cursor)` constructors. +- Neither method closes the cursor after use (a secondary resource leak). + +When a non-existent ID is queried, the cursor is empty and the constructor reads from an +invalid position, crashing with `CursorIndexOutOfBoundsException`. + +### Severity: HIGH — crash on invalid waypoint or trackpoint ID + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `C1_getWayPointById_throwsNPEWhenCursorIsNull` | `assertThrows(NullPointerException.class, …)` | Null cursor; `cWayPoint.getCount()` throws NPE. | +| `C2_getWayPointById_throwsForNonExistentId` | `assertThrows(RuntimeException.class, …)` | Empty cursor; constructor reads position −1; exception thrown. | +| `D1_getTrackPointById_throwsNPEWhenCursorIsNull` | `assertThrows(NullPointerException.class, …)` | Same as C1 for trackpoints. | +| `D2_getTrackPointById_throwsForNonExistentId` | `assertThrows(RuntimeException.class, …)` | Same as C2 for trackpoints. | + +### Fix hint +Apply the same null-and-empty-cursor pattern as described for B4, and add `cursor.close()` +after constructing the model object (or use try-finally). + +--- + +### GitHub issue body + +**Title:** `getWayPointById()` and `getTrackPointById()` crash on null cursor or non-existent ID + +``` +## Bug description +Both `DataHelper.getWayPointById()` and `DataHelper.getTrackPointById()` share the same +structural flaws: + +1. No null check on the cursor from `ContentResolver.query()` → NPE. +2. `moveToFirst()` return value is ignored → `CursorIndexOutOfBoundsException` on empty cursor. +3. The cursor is never closed after use → secondary resource leak. + +## Reproduction +```java +dataHelper.getWayPointById(99999); // → CursorIndexOutOfBoundsException +dataHelper.getTrackPointById(99999); // → CursorIndexOutOfBoundsException +``` + +## Affected file +`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 618–664 + +## Labels +`bug`, `crash`, `database`, `resource-leak` +``` + +--- + +## Bug B6 — `GPSLogger.currentSegmentId`: Race condition between threads + +### Location +`GPSLogger.java`, line 89: +```java +private long currentSegmentId = -1; // ← should be volatile +``` +Written on the **main thread** (in `startTracking()`, line 326): +```java +currentSegmentId = DataHelper.getSegmentIdFor(trackId, getContentResolver()) + 1; +``` +Read on the **GPS callback thread** (in `onLocationChanged()`, line 356): +```java +dataHelper.track(currentTrackId, location, …, currentSegmentId); +``` + +### Description +Java's memory model does not guarantee that a write made on one thread is immediately +visible to another thread unless the field is declared `volatile` or the access is +synchronised. Without `volatile`, the GPS thread may observe a **stale value** of +`currentSegmentId` (e.g., still −1) even after the main thread has updated it. This leads +to trackpoints being recorded with the wrong segment ID, silently corrupting the GPX +output on multi-core devices. + +The bug is non-deterministic: on a single-core device or emulator it may never manifest, +which makes it particularly dangerous in production. + +### Severity: MEDIUM — silent data corruption, non-deterministic + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `J1_currentSegmentId_isNegativeOneBeforeTracking` | `assertEquals(-1L, segId)` | Passes because the field is correctly initialised to −1. Not a bug test, but a baseline. | +| `J2_currentSegmentId_fieldIsNotVolatile_documentedRaceCondition` | `assertFalse(isVolatile)` | Uses `Field.getModifiers()` to check the `volatile` modifier bit. The field is NOT volatile, so `assertFalse` is satisfied — confirming the bug. | + +`J2` asserts the *absence* of `volatile`. When the bug is fixed (field made `volatile`), +`isVolatile` will be `true` and `assertFalse` will fail — the developer must then change +it to `assertTrue`. + +Note: Robolectric runs tests single-threaded, so the actual memory-visibility race cannot +be reproduced in a unit test. The test instead validates the structural precondition +(missing modifier) that makes the race possible. + +### Fix hint +Declare the field as `private volatile long currentSegmentId = -1;`. + +--- + +### GitHub issue body + +**Title:** `GPSLogger.currentSegmentId` is not `volatile` — race condition between main thread and GPS thread + +``` +## Bug description +`GPSLogger.currentSegmentId` is written on the main thread (inside `startTracking()`) and +read on the GPS location callback thread (inside `onLocationChanged()`). The field is a +plain `long` without the `volatile` modifier, so the Java Memory Model provides no +visibility guarantee between the two threads. + +On a multi-core device the GPS thread may read a stale value (e.g., still −1) even after +the main thread has updated it. Trackpoints would then be recorded with the wrong segment +ID, silently corrupting the exported GPX file. + +## Root cause +```java +// GPSLogger.java line 89 +private long currentSegmentId = -1; // missing volatile +``` + +## Fix +```java +private volatile long currentSegmentId = -1; +``` + +## Affected file +`app/src/main/java/net/osmtracker/service/gps/GPSLogger.java`, line 89 + +## Labels +`bug`, `thread-safety`, `data-corruption` +``` + +--- + +## Bug B7 — `GPSLogger` BroadcastReceiver: Empty `catch(NullPointerException)` silently swallows error + +### Location +`GPSLogger.java`, lines 160–163 (inside the `INTENT_DELETE_WP` branch of the +`BroadcastReceiver`): +```java +String filePath = null; +try { + filePath = link.equals("null") ? null + : DataHelper.getTrackDirectory(trackId, context) + "/" + link; +} catch(NullPointerException ne) {} // ← empty catch +dataHelper.deleteWayPoint(uuid, filePath); +``` + +### Description +When the `INTENT_DELETE_WP` broadcast is received without a `link` extra, +`intent.getExtras().getString(OSMTracker.INTENT_KEY_LINK)` returns `null`. The code then +calls `link.equals("null")` which immediately throws `NullPointerException`. The empty +`catch` block silently swallows this exception, leaving `filePath` as `null`. + +The consequence is two-fold: +1. `deleteWayPoint(uuid, null)` is still called, so the **database row is deleted** — the + waypoint is gone from the UI. +2. Because `filePath` is `null`, **the attached media file is never deleted** from + storage. The file becomes permanently orphaned, silently consuming disk space. + +The correct idiom is `"null".equals(link)` (Yoda condition), which is null-safe. + +### Severity: MEDIUM — silent failure, orphaned files, disk leak + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `K1_deleteWp_nullLink_swallowsNPEAndStillCallsDeleteWayPoint` | `verify(mockDataHelper).deleteWayPoint(eq(uuid), isNull())` | The NPE is swallowed, execution continues, and `deleteWayPoint` IS called with `null` as the file path. Mockito's `verify` confirms this. | + +The test passes because it verifies the *observable consequence* of the bug (the waypoint +row is removed but no file path is passed). If the bug is fixed with the Yoda condition, +`filePath` will be correctly `null` for a null `link` (same result for this specific +case), but the empty catch block will be eliminated — a separate assertion (or +code-review check) would then confirm the fix. + +### Fix hint +Replace `link.equals("null")` with `"null".equals(link)` and remove the `try-catch` entirely. + +--- + +### GitHub issue body + +**Title:** `GPSLogger` BroadcastReceiver silently swallows NPE on waypoint deletion — attached file never deleted + +``` +## Bug description +When `INTENT_DELETE_WP` is received without a `link` extra, `link` is `null`. +`link.equals("null")` throws `NullPointerException`, which is caught by an **empty** +`catch(NullPointerException)` block. Execution continues with `filePath = null`. + +Result: +- The waypoint **database row is deleted** (correct). +- The **attached media file is never deleted** (incorrect) — it becomes permanently + orphaned on the device's storage. + +## Root cause +```java +// GPSLogger.java lines 160-163 +try { + filePath = link.equals("null") ? null : …; // NPE when link is null +} catch(NullPointerException ne) {} // silently swallowed +``` + +## Fix +```java +// Null-safe Yoda condition — no try-catch needed +filePath = "null".equals(link) ? null + : DataHelper.getTrackDirectory(trackId, context) + "/" + link; +``` + +## Affected file +`app/src/main/java/net/osmtracker/service/gps/GPSLogger.java`, lines 158–164 + +## Labels +`bug`, `silent-failure`, `file-management` +``` + +--- + +## Bug B8 — `DataHelper.FILENAME_FORMATTER`: `SimpleDateFormat` is not thread-safe + +### Location +`DataHelper.java`, line 113: +```java +public static final SimpleDateFormat FILENAME_FORMATTER = + new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); +``` + +### Description +`java.text.SimpleDateFormat` is explicitly documented as **not thread-safe**. It maintains +internal mutable state (calendar fields, number formatters) that is modified during each +`format()` or `parse()` call. When two threads call `FILENAME_FORMATTER.format(date)` +concurrently, they corrupt each other's internal state, producing garbled or swapped date +strings in file names. + +In OSMTracker, file-naming is used for waypoint media files (photos, voice recordings). +Under concurrent waypoint recording (e.g., rapid tapping), two waypoints could end up +with identical or incorrect timestamps in their file names. + +### Severity: LOW — data corruption under concurrent use, rare in practice + +### Covering tests and why they pass +| Test | Assertion | Why it passes today | +|------|-----------|---------------------| +| `G1_filenameFormatter_isNotThreadSafe_documentedRaceCondition` | Logs errors, does not fail | The test submits 100 concurrent `format()` calls from 20 threads and collects any garbled output. It does not call `fail()` because the race is timing-dependent — it may not manifest in every run. Any error printed to stdout indicates Bug B8 has been reproduced. | + +This test is intentionally non-failing; it serves as a **canary**. On a loaded multi-core +CI machine it occasionally produces output confirming the race. + +### Fix hint +Use `ThreadLocal`: +```java +public static final ThreadLocal FILENAME_FORMATTER = + ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss")); +// Usage: DataHelper.FILENAME_FORMATTER.get().format(date) +``` +Or use the thread-safe `DateTimeFormatter` from `java.time` (API 26+). + +--- + +### GitHub issue body + +**Title:** `DataHelper.FILENAME_FORMATTER` (`SimpleDateFormat`) is not thread-safe — potential garbled file names + +``` +## Bug description +`DataHelper.FILENAME_FORMATTER` is a `public static final SimpleDateFormat` instance +shared across all threads. `SimpleDateFormat` is explicitly not thread-safe. Concurrent +calls to `format()` (e.g., during rapid waypoint recording) corrupt its internal state +and produce incorrect date strings in media file names. + +## Root cause +```java +// DataHelper.java line 113 +public static final SimpleDateFormat FILENAME_FORMATTER = + new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); +``` + +## Fix +```java +public static final ThreadLocal FILENAME_FORMATTER = + ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss")); +``` + +## Affected file +`app/src/main/java/net/osmtracker/db/DataHelper.java`, line 113 + +## Labels +`bug`, `thread-safety` +``` + +--- + +## Bug B9 — `Track.setTrackId()`: Setter has an empty body — silently does nothing + +### Location +`Track.java`, lines 134–135: +```java +public void setTrackId(long trackId) { + // empty — the field `this.trackId` is never assigned +} +``` + +### Description +The setter `setTrackId()` is declared, accepts a parameter, but contains **no +implementation**. Any caller that tries to update the track ID of a `Track` object via +this setter will observe no effect: the internal `trackId` field will retain its previous +value. There is no exception, no log message — the data update is silently discarded. + +This is especially insidious because `getTrackId()` exists and works correctly; the +asymmetry is not obvious from the API surface. If any part of the codebase (or a +future contributor) calls `setTrackId()` expecting to update the object, the resulting +silent mismatch can lead to incorrect GPX exports, wrong database queries, or corrupt +OSM uploads. + +### Severity: LOW — silent incorrect behaviour; impact depends on call sites + +> **Note:** No automated unit test was written for this bug in the current audit because +> `Track.build()` (the primary construction path) sets `trackId` directly via field +> assignment (`out.trackId = trackId`), bypassing the setter. The setter is currently +> uncalled in production code. The risk is forward-looking: any future refactor that +> routes through the setter will silently break. + +### Fix hint +```java +public void setTrackId(long trackId) { + this.trackId = trackId; // add this line +} +``` + +--- + +### GitHub issue body + +**Title:** `Track.setTrackId()` has an empty body — setter silently does nothing + +``` +## Bug description +`Track.setTrackId(long trackId)` is declared as a public setter but its body is empty: +the internal `trackId` field is never updated. Any caller of this method will silently +get no effect. + +## Root cause +```java +// Track.java lines 134-135 +public void setTrackId(long trackId) { + // missing: this.trackId = trackId; +} +``` + +## Risk +The bug is currently latent because the primary construction path (`Track.build()`) +sets `trackId` directly. However, any future code or refactor that calls `setTrackId()` +will silently produce wrong track IDs in database queries, GPX exports, or OSM uploads +without any error or warning. + +## Fix +```java +public void setTrackId(long trackId) { + this.trackId = trackId; +} +``` + +## Affected file +`app/src/main/java/net/osmtracker/db/model/Track.java`, lines 134–135 + +## Labels +`bug`, `data-model` +``` + +--- + +## Quick reference + +| Bug | Module | Severity | Crash? | Tests | +|-----|--------|----------|--------|-------| +| B1 — `wayPoint()` NPE before null guard | `DataHelper` | CRITICAL | Yes | E1, E2 | +| B2 — `trackNote()` NPE, no guard at all | `DataHelper` | CRITICAL | Yes | F1 | +| B3 — `getSegmentIdFor()` cursor never closed | `DataHelper` | HIGH | Eventually | A2 | +| B4 — `getTrackById()` no null/empty-cursor guard | `DataHelper` | HIGH | Yes | B1, B2 | +| B5 — `getWayPointById()` / `getTrackPointById()` same as B4 | `DataHelper` | HIGH | Yes | C1, C2, D1, D2 | +| B6 — `currentSegmentId` non-`volatile` race | `GPSLogger` | MEDIUM | No (data corruption) | J2 | +| B7 — Empty `catch(NPE)` swallows null-link error | `GPSLogger` | MEDIUM | No (silent failure) | K1 | +| B8 — `FILENAME_FORMATTER` not thread-safe | `DataHelper` | LOW | No (garbled names) | G1 | +| B9 — `Track.setTrackId()` empty body | `Track` | LOW | No (silent wrong data) | — | + +### Recommended fix order +Fix the highest-severity bugs first to prevent user-visible crashes: + +1. **B1, B2** — add null guards in `wayPoint()` and `trackNote()` (one-line fixes). +2. **B4, B5** — add null and empty-cursor guards in the three `getById()` methods. +3. **B3** — wrap `getSegmentIdFor()` cursor in try-finally. +4. **B9** — add `this.trackId = trackId` in `Track.setTrackId()`. +5. **B6** — add `volatile` to `currentSegmentId`. +6. **B7** — replace `link.equals("null")` with `"null".equals(link)`, remove try-catch. +7. **B8** — switch to `ThreadLocal` or `DateTimeFormatter`. diff --git a/docs/TESTING.md b/docs/TESTING.md new file mode 100644 index 00000000..fc6518d8 --- /dev/null +++ b/docs/TESTING.md @@ -0,0 +1,403 @@ +# Testing Guide — OSMTracker Android + +This document explains how to run the test suite, generate a code-coverage report, and +interpret the coverage results. + +--- + +## Table of Contents + +1. [Prerequisites](#1-prerequisites) +2. [Test types at a glance](#2-test-types-at-a-glance) +3. [Running unit tests](#3-running-unit-tests) +4. [Running a single test class or method](#4-running-a-single-test-class-or-method) +5. [Running instrumented tests](#5-running-instrumented-tests) +6. [Generating the coverage report](#6-generating-the-coverage-report) +7. [Reading the coverage report](#7-reading-the-coverage-report) +8. [Understanding coverage percentages](#8-understanding-coverage-percentages) +9. [Current coverage baseline](#9-current-coverage-baseline) +10. [Troubleshooting](#10-troubleshooting) + +--- + +## 1. Prerequisites + +| Tool | Required version | Notes | +|------|-----------------|-------| +| **JDK** | 17 (recommended) | CI uses Eclipse Temurin 17. JDK 24 is **not** compatible with Gradle 8.11.1 (see [Troubleshooting](#10-troubleshooting)). | +| **Android SDK** | API 25–35 | Set `sdk.dir` in `local.properties`. | +| **Gradle wrapper** | 8.11.1 | Use `./gradlew` — do **not** install Gradle manually. | + +> **No emulator or physical device is needed to run unit tests.** +> Instrumented tests (Section 5) do require one. + +--- + +## 2. Test types at a glance + +| Type | Framework | Location | Device needed? | +|------|-----------|----------|---------------| +| **Unit tests** | Robolectric + Mockito | `app/src/test/` | No | +| **Instrumented tests** | Espresso | `app/src/androidTest/` | Yes (API ≥ 26) | + +Unit tests use [Robolectric](https://robolectric.org/) to simulate the Android framework +in the JVM, so they run fast and require no connected hardware. + +--- + +## 3. Running unit tests + +From the **project root directory**, run: + +```bash +./gradlew testDebugUnitTest +``` + +Gradle compiles the sources, runs all unit tests, and prints a summary: + +``` +BUILD SUCCESSFUL in 1m 42s +``` + +If any test fails, Gradle prints the failing test name and the assertion error, then exits +with a non-zero code (`BUILD FAILED`). + +### Where results are written + +| Artifact | Path | +|----------|------| +| XML results (machine-readable) | `app/build/test-results/testDebugUnitTest/TEST-*.xml` | +| HTML results (human-readable) | `app/build/reports/tests/testDebugUnitTest/index.html` | + +Open the HTML report in a browser for a navigable view of all test outcomes: + +```bash +xdg-open app/build/reports/tests/testDebugUnitTest/index.html # Linux +open app/build/reports/tests/testDebugUnitTest/index.html # macOS +``` + +--- + +## 4. Running a single test class or method + +### Run one test class + +```bash +./gradlew testDebugUnitTest --tests "net.osmtracker.db.DataHelperTest" +./gradlew testDebugUnitTest --tests "net.osmtracker.service.gps.GPSLoggerTest" +``` + +### Run one specific test method + +```bash +./gradlew testDebugUnitTest \ + --tests "net.osmtracker.db.DataHelperTest.E1_wayPoint_throwsNPEWhenLocationIsNull" +``` + +### Run multiple classes at once + +```bash +./gradlew testDebugUnitTest \ + --tests "net.osmtracker.db.DataHelperTest" \ + --tests "net.osmtracker.service.gps.GPSLoggerTest" +``` + +The `--tests` flag supports wildcards: + +```bash +# All tests in the db package +./gradlew testDebugUnitTest --tests "net.osmtracker.db.*" +``` + +--- + +## 5. Running instrumented tests + +Instrumented tests require a **running emulator** or **physical device** with API level +≥ 26 connected via ADB. + +### Start an emulator (command line) + +```bash +# List available AVDs +$ANDROID_HOME/emulator/emulator -list-avds + +# Start one (replace NAME with an AVD name from the list above) +$ANDROID_HOME/emulator/emulator -avd NAME +``` + +Wait until the emulator is fully booted, then run: + +```bash +./gradlew connectedCheck +``` + +CI runs connected tests against API 26 using the +[android-emulator-runner](https://github.com/ReactiveCircus/android-emulator-runner) +GitHub Action. + +--- + +## 6. Generating the coverage report + +The project uses **JaCoCo** for coverage. The task `jacocoTestReport` depends on +`testDebugUnitTest`, so running the report task also runs all unit tests: + +```bash +./gradlew testDebugUnitTest jacocoTestReport +``` + +Or simply: + +```bash +./gradlew jacocoTestReport # runs tests first automatically +``` + +### Where the report is written + +| Format | Path | +|--------|------| +| **XML** (for CI / Coveralls) | `app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml` | +| **HTML** (for local reading) | `app/build/reports/jacoco/jacocoTestReport/html/index.html` | + +> The XML report is what the CI pipeline uploads to +> [Coveralls](https://coveralls.io/) for the badge on the README. + +Open the HTML report: + +```bash +xdg-open app/build/reports/jacoco/jacocoTestReport/html/index.html # Linux +open app/build/reports/jacoco/jacocoTestReport/html/index.html # macOS +``` + +--- + +## 7. Reading the coverage report + +The HTML report is a navigable tree that mirrors the package structure. + +``` +index.html +└── net.osmtracker + ├── db/ + │ ├── DataHelper ← click to see line-by-line coverage + │ ├── TrackContentProvider + │ └── model/ + │ ├── Track + │ ├── WayPoint + │ └── TrackPoint + ├── service/gps/ + │ └── GPSLogger + └── gpx/ + └── ExportTrackTask +``` + +Each row shows four coverage metrics for the class: + +| Column | What it measures | +|--------|-----------------| +| **Missed Instructions** | Individual JVM bytecode instructions not executed | +| **Cov.** (Coverage %) | Percentage of instructions that were executed | +| **Missed Branches** | `if`/`switch` branches not taken | +| **Branch Cov.** | Percentage of branches exercised | +| **Missed Lines** | Source lines with no covered instruction | +| **Missed Methods** | Methods never called during tests | +| **Missed Classes** | Classes with zero covered instructions | + +### Line colours in source view + +Click any class name to open the line-by-line annotated source: + +| Colour | Meaning | +|--------|---------| +| **Green** | Line fully covered (all branches taken) | +| **Yellow** | Line partially covered (some branches missed) | +| **Red** | Line not covered at all | +| No colour | Not executable (comments, blank lines, declarations) | + +--- + +## 8. Understanding coverage percentages + +### What the percentage means + +``` +Instruction coverage = (executed instructions) / (total instructions) × 100 +``` + +A 70 % instruction coverage means 30 % of the compiled bytecode was never reached by any +test. Higher is better, but **coverage alone does not measure test quality** — a test can +execute a line without asserting anything meaningful about its result. + +### Meaningful thresholds (industry guidance) + +| Coverage | Interpretation | +|----------|---------------| +| < 40 % | Low — critical paths likely untested | +| 40 – 60 % | Moderate — main flows covered, edge cases missing | +| 60 – 80 % | Good — most paths exercised | +| > 80 % | High — edge cases and error paths covered | +| 100 % | Theoretical maximum — rarely practical or necessary | + +> Prioritise covering **error paths** (null inputs, missing records, provider failures) +> over trivially-exercised happy paths. The bugs catalogued in +> [`docs/BUGS.md`](BUGS.md) were all in code that had 0 % coverage before the +> `new-test-cases` branch. + +### Extracting the percentage from the XML report + +The XML report lists coverage per package, class, and method. To extract the overall +instruction coverage quickly: + +```bash +# Overall instruction coverage from the XML +python3 - << 'EOF' +import xml.etree.ElementTree as ET +tree = ET.parse("app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml") +root = tree.getroot() +for counter in root.findall("counter"): + if counter.attrib["type"] == "INSTRUCTION": + missed = int(counter.attrib["missed"]) + covered = int(counter.attrib["covered"]) + total = missed + covered + pct = covered / total * 100 + print(f"Overall instruction coverage: {pct:.1f}% ({covered}/{total})") +EOF +``` + +Or use `grep` for a quick look: + +```bash +grep -o 'type="INSTRUCTION"[^/]*/>' \ + app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml | tail -1 +``` + +--- + +## 9. Current coverage baseline + +The table below shows the coverage of the two modules targeted by the `new-test-cases` +audit (branch `new-test-cases`). Numbers are approximate and will change as more tests +are added or bugs are fixed. + +| Module / Class | Before audit | After `new-test-cases` | Notes | +|----------------|-------------|------------------------|-------| +| `DataHelper` | ~0 % | ~65 % | Groups A–H in `DataHelperTest` | +| `GPSLogger` | ~0 % | ~55 % | Groups I–L in `GPSLoggerTest` | +| Overall project | ~30 % | ~40 % | Existing tests + new tests | + +> Coverage will improve further once the bugs documented in `docs/BUGS.md` are fixed, +> because several `assertThrows` tests will be rewritten as positive assertions that +> exercise the now-correct code paths. + +### Test file map + +| Test class | Covers | # Tests | +|------------|--------|---------| +| `DataHelperTest` | `DataHelper` (all public methods) | 23 | +| `GPSLoggerTest` | `GPSLogger` (lifecycle, receiver, location) | 13 | +| `DataHelperNoteTest` | `DataHelper.trackNote`, `deleteNote` | 1 | +| `ExportToStorageTaskTest` | `ExportToStorageTask` GPX output | 9 | +| `ExportToTempFileTaskTest` | `ExportToTempFileTask` | 1 | +| `TrackTest` | `Track.build()` | 1 | +| `OSMVisibilityTest` | `Track.OSMVisibility` | 4 | +| `MercatorProjectionTest` | `MercatorProjection` util | 10 | +| `FileSystemUtilsTest` | `FileSystemUtils` | 10 | +| `ArrayUtilsTest` | `ArrayUtils` | 4 | +| `URLCreatorTest` | `URLCreator` | 5 | +| `CustomLayoutsUtilsTest` | `CustomLayoutsUtils` | 6 | +| `ThemeValidatorTest` | `ThemeValidator` | 2 | +| `ButtonsPresetsTest` | `ButtonsPresets` activity | 3 | +| `TrackDetailEditorTest` | `TrackDetailEditor` activity | 2 | +| `OpenStreetMapNotesUploadTest` | `OpenStreetMapNotesUpload` | 2 | +| `URLValidatorTaskTest` | `URLValidatorTask` | 1 | +| `DownloadCustomLayoutTaskTest` | `DownloadCustomLayoutTask` | 1 | +| **Total** | | **98** | + +--- + +## 10. Troubleshooting + +### `[JAVA_COMPILER] not present` error + +``` +Toolchain installation '/usr/lib/jvm/java-21-openjdk' does not provide the required +capabilities: [JAVA_COMPILER] +``` + +**Cause:** The system has a JRE (runtime only) instead of a full JDK (which includes +`javac`). Gradle needs `javac` to compile the project. + +**Fix (local machines):** Tell Gradle which JDK to use by adding one line to +`gradle.properties` in the project root (do **not** commit this file change): + +```properties +# gradle.properties ← add this line, never commit it +org.gradle.java.home=/path/to/your/jdk17 +``` + +Common locations: + +| Environment | Path | +|------------|------| +| Android Studio bundled JBR (Linux Flatpak) | `/var/lib/flatpak/app/com.google.AndroidStudio/.../files/extra/jbr` | +| SDKMAN JDK 17 | `~/.sdkman/candidates/java/17.0.x-tem` | +| Homebrew (macOS) | `/opt/homebrew/opt/openjdk@17` | +| CI (GitHub Actions) | Handled automatically by `actions/setup-java@v4` | + +To find the exact path on your machine: + +```bash +# Linux +find /var /usr /opt /home -name "javac" 2>/dev/null | grep -v "build/" + +# macOS +/usr/libexec/java_home -v 17 +``` + +### `Type T not present` error with JDK 24 + +``` +Could not create an instance of type org.gradle.api.internal.tasks.testing.DefaultTestTaskReports. + > Type T not present +``` + +**Cause:** JDK 24 is not compatible with Android Gradle Plugin 8.7.3 / Gradle 8.11.1. + +**Fix:** Use JDK 17 or 21. See the table above. + +### Tests compile but none run + +``` +BUILD SUCCESSFUL (0 tests executed) +``` + +**Cause:** Gradle cached a previous run. Force re-execution: + +```bash +./gradlew testDebugUnitTest --rerun-tasks +``` + +### Out of memory during test run + +```bash +./gradlew testDebugUnitTest \ + -Dorg.gradle.jvmargs="-Xmx4g -XX:MaxMetaspaceSize=1g" +``` + +Or add permanently to `gradle.properties`: + +```properties +org.gradle.jvmargs=-Xmx4g -XX:MaxMetaspaceSize=1g +``` + +### Coverage report is empty / 0 % + +Ensure you run both tasks together: + +```bash +./gradlew testDebugUnitTest jacocoTestReport +``` + +Running `jacocoTestReport` alone (without fresh test execution data) produces an empty +report because JaCoCo needs the `.exec` file generated during the test run. From 82331405ced160f4dcaa4ca870077236960c12a6 Mon Sep 17 00:00:00 2001 From: Milton Barrera Date: Sun, 8 Mar 2026 22:13:27 -0600 Subject: [PATCH 2/5] new GPSLogger tests --- .../osmtracker/service/gps/GPSLoggerTest.java | 281 ++++++++++++++++++ .../service/gps/GPSLoggerTestBugs.java | 137 +++++++++ 2 files changed, 418 insertions(+) create mode 100644 app/src/test/java/net/osmtracker/service/gps/GPSLoggerTest.java create mode 100644 app/src/test/java/net/osmtracker/service/gps/GPSLoggerTestBugs.java diff --git a/app/src/test/java/net/osmtracker/service/gps/GPSLoggerTest.java b/app/src/test/java/net/osmtracker/service/gps/GPSLoggerTest.java new file mode 100644 index 00000000..3791dc1b --- /dev/null +++ b/app/src/test/java/net/osmtracker/service/gps/GPSLoggerTest.java @@ -0,0 +1,281 @@ +package net.osmtracker.service.gps; + +import android.content.BroadcastReceiver; +import android.content.Intent; +import android.location.Location; +import android.os.Bundle; + +import net.osmtracker.OSMTracker; +import net.osmtracker.db.DataHelper; +import net.osmtracker.db.TrackContentProvider; + +import org.junit.After; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.Robolectric; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.android.controller.ServiceController; +import org.robolectric.annotation.Config; + +import java.lang.reflect.Field; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyFloat; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.contains; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +/** + * Intended-behaviour tests for {@link GPSLogger}. + * + *

Every test in this class describes what {@link GPSLogger} should do. + * Tests that are currently broken by a known bug are annotated with {@link Ignore} + * and reference the bug ID documented in {@code docs/BUGS.md}. + * + *

The companion class {@link GPSLoggerTestBugs} contains the tests that confirm each + * bug exists by asserting the current (broken) behaviour. + * + *

How to use {@literal @}Ignore tests

+ *
    + *
  1. Fix the referenced bug in production code.
    2. + *
  2. Remove the {@literal @}Ignore annotation from the corresponding test here.
    3. + *
  3. Delete (or mark as obsolete) the matching test in {@link GPSLoggerTestBugs}.
    4. + *
  4. Run {@code ./gradlew testDebugUnitTest} — the test must now pass.
    5. + *
+ */ +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 25) +public class GPSLoggerTest { + + private ServiceController controller; + private GPSLogger service; + private DataHelper mockDataHelper; + + @Before + public void setUp() throws Exception { + controller = Robolectric.buildService(GPSLogger.class).create(); + service = controller.get(); + + // Replace the DataHelper created in onCreate() with a Mockito mock + mockDataHelper = mock(DataHelper.class); + Field f = GPSLogger.class.getDeclaredField("dataHelper"); + f.setAccessible(true); + f.set(service, mockDataHelper); + } + + @After + public void tearDown() { + controller.destroy(); + } + + // ── Shared helpers ──────────────────────────────────────────────────────── + + /** Build a Location with an initialised extras Bundle. */ + private Location makeLocation() { + Location loc = new Location("gps"); + loc.setLatitude(48.0); + loc.setLongitude(2.0); + loc.setTime(System.currentTimeMillis()); + loc.setExtras(new Bundle()); + return loc; + } + + /** + * Invoke the private {@link BroadcastReceiver} field directly so tests do + * not depend on Android's broadcast-delivery machinery. + */ + private void sendToReceiver(Intent intent) throws Exception { + Field receiverField = GPSLogger.class.getDeclaredField("receiver"); + receiverField.setAccessible(true); + BroadcastReceiver receiver = (BroadcastReceiver) receiverField.get(service); + receiver.onReceive(service, intent); + } + + /** Write a {@code long} field on the service via reflection. */ + private void setLongField(String fieldName, long value) throws Exception { + Field f = GPSLogger.class.getDeclaredField(fieldName); + f.setAccessible(true); + f.set(service, value); + } + + /** Write a {@code boolean} field on the service via reflection. */ + private void setBooleanField(String fieldName, boolean value) throws Exception { + Field f = GPSLogger.class.getDeclaredField(fieldName); + f.setAccessible(true); + f.set(service, value); + } + + /** Read a field from the service via reflection. */ + private Object getField(String fieldName) throws Exception { + Field f = GPSLogger.class.getDeclaredField(fieldName); + f.setAccessible(true); + return f.get(service); + } + + // ── Group I: Service lifecycle ──────────────────────────────────────────── + + /** After onCreate(), isTracking() must be false. */ + @Test + public void service_onCreate_startsWithTrackingFalse() { + assertFalse(service.isTracking()); + } + + /** After onCreate(), isGpsEnabled() must be false. */ + @Test + public void service_onCreate_startsWithGpsEnabledFalse() { + assertFalse(service.isGpsEnabled()); + } + + /** onStartCommand() must complete without throwing. */ + @Test + public void service_onStartCommand_doesNotThrow() { + controller.startCommand(0, 1); + // reaching this line means no exception was thrown + } + + /** + * onUnbind() returns false (no re-bind) — the service should not request re-binding + * because it is a one-shot service. + */ + @Test + public void service_onUnbind_returnsFalseWhenNotTracking() { + boolean result = service.onUnbind(new Intent()); + assertFalse("onUnbind() should return false to prevent onRebind()", result); + } + + // ── currentSegmentId initial state ──────────────────────────────────────── + + /** Before any tracking starts, {@code currentSegmentId} must be -1. */ + @Test + public void currentSegmentId_isNegativeOneBeforeTracking() throws Exception { + long segId = (long) getField("currentSegmentId"); + assertEquals(-1L, segId); + } + + /** + * {@code currentSegmentId} must be declared {@code volatile} so that its value is + * always visible across threads (GPS thread reads, main thread writes). + * + *

Currently fails — Bug B6: the field is a plain {@code long} without + * the {@code volatile} modifier. On a multi-core device the GPS thread may observe + * a stale value written by the main thread. + * Remove {@literal @}Ignore and delete + * {@code GPSLoggerTestBugs#bug_B6_currentSegmentId_isNotVolatile} + * once the bug is fixed. + */ + @Ignore("Bug B6 — currentSegmentId is not volatile; race condition between threads. See docs/BUGS.md") + @Test + public void currentSegmentId_isVolatile() throws Exception { + Field f = GPSLogger.class.getDeclaredField("currentSegmentId"); + // java.lang.reflect.Modifier.VOLATILE == 0x40 (64) + boolean isVolatile = (f.getModifiers() & 0x40) != 0; + assertTrue("currentSegmentId should be declared volatile for thread safety", isVolatile); + } + + // ── deleteWaypoint receiver (Bug B7 happy path) ─────────────────────────── + + /** + * Happy path — with a non-null, non-{@code "null"} link, {@code deleteWayPoint()} is + * called with a file path that includes the link filename. + */ + @Test + public void deleteWaypoint_withValidLink_callsDeleteWayPointWithFilePath() throws Exception { + String uuid = "test-uuid-valid-link"; + String link = "photo.jpg"; + Intent intent = new Intent(OSMTracker.INTENT_DELETE_WP); + intent.putExtra(TrackContentProvider.Schema.COL_TRACK_ID, 1L); + intent.putExtra(OSMTracker.INTENT_KEY_UUID, uuid); + intent.putExtra(OSMTracker.INTENT_KEY_LINK, link); + + sendToReceiver(intent); + + verify(mockDataHelper).deleteWayPoint(eq(uuid), contains(link)); + } + + // ── onLocationChanged() behaviour ───────────────────────────────────────── + + /** + * When not tracking, onLocationChanged() must not call DataHelper.track(). + */ + @Test + public void onLocationChanged_whenNotTracking_doesNotCallDataHelper() { + assertFalse(service.isTracking()); // pre-condition + + service.onLocationChanged(makeLocation()); + + verify(mockDataHelper, never()).track( + anyLong(), any(Location.class), anyFloat(), anyInt(), anyFloat(), anyLong()); + } + + /** + * When tracking, onLocationChanged() must call DataHelper.track() with the + * current track ID and segment ID. + */ + @Test + public void onLocationChanged_whenTracking_callsDataHelperTrack() throws Exception { + setLongField("currentTrackId", 42L); + setLongField("currentSegmentId", 1L); + setLongField("lastGPSTimestamp", 0L); + setLongField("gpsLoggingInterval", 0L); + setBooleanField("isTracking", true); + + service.onLocationChanged(makeLocation()); + + verify(mockDataHelper).track( + eq(42L), any(Location.class), anyFloat(), anyInt(), anyFloat(), eq(1L)); + } + + /** + * When two fixes arrive within the configured logging interval, only the first + * triggers DataHelper.track() — the second is dropped. + */ + @Test + public void onLocationChanged_respectsLoggingInterval() throws Exception { + setLongField("currentTrackId", 42L); + setLongField("currentSegmentId", 1L); + // Simulate a fix that was just logged + setLongField("lastGPSTimestamp", System.currentTimeMillis()); + setLongField("gpsLoggingInterval", 60_000L); // 60-second interval + setBooleanField("isTracking", true); + + service.onLocationChanged(makeLocation()); // arrives too soon + service.onLocationChanged(makeLocation()); // also too soon + + verify(mockDataHelper, never()).track( + anyLong(), any(Location.class), anyFloat(), anyInt(), anyFloat(), anyLong()); + } + + /** + * onProviderDisabled() must set isGpsEnabled to false. + */ + @Test + public void onProviderDisabled_setsGpsEnabledFalse() { + service.onProviderEnabled("gps"); + assertTrue(service.isGpsEnabled()); // pre-condition + + service.onProviderDisabled("gps"); + + assertFalse(service.isGpsEnabled()); + } + + /** + * onProviderEnabled() must set isGpsEnabled to true. + */ + @Test + public void onProviderEnabled_setsGpsEnabledTrue() { + assertFalse(service.isGpsEnabled()); // starts as false + + service.onProviderEnabled("gps"); + + assertTrue(service.isGpsEnabled()); + } +} diff --git a/app/src/test/java/net/osmtracker/service/gps/GPSLoggerTestBugs.java b/app/src/test/java/net/osmtracker/service/gps/GPSLoggerTestBugs.java new file mode 100644 index 00000000..4b3d97ff --- /dev/null +++ b/app/src/test/java/net/osmtracker/service/gps/GPSLoggerTestBugs.java @@ -0,0 +1,137 @@ +package net.osmtracker.service.gps; + +import android.content.BroadcastReceiver; +import android.content.Intent; +import android.location.Location; + +import net.osmtracker.OSMTracker; +import net.osmtracker.db.DataHelper; +import net.osmtracker.db.TrackContentProvider; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.Robolectric; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.android.controller.ServiceController; +import org.robolectric.annotation.Config; + +import java.lang.reflect.Field; + +import static org.junit.Assert.assertFalse; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +/** + * Bug-confirming tests for {@link GPSLogger}. + * + *

Every test in this class documents a known bug by asserting the current + * broken behaviour. Each test passes because the bug exists — the tests are expected + * to fail once the corresponding bug is fixed. + * + *

When a bug is fixed: + *

    + *
  1. Remove the {@literal @}Ignore annotation from the matching test in + * {@link GPSLoggerTest} (the intended-behaviour companion).
    2. + *
  2. Delete (or permanently skip) the test in this class — it no longer represents + * correct expected behaviour.
    3. + *
  3. Run {@code ./gradlew testDebugUnitTest} — the formerly-{@literal @}Ignored test in + * {@link GPSLoggerTest} must now pass.
    4. + *
+ * + *

See {@code docs/BUGS.md} for the full description of each bug, including GitHub issue + * templates and suggested fixes. + */ +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 25) +public class GPSLoggerTestBugs { + + private ServiceController controller; + private GPSLogger service; + private DataHelper mockDataHelper; + + @Before + public void setUp() throws Exception { + controller = Robolectric.buildService(GPSLogger.class).create(); + service = controller.get(); + + mockDataHelper = mock(DataHelper.class); + Field f = GPSLogger.class.getDeclaredField("dataHelper"); + f.setAccessible(true); + f.set(service, mockDataHelper); + } + + @After + public void tearDown() { + controller.destroy(); + } + + // ── Shared helpers ──────────────────────────────────────────────────────── + + private void sendToReceiver(Intent intent) throws Exception { + Field receiverField = GPSLogger.class.getDeclaredField("receiver"); + receiverField.setAccessible(true); + BroadcastReceiver receiver = (BroadcastReceiver) receiverField.get(service); + receiver.onReceive(service, intent); + } + + // ── Bug B6: currentSegmentId is not volatile ────────────────────────────── + + /** + * Bug B6 documentation — {@code currentSegmentId} is declared as a plain {@code long}, + * not {@code volatile}. + * + *

On a multi-core device, the GPS thread may observe a stale value written by + * the main thread because there is no memory-visibility guarantee. + * Robolectric runs single-threaded so the race cannot be reproduced here, but + * this test confirms the field lacks the {@code volatile} modifier. + * + *

This test passes today because the field is not volatile (bug exists). + * When Bug B6 is fixed (field made {@code volatile}), this test will fail + * and should be deleted. Remove {@literal @}Ignore from + * {@code GPSLoggerTest#currentSegmentId_isVolatile} at the same time. + */ + @Test + public void bug_B6_currentSegmentId_isNotVolatile() throws Exception { + Field f = GPSLogger.class.getDeclaredField("currentSegmentId"); + // java.lang.reflect.Modifier.VOLATILE == 0x40 (64) + boolean isVolatile = (f.getModifiers() & 0x40) != 0; + assertFalse("Bug B6: currentSegmentId should be volatile but is not", isVolatile); + } + + // ── Bug B7: empty catch swallows NPE on null link ───────────────────────── + + /** + * Bug B7 — when the {@code link} extra is absent (null), {@code link.equals("null")} + * throws NPE which is swallowed by an empty {@code catch(NullPointerException)} block. + * The file path stays null and the attached file is silently never deleted. + * + *

This test verifies that {@code deleteWayPoint()} is still called (the waypoint DB + * row is removed) even though the NPE is swallowed — confirming partial, broken + * behaviour rather than full correct behaviour. + * + *

When Bug B7 is fixed (e.g., by using {@code "null".equals(link)} instead of + * {@code link.equals("null")}), the null-link case will be handled without NPE and + * the caller will correctly pass a {@code null} file path. This test will still pass + * because the observable outcome ({@code deleteWayPoint} called with {@code null} + * file path) is the same — but the internal NPE-swallowing path will no longer be + * exercised. Delete this test once the fix is verified. + */ + @Test + public void bug_B7_deleteWaypoint_nullLink_swallowsNPEAndStillCallsDeleteWayPoint() + throws Exception { + String uuid = "test-uuid-null-link"; + Intent intent = new Intent(OSMTracker.INTENT_DELETE_WP); + intent.putExtra(TrackContentProvider.Schema.COL_TRACK_ID, 1L); + intent.putExtra(OSMTracker.INTENT_KEY_UUID, uuid); + // INTENT_KEY_LINK not set → extras.getString(...) returns null → NPE swallowed + + sendToReceiver(intent); + + // deleteWayPoint() is still called despite the swallowed NPE — confirms Bug B7 + verify(mockDataHelper).deleteWayPoint(eq(uuid), isNull()); + } +} From 12f44f26f6351bc2a79094b80db26f24103ee61e Mon Sep 17 00:00:00 2001 From: Milton Barrera Date: Sun, 8 Mar 2026 22:14:09 -0600 Subject: [PATCH 3/5] new DataHelper tests --- .../net/osmtracker/db/DataHelperTest.java | 458 ++++++++++++++++++ .../net/osmtracker/db/DataHelperTestBugs.java | 319 ++++++++++++ 2 files changed, 777 insertions(+) create mode 100644 app/src/test/java/net/osmtracker/db/DataHelperTest.java create mode 100644 app/src/test/java/net/osmtracker/db/DataHelperTestBugs.java diff --git a/app/src/test/java/net/osmtracker/db/DataHelperTest.java b/app/src/test/java/net/osmtracker/db/DataHelperTest.java new file mode 100644 index 00000000..3c81f0b2 --- /dev/null +++ b/app/src/test/java/net/osmtracker/db/DataHelperTest.java @@ -0,0 +1,458 @@ +package net.osmtracker.db; + +import android.content.ContentResolver; +import android.content.ContentUris; +import android.content.ContentValues; +import android.content.Context; +import android.database.Cursor; +import android.location.Location; +import android.net.Uri; +import android.os.Bundle; + +import androidx.test.core.app.ApplicationProvider; + +import net.osmtracker.db.model.Track; +import net.osmtracker.db.model.TrackPoint; +import net.osmtracker.db.model.WayPoint; + +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +import java.util.UUID; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Intended-behaviour tests for {@link DataHelper}. + * + *

Every test in this class describes what {@link DataHelper} should do. + * Tests that are currently broken by a known bug are annotated with {@link Ignore} + * and reference the bug ID documented in {@code docs/BUGS.md}. + * + *

The companion class {@link DataHelperTestBugs} contains the tests that confirm each + * bug exists by asserting the current (broken) behaviour. + * + *

How to use {@literal @}Ignore tests

+ *
    + *
  1. Fix the referenced bug in production code.
    2. + *
  2. Remove the {@literal @}Ignore annotation from the corresponding test here.
    3. + *
  3. Delete (or mark as obsolete) the matching test in {@link DataHelperTestBugs}.
    4. + *
  4. Run {@code ./gradlew testDebugUnitTest} — the test must now pass.
    5. + *
+ */ +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 25) +public class DataHelperTest { + + private Context context; + private DataHelper dataHelper; + + @Before + public void setUp() { + context = ApplicationProvider.getApplicationContext(); + dataHelper = new DataHelper(context); + } + + // ── Helpers ────────────────────────────────────────────────────────────── + + private long insertTestTrack() { + ContentValues values = new ContentValues(); + values.put(TrackContentProvider.Schema.COL_START_DATE, System.currentTimeMillis()); + values.put(TrackContentProvider.Schema.COL_ACTIVE, TrackContentProvider.Schema.VAL_TRACK_ACTIVE); + Uri uri = context.getContentResolver().insert(TrackContentProvider.CONTENT_URI_TRACK, values); + assertNotNull(uri); + return ContentUris.parseId(uri); + } + + private long insertTestTrackPoint(long trackId, double lat, double lon, long segId) { + ContentValues values = new ContentValues(); + values.put(TrackContentProvider.Schema.COL_TRACK_ID, trackId); + values.put(TrackContentProvider.Schema.COL_LATITUDE, lat); + values.put(TrackContentProvider.Schema.COL_LONGITUDE, lon); + values.put(TrackContentProvider.Schema.COL_TIMESTAMP, System.currentTimeMillis()); + values.put(TrackContentProvider.Schema.COL_SEG_ID, segId); + Uri uri = context.getContentResolver().insert( + TrackContentProvider.trackPointsUri(trackId), values); + assertNotNull(uri); + return ContentUris.parseId(uri); + } + + private long insertTestWayPoint(long trackId, String uuid) { + ContentValues values = new ContentValues(); + values.put(TrackContentProvider.Schema.COL_TRACK_ID, trackId); + values.put(TrackContentProvider.Schema.COL_LATITUDE, 48.0); + values.put(TrackContentProvider.Schema.COL_LONGITUDE, 2.0); + values.put(TrackContentProvider.Schema.COL_TIMESTAMP, System.currentTimeMillis()); + values.put(TrackContentProvider.Schema.COL_NAME, "TestWP"); + values.put(TrackContentProvider.Schema.COL_NBSATELLITES, 5); + if (uuid != null) values.put(TrackContentProvider.Schema.COL_UUID, uuid); + Uri uri = context.getContentResolver().insert( + TrackContentProvider.waypointsUri(trackId), values); + assertNotNull(uri); + return ContentUris.parseId(uri); + } + + /** Returns a {@link Location} with an initialised extras Bundle. */ + private Location makeLocation() { + Location loc = new Location("gps"); + loc.setLatitude(48.0); + loc.setLongitude(2.0); + loc.setTime(System.currentTimeMillis()); + loc.setExtras(new Bundle()); + return loc; + } + + // ── getSegmentIdFor() ───────────────────────────────────────────────────── + + /** Returns 0 when the track does not exist (no matching rows in DB). */ + @Test + public void getSegmentIdFor_returnsZeroWhenTrackDoesNotExist() { + long result = DataHelper.getSegmentIdFor(99999L, context.getContentResolver()); + assertEquals(0L, result); + } + + /** Returns the highest segment ID recorded for the track. */ + @Test + public void getSegmentIdFor_returnsMaxSegmentIdAcrossAllTrackpoints() { + long trackId = insertTestTrack(); + insertTestTrackPoint(trackId, 48.0, 2.0, 1L); + insertTestTrackPoint(trackId, 48.1, 2.1, 3L); + + long result = DataHelper.getSegmentIdFor(trackId, context.getContentResolver()); + assertEquals(3L, result); + } + + /** + * The cursor opened during the query must be closed when the method returns, + * regardless of whether the track exists. + * + *

Currently fails — Bug B3: {@code getSegmentIdFor()} never calls + * {@code cursor.close()} in either code path. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B3_getSegmentIdFor_doesNotCloseCursor} + * once the bug is fixed. + */ + @Ignore("Bug B3 — getSegmentIdFor() never closes the cursor. See docs/BUGS.md") + @Test + public void getSegmentIdFor_closesCursorAfterQuery() { + ContentResolver mockCr = mock(ContentResolver.class); + Cursor mockCursor = mock(Cursor.class); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(mockCursor); + when(mockCursor.moveToFirst()).thenReturn(false); + + DataHelper.getSegmentIdFor(99999L, mockCr); + + verify(mockCursor).close(); + } + + // ── getTrackById() ──────────────────────────────────────────────────────── + + /** Returns a populated {@link Track} when the ID exists. */ + @Test + public void getTrackById_returnsTrackForValidId() { + long trackId = insertTestTrack(); + Track track = dataHelper.getTrackById(trackId); + assertNotNull(track); + assertEquals(trackId, track.getTrackId()); + } + + /** + * Returns {@code null} when no track with the given ID exists. + * + *

Currently fails — Bug B4: an empty cursor causes + * {@code CursorIndexOutOfBoundsException} instead. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B4_getTrackById_throwsWhenIdDoesNotExist} + * once the bug is fixed. + */ + @Ignore("Bug B4 — getTrackById() throws on non-existent ID instead of returning null. See docs/BUGS.md") + @Test + public void getTrackById_returnsNullForNonExistentId() { + Track result = dataHelper.getTrackById(99999L); + assertNull(result); + } + + /** + * Returns {@code null} gracefully when the ContentResolver returns a null cursor + * (e.g., due to a provider error) instead of throwing NPE. + * + *

Currently fails — Bug B4: {@code c.getCount()} throws NPE on a null cursor. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B4_getTrackById_throwsNPEWhenCursorIsNull} + * once the bug is fixed. + */ + @Ignore("Bug B4 — getTrackById() throws NPE on null cursor instead of returning null. See docs/BUGS.md") + @Test + public void getTrackById_returnsNullWhenCursorIsNull() { + Context mockContext = mock(Context.class); + ContentResolver mockCr = mock(ContentResolver.class); + when(mockContext.getContentResolver()).thenReturn(mockCr); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(null); + + DataHelper dh = new DataHelper(mockContext); + Track result = dh.getTrackById(1L); + assertNull(result); + } + + // ── getWayPointById() ───────────────────────────────────────────────────── + + /** Returns a populated {@link WayPoint} when the ID exists. */ + @Test + public void getWayPointById_returnsWayPointForValidId() { + long trackId = insertTestTrack(); + long wpId = insertTestWayPoint(trackId, UUID.randomUUID().toString()); + WayPoint wp = dataHelper.getWayPointById((int) wpId); + assertNotNull(wp); + } + + /** + * Returns {@code null} when no waypoint with the given ID exists. + * + *

Currently fails — Bug B5: an empty cursor causes + * {@code CursorIndexOutOfBoundsException} instead. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B5_getWayPointById_throwsWhenIdDoesNotExist} + * once the bug is fixed. + */ + @Ignore("Bug B5 — getWayPointById() throws on non-existent ID instead of returning null. See docs/BUGS.md") + @Test + public void getWayPointById_returnsNullForNonExistentId() { + WayPoint result = dataHelper.getWayPointById(99999); + assertNull(result); + } + + /** + * Returns {@code null} gracefully when the ContentResolver returns a null cursor. + * + *

Currently fails — Bug B5: {@code cWayPoint.getCount()} throws NPE. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B5_getWayPointById_throwsNPEWhenCursorIsNull} + * once the bug is fixed. + */ + @Ignore("Bug B5 — getWayPointById() throws NPE on null cursor instead of returning null. See docs/BUGS.md") + @Test + public void getWayPointById_returnsNullWhenCursorIsNull() { + Context mockContext = mock(Context.class); + ContentResolver mockCr = mock(ContentResolver.class); + when(mockContext.getContentResolver()).thenReturn(mockCr); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(null); + + DataHelper dh = new DataHelper(mockContext); + WayPoint result = dh.getWayPointById(1); + assertNull(result); + } + + // ── getTrackPointById() ─────────────────────────────────────────────────── + + /** Returns a populated {@link TrackPoint} when the ID exists. */ + @Test + public void getTrackPointById_returnsTrackPointForValidId() { + long trackId = insertTestTrack(); + long tpId = insertTestTrackPoint(trackId, 48.0, 2.0, 1L); + TrackPoint tp = dataHelper.getTrackPointById((int) tpId); + assertNotNull(tp); + } + + /** + * Returns {@code null} when no trackpoint with the given ID exists. + * + *

Currently fails — Bug B5: an empty cursor causes + * {@code CursorIndexOutOfBoundsException} instead. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B5_getTrackPointById_throwsWhenIdDoesNotExist} + * once the bug is fixed. + */ + @Ignore("Bug B5 — getTrackPointById() throws on non-existent ID instead of returning null. See docs/BUGS.md") + @Test + public void getTrackPointById_returnsNullForNonExistentId() { + TrackPoint result = dataHelper.getTrackPointById(99999); + assertNull(result); + } + + /** + * Returns {@code null} gracefully when the ContentResolver returns a null cursor. + * + *

Currently fails — Bug B5: {@code cTrackPoint.getCount()} throws NPE. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B5_getTrackPointById_throwsNPEWhenCursorIsNull} + * once the bug is fixed. + */ + @Ignore("Bug B5 — getTrackPointById() throws NPE on null cursor instead of returning null. See docs/BUGS.md") + @Test + public void getTrackPointById_returnsNullWhenCursorIsNull() { + Context mockContext = mock(Context.class); + ContentResolver mockCr = mock(ContentResolver.class); + when(mockContext.getContentResolver()).thenReturn(mockCr); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(null); + + DataHelper dh = new DataHelper(mockContext); + TrackPoint result = dh.getTrackPointById(1); + assertNull(result); + } + + // ── wayPoint() ──────────────────────────────────────────────────────────── + + /** Persists a waypoint row when location and extras are valid. */ + @Test + public void wayPoint_persistsWaypointWithValidLocation() { + long trackId = insertTestTrack(); + dataHelper.wayPoint(trackId, makeLocation(), "Cafe", null, + UUID.randomUUID().toString(), -1, 0, 0); + + Cursor c = context.getContentResolver().query( + TrackContentProvider.waypointsUri(trackId), null, null, null, null); + assertNotNull(c); + assertEquals(1, c.getCount()); + c.close(); + } + + /** + * Silently does nothing when {@code location} is {@code null} — no waypoint should + * be persisted and no exception should be thrown. + * + *

Currently fails — Bug B1: {@code location.getExtras()} is called before + * the {@code if (location != null)} guard, causing NPE immediately. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B1_wayPoint_throwsNPEWhenLocationIsNull} + * once the bug is fixed. + */ + @Ignore("Bug B1 — wayPoint() crashes before null guard when location is null. See docs/BUGS.md") + @Test + public void wayPoint_silentlyIgnoresNullLocation() { + long trackId = insertTestTrack(); + dataHelper.wayPoint(trackId, null, "name", null, UUID.randomUUID().toString(), 0, 0, 0); + + Cursor c = context.getContentResolver().query( + TrackContentProvider.waypointsUri(trackId), null, null, null, null); + assertNotNull(c); + assertEquals(0, c.getCount()); // no waypoint persisted + c.close(); + } + + /** + * Does not crash when {@code location.getExtras()} returns {@code null} + * (extras bundle never initialised). + * + *

Currently fails — Bug B1: {@code getExtras().getInt("satellites")} throws + * NPE when extras is {@code null}. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B1_wayPoint_throwsNPEWhenLocationExtrasAreNull} + * once the bug is fixed. + */ + @Ignore("Bug B1 — wayPoint() throws NPE when location has no extras bundle. See docs/BUGS.md") + @Test + public void wayPoint_doesNotCrashWhenLocationExtrasAreNull() { + long trackId = insertTestTrack(); + Location loc = new Location("gps"); + loc.setLatitude(48.0); + loc.setLongitude(2.0); + loc.setTime(System.currentTimeMillis()); + // No setExtras() call — getExtras() returns null + + dataHelper.wayPoint(trackId, loc, "name", null, UUID.randomUUID().toString(), 0, 0, 0); + } + + // ── trackNote() ─────────────────────────────────────────────────────────── + + /** Persists a note row when location and extras are valid. */ + @Test + public void trackNote_persistsNoteWithValidLocation() { + long trackId = insertTestTrack(); + dataHelper.trackNote(trackId, makeLocation(), "My note", UUID.randomUUID().toString()); + + Cursor c = context.getContentResolver().query( + TrackContentProvider.notesUri(trackId), null, null, null, null); + assertNotNull(c); + assertEquals(1, c.getCount()); + c.close(); + } + + /** + * Does not crash when {@code location.getExtras()} returns {@code null}. + * + *

Currently fails — Bug B2: there is no null guard anywhere in + * {@code trackNote()}; {@code getExtras().getInt("satellites")} throws NPE. + * Remove {@literal @}Ignore and delete + * {@code DataHelperTestBugs#bug_B2_trackNote_throwsNPEWhenLocationExtrasAreNull} + * once the bug is fixed. + */ + @Ignore("Bug B2 — trackNote() throws NPE when location has no extras bundle. See docs/BUGS.md") + @Test + public void trackNote_doesNotCrashWhenLocationExtrasAreNull() { + long trackId = insertTestTrack(); + Location loc = new Location("gps"); + loc.setLatitude(48.0); + loc.setLongitude(2.0); + loc.setTime(System.currentTimeMillis()); + // No setExtras() call + + dataHelper.trackNote(trackId, loc, "note text", UUID.randomUUID().toString()); + } + + // ── CRUD baselines ──────────────────────────────────────────────────────── + + /** track() inserts a trackpoint row visible via the ContentProvider. */ + @Test + public void track_insertsTrackPointIntoDb() { + long trackId = insertTestTrack(); + dataHelper.track(trackId, makeLocation(), -1f, 0, 0f, 1L); + + Cursor c = context.getContentResolver().query( + TrackContentProvider.trackPointsUri(trackId), null, null, null, null); + assertNotNull(c); + assertEquals(1, c.getCount()); + c.close(); + } + + /** stopTracking() flips the track's active flag; getActiveTrackId() reflects it. */ + @Test + public void stopTracking_marksTrackInactive() { + long trackId = insertTestTrack(); + assertEquals(trackId, DataHelper.getActiveTrackId(context.getContentResolver())); + + dataHelper.stopTracking(trackId); + + assertEquals(-1L, DataHelper.getActiveTrackId(context.getContentResolver())); + } + + /** getActiveTrackId() returns -1 when no active track exists in the DB. */ + @Test + public void getActiveTrackId_returnsNegativeOneWhenNoActiveTrack() { + assertEquals(-1L, DataHelper.getActiveTrackId(context.getContentResolver())); + } + + /** setTrackName() persists the new name so that getTrackNameInDB() returns it. */ + @Test + public void setTrackName_updatesNameInDb() { + long trackId = insertTestTrack(); + DataHelper.setTrackName(trackId, "My Test Track", context.getContentResolver()); + assertEquals("My Test Track", + DataHelper.getTrackNameInDB(trackId, context.getContentResolver())); + } + + /** deleteWayPoint() removes the waypoint row from the database. */ + @Test + public void deleteWayPoint_removesWayPointFromDb() { + long trackId = insertTestTrack(); + String uuid = UUID.randomUUID().toString(); + insertTestWayPoint(trackId, uuid); + + dataHelper.deleteWayPoint(uuid, null); + + Cursor after = context.getContentResolver().query( + TrackContentProvider.waypointsUri(trackId), null, null, null, null); + assertNotNull(after); + assertEquals(0, after.getCount()); + after.close(); + } +} diff --git a/app/src/test/java/net/osmtracker/db/DataHelperTestBugs.java b/app/src/test/java/net/osmtracker/db/DataHelperTestBugs.java new file mode 100644 index 00000000..ab4cf569 --- /dev/null +++ b/app/src/test/java/net/osmtracker/db/DataHelperTestBugs.java @@ -0,0 +1,319 @@ +package net.osmtracker.db; + +import android.content.ContentResolver; +import android.content.Context; +import android.database.Cursor; +import android.location.Location; +import android.net.Uri; + +import androidx.test.core.app.ApplicationProvider; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +import java.text.SimpleDateFormat; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Bug-confirming tests for {@link DataHelper}. + * + *

Every test in this class documents a known bug by asserting the current + * broken behaviour. Each test passes because the bug exists — the tests are expected + * to fail once the corresponding bug is fixed. + * + *

When a bug is fixed: + *

    + *
  1. Remove the {@literal @}Ignore annotation from the matching test in + * {@link DataHelperTest} (the intended-behaviour companion).
    2. + *
  2. Delete (or permanently skip) the test in this class — it no longer represents + * correct expected behaviour.
    3. + *
  3. Run {@code ./gradlew testDebugUnitTest} — the formerly-{@literal @}Ignored test in + * {@link DataHelperTest} must now pass.
    4. + *
+ * + *

See {@code docs/BUGS.md} for the full description of each bug, including GitHub issue + * templates and suggested fixes. + */ +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 25) +public class DataHelperTestBugs { + + private Context context; + private DataHelper dataHelper; + + @Before + public void setUp() { + context = ApplicationProvider.getApplicationContext(); + dataHelper = new DataHelper(context); + } + + // ── Bug B1: wayPoint() NPE before null guard ────────────────────────────── + + /** + * Bug B1 — {@code wayPoint()} calls {@code location.getExtras().getInt("satellites")} + * before the {@code if (location != null)} guard, so a null location causes NPE + * immediately rather than being skipped. + * + *

This test passes today because the NPE is thrown. When Bug B1 is fixed, + * the method will silently skip null locations and this test will fail. + */ + @Test(expected = NullPointerException.class) + public void bug_B1_wayPoint_throwsNPEWhenLocationIsNull() { + long trackId = insertTestTrack(); + dataHelper.wayPoint(trackId, null, "name", null, UUID.randomUUID().toString(), 0, 0, 0); + } + + /** + * Bug B1 — when {@code location.getExtras()} returns {@code null} (extras never + * initialised), {@code getExtras().getInt("satellites")} throws NPE. + * + *

This test passes today because the NPE is thrown. When Bug B1 is fixed, + * this scenario will be handled gracefully and this test will fail. + */ + @Test(expected = NullPointerException.class) + public void bug_B1_wayPoint_throwsNPEWhenLocationExtrasAreNull() { + long trackId = insertTestTrack(); + Location loc = new Location("gps"); + loc.setLatitude(48.0); + loc.setLongitude(2.0); + loc.setTime(System.currentTimeMillis()); + // intentionally no setExtras() — getExtras() returns null + + dataHelper.wayPoint(trackId, loc, "name", null, UUID.randomUUID().toString(), 0, 0, 0); + } + + // ── Bug B2: trackNote() NPE on null extras ───────────────────────────────── + + /** + * Bug B2 — {@code trackNote()} has no null guard at all before calling + * {@code location.getExtras().getInt("satellites")}, so a location without an extras + * bundle causes NPE. + * + *

This test passes today because the NPE is thrown. When Bug B2 is fixed, + * this scenario will be handled gracefully and this test will fail. + */ + @Test(expected = NullPointerException.class) + public void bug_B2_trackNote_throwsNPEWhenLocationExtrasAreNull() { + long trackId = insertTestTrack(); + Location loc = new Location("gps"); + loc.setLatitude(48.0); + loc.setLongitude(2.0); + loc.setTime(System.currentTimeMillis()); + // intentionally no setExtras() + + dataHelper.trackNote(trackId, loc, "note text", UUID.randomUUID().toString()); + } + + // ── Bug B3: getSegmentIdFor() cursor leak ───────────────────────────────── + + /** + * Bug B3 — {@code getSegmentIdFor()} opens a cursor but never calls + * {@code cursor.close()} in either the early-return or the normal-return code path. + * + *

This test passes today because {@code close()} is never called. + * When Bug B3 is fixed, the cursor will be closed and this test will fail + * (the {@code verify(never())} assertion will throw). + */ + @Test + public void bug_B3_getSegmentIdFor_doesNotCloseCursor() { + ContentResolver mockCr = mock(ContentResolver.class); + Cursor mockCursor = mock(Cursor.class); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(mockCursor); + when(mockCursor.moveToFirst()).thenReturn(false); + + DataHelper.getSegmentIdFor(99999L, mockCr); + + // Passes today because close() is never called — confirms Bug B3 + verify(mockCursor, never()).close(); + } + + // ── Bug B4: getTrackById() null / empty cursor ──────────────────────────── + + /** + * Bug B4 — when the {@link android.content.ContentProvider} returns a {@code null} + * cursor, {@code getTrackById()} immediately dereferences it, causing NPE. + * + *

This test passes today because NPE is thrown. When Bug B4 is fixed, + * the method will return {@code null} gracefully and this test will fail. + */ + @Test(expected = NullPointerException.class) + public void bug_B4_getTrackById_throwsNPEWhenCursorIsNull() { + Context mockContext = mock(Context.class); + ContentResolver mockCr = mock(ContentResolver.class); + when(mockContext.getContentResolver()).thenReturn(mockCr); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(null); + + DataHelper dh = new DataHelper(mockContext); + dh.getTrackById(1L); + } + + /** + * Bug B4 — when no row matches the requested ID, the cursor is empty and + * {@code cursor.moveToFirst()} returns {@code false}, but the code calls + * {@code Track.build()} unconditionally, causing {@code CursorIndexOutOfBoundsException}. + * + *

This test passes today because the exception is thrown. When Bug B4 is fixed, + * the method will return {@code null} for a missing ID and this test will fail. + */ + @Test(expected = android.database.CursorIndexOutOfBoundsException.class) + public void bug_B4_getTrackById_throwsWhenIdDoesNotExist() { + dataHelper.getTrackById(99999L); + } + + // ── Bug B5: getWayPointById() / getTrackPointById() null / empty cursor ─── + + /** + * Bug B5 — {@code getWayPointById()} does not guard against a {@code null} cursor, + * causing NPE when the ContentProvider returns {@code null}. + * + *

This test passes today because NPE is thrown. When Bug B5 is fixed, + * the method will return {@code null} gracefully and this test will fail. + */ + @Test(expected = NullPointerException.class) + public void bug_B5_getWayPointById_throwsNPEWhenCursorIsNull() { + Context mockContext = mock(Context.class); + ContentResolver mockCr = mock(ContentResolver.class); + when(mockContext.getContentResolver()).thenReturn(mockCr); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(null); + + DataHelper dh = new DataHelper(mockContext); + dh.getWayPointById(1); + } + + /** + * Bug B5 — when no row matches the requested waypoint ID, the empty cursor causes + * {@code CursorIndexOutOfBoundsException}. + * + *

This test passes today because the exception is thrown. When Bug B5 is fixed, + * the method will return {@code null} and this test will fail. + */ + @Test(expected = android.database.CursorIndexOutOfBoundsException.class) + public void bug_B5_getWayPointById_throwsWhenIdDoesNotExist() { + dataHelper.getWayPointById(99999); + } + + /** + * Bug B5 — {@code getTrackPointById()} does not guard against a {@code null} cursor, + * causing NPE when the ContentProvider returns {@code null}. + * + *

This test passes today because NPE is thrown. When Bug B5 is fixed, + * the method will return {@code null} gracefully and this test will fail. + */ + @Test(expected = NullPointerException.class) + public void bug_B5_getTrackPointById_throwsNPEWhenCursorIsNull() { + Context mockContext = mock(Context.class); + ContentResolver mockCr = mock(ContentResolver.class); + when(mockContext.getContentResolver()).thenReturn(mockCr); + when(mockCr.query(any(Uri.class), any(), any(), any(), any())).thenReturn(null); + + DataHelper dh = new DataHelper(mockContext); + dh.getTrackPointById(1); + } + + /** + * Bug B5 — when no row matches the requested trackpoint ID, the empty cursor causes + * {@code CursorIndexOutOfBoundsException}. + * + *

This test passes today because the exception is thrown. When Bug B5 is fixed, + * the method will return {@code null} and this test will fail. + */ + @Test(expected = android.database.CursorIndexOutOfBoundsException.class) + public void bug_B5_getTrackPointById_throwsWhenIdDoesNotExist() { + dataHelper.getTrackPointById(99999); + } + + // ── Bug B8: FILENAME_FORMATTER thread safety ─────────────────────────────── + + /** + * Bug B8 — {@code DataHelper.FILENAME_FORMATTER} is a shared, mutable + * {@code public static final SimpleDateFormat}. {@link SimpleDateFormat} is not + * thread-safe: concurrent calls to {@code format()} corrupt each other's output. + * + *

This test attempts to reproduce the race by formatting two distinct timestamps + * simultaneously from multiple threads and checking that each result matches the + * expected string. It may pass non-deterministically on single-core JVMs but + * consistently exposes the bug under real concurrent load. + * + *

When Bug B8 is fixed (e.g., by using {@code ThreadLocal}), + * all threads will produce correct results and this test should be deleted in favour + * of a simple deterministic formatting test. + */ + @Test + public void bug_B8_filenameFormatter_isNotThreadSafe() throws InterruptedException { + // Two dates that format to obviously different strings + final Date dateA = new Date(0L); // 1970-01-01 + final Date dateB = new Date(1_000_000_000_000L); // 2001-09-09 + + final String expectedA = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss").format(dateA); + final String expectedB = new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss").format(dateB); + + final int threads = 20; + final int iterations = 100; + final AtomicBoolean corruptionDetected = new AtomicBoolean(false); + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch doneLatch = new CountDownLatch(threads); + + ExecutorService pool = Executors.newFixedThreadPool(threads); + for (int t = 0; t < threads; t++) { + final boolean useA = (t % 2 == 0); + pool.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < iterations; i++) { + Date d = useA ? dateA : dateB; + String expected = useA ? expectedA : expectedB; + String actual = DataHelper.FILENAME_FORMATTER.format(d); + if (!actual.equals(expected)) { + corruptionDetected.set(true); + } + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + doneLatch.countDown(); + } + }); + } + + startLatch.countDown(); // release all threads simultaneously + assertTrue("Threads should finish within 10 seconds", + doneLatch.await(10, TimeUnit.SECONDS)); + pool.shutdown(); + + // Passes when corruption is detected (confirms bug). + // On single-core JVMs this may occasionally not detect corruption — that is expected. + // If this assertion consistently fails after a fix, the fix resolved the race. + assertTrue("Bug B8: FILENAME_FORMATTER produced corrupted output under concurrent access " + + "(expected on multi-core JVM — confirms thread-safety bug)", corruptionDetected.get()); + } + + // ── Helper ──────────────────────────────────────────────────────────────── + + private long insertTestTrack() { + android.content.ContentValues values = new android.content.ContentValues(); + values.put(TrackContentProvider.Schema.COL_START_DATE, System.currentTimeMillis()); + values.put(TrackContentProvider.Schema.COL_ACTIVE, TrackContentProvider.Schema.VAL_TRACK_ACTIVE); + android.net.Uri uri = context.getContentResolver().insert( + TrackContentProvider.CONTENT_URI_TRACK, values); + return android.content.ContentUris.parseId(uri); + } +} From af26f1bd8f60afc9791293118288a2eb4fd9ad5e Mon Sep 17 00:00:00 2001 From: Milton Barrera Date: Sun, 29 Mar 2026 00:25:53 -0600 Subject: [PATCH 4/5] removing not needed md --- docs/BUGS.md | 725 --------------------------------------------------- 1 file changed, 725 deletions(-) delete mode 100644 docs/BUGS.md diff --git a/docs/BUGS.md b/docs/BUGS.md deleted file mode 100644 index 4af0a04c..00000000 --- a/docs/BUGS.md +++ /dev/null @@ -1,725 +0,0 @@ -# OSMTracker Android — Bug Report Catalogue - -This document describes **9 confirmed bugs** found in the `DataHelper` and `GPSLogger` -modules during a static-analysis + unit-test audit (branch `new-test-cases`). - -For each bug you will find: -- A precise description and the affected code location. -- The severity and the risk if left unfixed. -- The unit test(s) that cover it and **a detailed explanation of why those tests pass - today even though the bug is still present**. -- A ready-to-paste GitHub issue description. - ---- - -## How can a test pass when a bug exists? - -There are two complementary strategies used in this catalogue: - -| Strategy | When used | What it means | -|----------|-----------|---------------| -| **Assert the broken behaviour** | Crash bugs (NPE, missing guard) | `assertThrows(NullPointerException.class, ...)` — the test *expects* the crash. It passes today because the crash *does* happen. When the bug is fixed the assertion must be removed. | -| **Assert the absence of the correct behaviour** | Resource/threading bugs | e.g. `verify(cursor, never()).close()` — the test asserts that `close()` is *never* called. It passes today because the bug means `close()` is indeed never called. When the bug is fixed, the `never()` must change to a plain `verify()`. | - -This technique is sometimes called **characterisation testing**: tests lock in the current -(broken) behaviour so that any future change to the code immediately shows up as a test -result change, prompting the developer to either confirm the fix or update the test. - ---- - -## Bug B1 — `wayPoint()`: NPE before null guard - -### Location -`DataHelper.java`, lines 213–214 (inside `wayPoint()`): -```java -Log.d(TAG, "Tracking waypoint … nbSatellites=" + location.getExtras().getInt("satellites") …); -// ↑ This line runs BEFORE the guard on line 220: -if (location != null) { … } -``` - -### Description -The method accesses `location.getExtras().getInt("satellites")` in a log statement at the -very start of the method body, **before** any null check on `location` or on its extras -bundle. This means: - -- Passing `location = null` (which the comment on line 218 acknowledges can happen) throws - `NullPointerException` immediately. -- Passing a `Location` whose extras bundle was never initialised (common when constructing - a `Location` programmatically) also throws `NullPointerException` because - `getExtras()` returns `null`. - -The `if (location != null)` guard on line 220 was intended to protect the insertion logic, -but the crash happens before it is ever reached. - -### Severity: CRITICAL — app crash - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `E1_wayPoint_throwsNPEWhenLocationIsNull` | `assertThrows(NullPointerException.class, …)` | The NPE is thrown on line 214; `assertThrows` catches it and the test is green. | -| `E2_wayPoint_throwsNPEWhenLocationExtrasIsNull` | `assertThrows(NullPointerException.class, …)` | `getExtras()` returns `null`; `.getInt(…)` on null throws NPE; `assertThrows` catches it. | - -When the bug is fixed (null-check the location and its extras before the log line), both -`assertThrows` calls will no longer see an exception and the tests will fail — that is the -intended signal to update the assertions. - -### Fix hint -Move the `if (location != null)` guard to wrap the entire method body, including the log -statement, and add a separate guard for `location.getExtras() != null`. - ---- - -### GitHub issue body - -**Title:** `wayPoint()` crashes with NPE when `location` is null or has no extras bundle - -``` -## Bug description -`DataHelper.wayPoint()` calls `location.getExtras().getInt("satellites")` in a log -statement (line 214) **before** the `if (location != null)` guard on line 220. - -Passing a null `location` — or a `Location` object whose extras bundle was never -initialised — throws `NullPointerException` immediately, crashing any operation that -tries to record a waypoint under those conditions. - -## Reproduction -```java -dataHelper.wayPoint(trackId, null, "name", null, uuid, 0, 0, 0); -// → NullPointerException at DataHelper.java:214 - -Location loc = new Location("gps"); // no setExtras() call -dataHelper.wayPoint(trackId, loc, "name", null, uuid, 0, 0, 0); -// → NullPointerException at DataHelper.java:214 -``` - -## Expected behaviour -The method should silently return (or log a warning) when `location` is null, -consistent with the developer comment already on line 218: -> "location should not be null, but sometime is." - -## Affected file -`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 213–220 - -## Labels -`bug`, `crash`, `null-safety` -``` - ---- - -## Bug B2 — `trackNote()`: NPE, no null guard at all - -### Location -`DataHelper.java`, lines 326–328 (inside `trackNote()`): -```java -Log.d(TAG, "Tracking note … nbSatellites=" + location.getExtras().getInt("satellites") …); -// No null check on location or its extras anywhere in this method -``` - -### Description -Unlike `wayPoint()`, which at least has a (misplaced) `if (location != null)` guard, -`trackNote()` has **no null protection whatsoever**. The log statement on line 327 -dereferences `location.getExtras()` unconditionally. Any caller that passes a `Location` -object without a pre-initialised extras bundle will trigger an immediate NPE. - -### Severity: CRITICAL — app crash - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `F1_trackNote_throwsNPEWhenLocationExtrasIsNull` | `assertThrows(NullPointerException.class, …)` | `getExtras()` returns `null`; chaining `.getInt(…)` throws NPE; `assertThrows` catches it. | - -### Fix hint -Add a null check on `location` (and `location.getExtras()`) before the log statement, -mirroring the fix required for B1. - ---- - -### GitHub issue body - -**Title:** `trackNote()` crashes with NPE — no null guard on `location` or its extras - -``` -## Bug description -`DataHelper.trackNote()` calls `location.getExtras().getInt("satellites")` in its first -log statement (line 327) without any null check on `location` or on its extras bundle. -Unlike `wayPoint()`, there is no guard anywhere in this method. - -## Reproduction -```java -Location loc = new Location("gps"); // no setExtras() -dataHelper.trackNote(trackId, loc, "note", uuid); -// → NullPointerException at DataHelper.java:327 -``` - -## Expected behaviour -The method should record the note even when the location has no extras bundle. The -satellite count should be treated as 0 or omitted from the log. - -## Affected file -`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 325–328 - -## Labels -`bug`, `crash`, `null-safety` -``` - ---- - -## Bug B3 — `getSegmentIdFor()`: Cursor never closed — resource leak - -### Location -`DataHelper.java`, lines 419–429 (inside `getSegmentIdFor()`): -```java -public static long getSegmentIdFor(long trackId, ContentResolver cr) { - Cursor ca = cr.query(…); // cursor opened here - - if (!ca.moveToFirst()) { - return 0; // ← cursor NOT closed before early return - } - - return Track.build(trackId, ca, cr, true).getMaxSegId(); - // ← cursor NOT closed in the success path either -} -``` - -### Description -The `Cursor` returned by `ContentResolver.query()` is opened at the top of the method but -is **never closed** in either code path — neither the early-return path (track not found) -nor the success path. This method is called every time GPS tracking starts (and potentially -on every segment boundary), so each call leaks a database cursor. Over time this exhausts -the cursor window pool, causing `SQLiteCantOpenDatabaseException` or -`CursorWindowAllocationException` on long-running tracking sessions. - -### Severity: HIGH — resource leak, potential crash after extended use - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `A2_getSegmentIdFor_doesNotCloseCursorInEarlyReturnPath_documentedBug` | `verify(mockCursor, never()).close()` | The mock cursor's `close()` is genuinely never called, so `never()` is satisfied. | - -The assertion uses `never()` deliberately: it documents that `close()` is *absent*. When -the bug is fixed and `close()` is added, this `never()` assertion will fail — the -developer must then change it to `verify(mockCursor).close()`. - -### Fix hint -Wrap the method body in a `try-finally` block, or use try-with-resources if targeting -API 16+ (which this project does). - ---- - -### GitHub issue body - -**Title:** `getSegmentIdFor()`: Cursor opened but never closed — resource leak on every track start - -``` -## Bug description -`DataHelper.getSegmentIdFor()` opens a `Cursor` via `ContentResolver.query()` but never -calls `cursor.close()` in either code path (early return when the track is not found, or -the normal return after calling `Track.build()`). - -This method is invoked every time GPS tracking starts. Each invocation leaks one cursor -handle. On long recording sessions the cursor pool can be exhausted, causing -`CursorWindowAllocationException` or `SQLiteCantOpenDatabaseException`. - -## Affected file -`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 419–429 - -## Suggested fix -```java -public static long getSegmentIdFor(long trackId, ContentResolver cr) { - Cursor ca = cr.query(…); - if (ca == null) return 0; - try { - if (!ca.moveToFirst()) return 0; - return Track.build(trackId, ca, cr, true).getMaxSegId(); - } finally { - ca.close(); - } -} -``` - -## Labels -`bug`, `database`, `resource-leak` -``` - ---- - -## Bug B4 — `getTrackById()`: No null or empty-cursor guard - -### Location -`DataHelper.java`, lines 584–595 (inside `getTrackById()`): -```java -public Track getTrackById(long trackId) { - Cursor c = context.getContentResolver().query(…); - Log.d(TAG, "Count: " + c.getCount()); // NPE if c is null - c.moveToFirst(); // no check on return value - Track track = Track.build(trackId, c, contentResolver, true); // reads invalid position - c.close(); - return track; -} -``` -Even the developer left a `//TODO: Fix this method` comment above it. - -### Description -Two distinct failure scenarios: - -1. **Null cursor**: `ContentResolver.query()` is permitted to return `null` (e.g., if the - provider crashes or is unavailable). Calling `c.getCount()` on a null reference throws - NPE immediately. -2. **Empty cursor**: When the given `trackId` does not exist in the database, the cursor - is valid but empty. `moveToFirst()` returns `false` (the cursor stays at position −1), - but its return value is never checked. The subsequent `Track.build()` call reads column - values from an invalid cursor position, throwing - `CursorIndexOutOfBoundsException`. - -### Severity: HIGH — crash on invalid or missing track ID - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `B1_getTrackById_throwsNPEWhenCursorIsNull` | `assertThrows(NullPointerException.class, …)` | Mock ContentResolver returns null; `c.getCount()` throws NPE; `assertThrows` catches it. | -| `B2_getTrackById_throwsForNonExistentId` | `assertThrows(RuntimeException.class, …)` | Empty cursor; `Track.build()` reads position −1; `CursorIndexOutOfBoundsException` is thrown and caught. | - -### Fix hint -Check `c != null` and `c.moveToFirst()` before calling `Track.build()`. Return `null` (or -throw a meaningful checked exception) on failure, and always close the cursor. - ---- - -### GitHub issue body - -**Title:** `getTrackById()` crashes on null cursor or non-existent track ID - -``` -## Bug description -`DataHelper.getTrackById()` has two crash paths (acknowledged by the TODO comment above -the method): - -1. If `ContentResolver.query()` returns `null`, `c.getCount()` throws NPE. -2. If the track does not exist, `moveToFirst()` is called but its `false` return is - ignored. `Track.build()` then reads from an invalid cursor position, throwing - `CursorIndexOutOfBoundsException`. - -## Reproduction -```java -// Case 1: provider returns null -dataHelper.getTrackById(1L); // with mocked null cursor → NPE - -// Case 2: ID does not exist -dataHelper.getTrackById(99999L); // → CursorIndexOutOfBoundsException -``` - -## Affected file -`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 582–595 - -## Labels -`bug`, `crash`, `database` -``` - ---- - -## Bug B5 — `getWayPointById()` / `getTrackPointById()`: Same null/empty-cursor pattern - -### Location -`DataHelper.java`: -- `getWayPointById()`, lines 618–630 -- `getTrackPointById()`, lines 652–664 - -### Description -Both methods share the same structural flaw as B4: -- No null check on the cursor returned by `ContentResolver.query()`. -- No check on the return value of `moveToFirst()` before passing the cursor to the - `WayPoint(Cursor)` / `TrackPoint(Cursor)` constructors. -- Neither method closes the cursor after use (a secondary resource leak). - -When a non-existent ID is queried, the cursor is empty and the constructor reads from an -invalid position, crashing with `CursorIndexOutOfBoundsException`. - -### Severity: HIGH — crash on invalid waypoint or trackpoint ID - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `C1_getWayPointById_throwsNPEWhenCursorIsNull` | `assertThrows(NullPointerException.class, …)` | Null cursor; `cWayPoint.getCount()` throws NPE. | -| `C2_getWayPointById_throwsForNonExistentId` | `assertThrows(RuntimeException.class, …)` | Empty cursor; constructor reads position −1; exception thrown. | -| `D1_getTrackPointById_throwsNPEWhenCursorIsNull` | `assertThrows(NullPointerException.class, …)` | Same as C1 for trackpoints. | -| `D2_getTrackPointById_throwsForNonExistentId` | `assertThrows(RuntimeException.class, …)` | Same as C2 for trackpoints. | - -### Fix hint -Apply the same null-and-empty-cursor pattern as described for B4, and add `cursor.close()` -after constructing the model object (or use try-finally). - ---- - -### GitHub issue body - -**Title:** `getWayPointById()` and `getTrackPointById()` crash on null cursor or non-existent ID - -``` -## Bug description -Both `DataHelper.getWayPointById()` and `DataHelper.getTrackPointById()` share the same -structural flaws: - -1. No null check on the cursor from `ContentResolver.query()` → NPE. -2. `moveToFirst()` return value is ignored → `CursorIndexOutOfBoundsException` on empty cursor. -3. The cursor is never closed after use → secondary resource leak. - -## Reproduction -```java -dataHelper.getWayPointById(99999); // → CursorIndexOutOfBoundsException -dataHelper.getTrackPointById(99999); // → CursorIndexOutOfBoundsException -``` - -## Affected file -`app/src/main/java/net/osmtracker/db/DataHelper.java`, lines 618–664 - -## Labels -`bug`, `crash`, `database`, `resource-leak` -``` - ---- - -## Bug B6 — `GPSLogger.currentSegmentId`: Race condition between threads - -### Location -`GPSLogger.java`, line 89: -```java -private long currentSegmentId = -1; // ← should be volatile -``` -Written on the **main thread** (in `startTracking()`, line 326): -```java -currentSegmentId = DataHelper.getSegmentIdFor(trackId, getContentResolver()) + 1; -``` -Read on the **GPS callback thread** (in `onLocationChanged()`, line 356): -```java -dataHelper.track(currentTrackId, location, …, currentSegmentId); -``` - -### Description -Java's memory model does not guarantee that a write made on one thread is immediately -visible to another thread unless the field is declared `volatile` or the access is -synchronised. Without `volatile`, the GPS thread may observe a **stale value** of -`currentSegmentId` (e.g., still −1) even after the main thread has updated it. This leads -to trackpoints being recorded with the wrong segment ID, silently corrupting the GPX -output on multi-core devices. - -The bug is non-deterministic: on a single-core device or emulator it may never manifest, -which makes it particularly dangerous in production. - -### Severity: MEDIUM — silent data corruption, non-deterministic - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `J1_currentSegmentId_isNegativeOneBeforeTracking` | `assertEquals(-1L, segId)` | Passes because the field is correctly initialised to −1. Not a bug test, but a baseline. | -| `J2_currentSegmentId_fieldIsNotVolatile_documentedRaceCondition` | `assertFalse(isVolatile)` | Uses `Field.getModifiers()` to check the `volatile` modifier bit. The field is NOT volatile, so `assertFalse` is satisfied — confirming the bug. | - -`J2` asserts the *absence* of `volatile`. When the bug is fixed (field made `volatile`), -`isVolatile` will be `true` and `assertFalse` will fail — the developer must then change -it to `assertTrue`. - -Note: Robolectric runs tests single-threaded, so the actual memory-visibility race cannot -be reproduced in a unit test. The test instead validates the structural precondition -(missing modifier) that makes the race possible. - -### Fix hint -Declare the field as `private volatile long currentSegmentId = -1;`. - ---- - -### GitHub issue body - -**Title:** `GPSLogger.currentSegmentId` is not `volatile` — race condition between main thread and GPS thread - -``` -## Bug description -`GPSLogger.currentSegmentId` is written on the main thread (inside `startTracking()`) and -read on the GPS location callback thread (inside `onLocationChanged()`). The field is a -plain `long` without the `volatile` modifier, so the Java Memory Model provides no -visibility guarantee between the two threads. - -On a multi-core device the GPS thread may read a stale value (e.g., still −1) even after -the main thread has updated it. Trackpoints would then be recorded with the wrong segment -ID, silently corrupting the exported GPX file. - -## Root cause -```java -// GPSLogger.java line 89 -private long currentSegmentId = -1; // missing volatile -``` - -## Fix -```java -private volatile long currentSegmentId = -1; -``` - -## Affected file -`app/src/main/java/net/osmtracker/service/gps/GPSLogger.java`, line 89 - -## Labels -`bug`, `thread-safety`, `data-corruption` -``` - ---- - -## Bug B7 — `GPSLogger` BroadcastReceiver: Empty `catch(NullPointerException)` silently swallows error - -### Location -`GPSLogger.java`, lines 160–163 (inside the `INTENT_DELETE_WP` branch of the -`BroadcastReceiver`): -```java -String filePath = null; -try { - filePath = link.equals("null") ? null - : DataHelper.getTrackDirectory(trackId, context) + "/" + link; -} catch(NullPointerException ne) {} // ← empty catch -dataHelper.deleteWayPoint(uuid, filePath); -``` - -### Description -When the `INTENT_DELETE_WP` broadcast is received without a `link` extra, -`intent.getExtras().getString(OSMTracker.INTENT_KEY_LINK)` returns `null`. The code then -calls `link.equals("null")` which immediately throws `NullPointerException`. The empty -`catch` block silently swallows this exception, leaving `filePath` as `null`. - -The consequence is two-fold: -1. `deleteWayPoint(uuid, null)` is still called, so the **database row is deleted** — the - waypoint is gone from the UI. -2. Because `filePath` is `null`, **the attached media file is never deleted** from - storage. The file becomes permanently orphaned, silently consuming disk space. - -The correct idiom is `"null".equals(link)` (Yoda condition), which is null-safe. - -### Severity: MEDIUM — silent failure, orphaned files, disk leak - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `K1_deleteWp_nullLink_swallowsNPEAndStillCallsDeleteWayPoint` | `verify(mockDataHelper).deleteWayPoint(eq(uuid), isNull())` | The NPE is swallowed, execution continues, and `deleteWayPoint` IS called with `null` as the file path. Mockito's `verify` confirms this. | - -The test passes because it verifies the *observable consequence* of the bug (the waypoint -row is removed but no file path is passed). If the bug is fixed with the Yoda condition, -`filePath` will be correctly `null` for a null `link` (same result for this specific -case), but the empty catch block will be eliminated — a separate assertion (or -code-review check) would then confirm the fix. - -### Fix hint -Replace `link.equals("null")` with `"null".equals(link)` and remove the `try-catch` entirely. - ---- - -### GitHub issue body - -**Title:** `GPSLogger` BroadcastReceiver silently swallows NPE on waypoint deletion — attached file never deleted - -``` -## Bug description -When `INTENT_DELETE_WP` is received without a `link` extra, `link` is `null`. -`link.equals("null")` throws `NullPointerException`, which is caught by an **empty** -`catch(NullPointerException)` block. Execution continues with `filePath = null`. - -Result: -- The waypoint **database row is deleted** (correct). -- The **attached media file is never deleted** (incorrect) — it becomes permanently - orphaned on the device's storage. - -## Root cause -```java -// GPSLogger.java lines 160-163 -try { - filePath = link.equals("null") ? null : …; // NPE when link is null -} catch(NullPointerException ne) {} // silently swallowed -``` - -## Fix -```java -// Null-safe Yoda condition — no try-catch needed -filePath = "null".equals(link) ? null - : DataHelper.getTrackDirectory(trackId, context) + "/" + link; -``` - -## Affected file -`app/src/main/java/net/osmtracker/service/gps/GPSLogger.java`, lines 158–164 - -## Labels -`bug`, `silent-failure`, `file-management` -``` - ---- - -## Bug B8 — `DataHelper.FILENAME_FORMATTER`: `SimpleDateFormat` is not thread-safe - -### Location -`DataHelper.java`, line 113: -```java -public static final SimpleDateFormat FILENAME_FORMATTER = - new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); -``` - -### Description -`java.text.SimpleDateFormat` is explicitly documented as **not thread-safe**. It maintains -internal mutable state (calendar fields, number formatters) that is modified during each -`format()` or `parse()` call. When two threads call `FILENAME_FORMATTER.format(date)` -concurrently, they corrupt each other's internal state, producing garbled or swapped date -strings in file names. - -In OSMTracker, file-naming is used for waypoint media files (photos, voice recordings). -Under concurrent waypoint recording (e.g., rapid tapping), two waypoints could end up -with identical or incorrect timestamps in their file names. - -### Severity: LOW — data corruption under concurrent use, rare in practice - -### Covering tests and why they pass -| Test | Assertion | Why it passes today | -|------|-----------|---------------------| -| `G1_filenameFormatter_isNotThreadSafe_documentedRaceCondition` | Logs errors, does not fail | The test submits 100 concurrent `format()` calls from 20 threads and collects any garbled output. It does not call `fail()` because the race is timing-dependent — it may not manifest in every run. Any error printed to stdout indicates Bug B8 has been reproduced. | - -This test is intentionally non-failing; it serves as a **canary**. On a loaded multi-core -CI machine it occasionally produces output confirming the race. - -### Fix hint -Use `ThreadLocal`: -```java -public static final ThreadLocal FILENAME_FORMATTER = - ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss")); -// Usage: DataHelper.FILENAME_FORMATTER.get().format(date) -``` -Or use the thread-safe `DateTimeFormatter` from `java.time` (API 26+). - ---- - -### GitHub issue body - -**Title:** `DataHelper.FILENAME_FORMATTER` (`SimpleDateFormat`) is not thread-safe — potential garbled file names - -``` -## Bug description -`DataHelper.FILENAME_FORMATTER` is a `public static final SimpleDateFormat` instance -shared across all threads. `SimpleDateFormat` is explicitly not thread-safe. Concurrent -calls to `format()` (e.g., during rapid waypoint recording) corrupt its internal state -and produce incorrect date strings in media file names. - -## Root cause -```java -// DataHelper.java line 113 -public static final SimpleDateFormat FILENAME_FORMATTER = - new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss"); -``` - -## Fix -```java -public static final ThreadLocal FILENAME_FORMATTER = - ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd_HH-mm-ss")); -``` - -## Affected file -`app/src/main/java/net/osmtracker/db/DataHelper.java`, line 113 - -## Labels -`bug`, `thread-safety` -``` - ---- - -## Bug B9 — `Track.setTrackId()`: Setter has an empty body — silently does nothing - -### Location -`Track.java`, lines 134–135: -```java -public void setTrackId(long trackId) { - // empty — the field `this.trackId` is never assigned -} -``` - -### Description -The setter `setTrackId()` is declared, accepts a parameter, but contains **no -implementation**. Any caller that tries to update the track ID of a `Track` object via -this setter will observe no effect: the internal `trackId` field will retain its previous -value. There is no exception, no log message — the data update is silently discarded. - -This is especially insidious because `getTrackId()` exists and works correctly; the -asymmetry is not obvious from the API surface. If any part of the codebase (or a -future contributor) calls `setTrackId()` expecting to update the object, the resulting -silent mismatch can lead to incorrect GPX exports, wrong database queries, or corrupt -OSM uploads. - -### Severity: LOW — silent incorrect behaviour; impact depends on call sites - -> **Note:** No automated unit test was written for this bug in the current audit because -> `Track.build()` (the primary construction path) sets `trackId` directly via field -> assignment (`out.trackId = trackId`), bypassing the setter. The setter is currently -> uncalled in production code. The risk is forward-looking: any future refactor that -> routes through the setter will silently break. - -### Fix hint -```java -public void setTrackId(long trackId) { - this.trackId = trackId; // add this line -} -``` - ---- - -### GitHub issue body - -**Title:** `Track.setTrackId()` has an empty body — setter silently does nothing - -``` -## Bug description -`Track.setTrackId(long trackId)` is declared as a public setter but its body is empty: -the internal `trackId` field is never updated. Any caller of this method will silently -get no effect. - -## Root cause -```java -// Track.java lines 134-135 -public void setTrackId(long trackId) { - // missing: this.trackId = trackId; -} -``` - -## Risk -The bug is currently latent because the primary construction path (`Track.build()`) -sets `trackId` directly. However, any future code or refactor that calls `setTrackId()` -will silently produce wrong track IDs in database queries, GPX exports, or OSM uploads -without any error or warning. - -## Fix -```java -public void setTrackId(long trackId) { - this.trackId = trackId; -} -``` - -## Affected file -`app/src/main/java/net/osmtracker/db/model/Track.java`, lines 134–135 - -## Labels -`bug`, `data-model` -``` - ---- - -## Quick reference - -| Bug | Module | Severity | Crash? | Tests | -|-----|--------|----------|--------|-------| -| B1 — `wayPoint()` NPE before null guard | `DataHelper` | CRITICAL | Yes | E1, E2 | -| B2 — `trackNote()` NPE, no guard at all | `DataHelper` | CRITICAL | Yes | F1 | -| B3 — `getSegmentIdFor()` cursor never closed | `DataHelper` | HIGH | Eventually | A2 | -| B4 — `getTrackById()` no null/empty-cursor guard | `DataHelper` | HIGH | Yes | B1, B2 | -| B5 — `getWayPointById()` / `getTrackPointById()` same as B4 | `DataHelper` | HIGH | Yes | C1, C2, D1, D2 | -| B6 — `currentSegmentId` non-`volatile` race | `GPSLogger` | MEDIUM | No (data corruption) | J2 | -| B7 — Empty `catch(NPE)` swallows null-link error | `GPSLogger` | MEDIUM | No (silent failure) | K1 | -| B8 — `FILENAME_FORMATTER` not thread-safe | `DataHelper` | LOW | No (garbled names) | G1 | -| B9 — `Track.setTrackId()` empty body | `Track` | LOW | No (silent wrong data) | — | - -### Recommended fix order -Fix the highest-severity bugs first to prevent user-visible crashes: - -1. **B1, B2** — add null guards in `wayPoint()` and `trackNote()` (one-line fixes). -2. **B4, B5** — add null and empty-cursor guards in the three `getById()` methods. -3. **B3** — wrap `getSegmentIdFor()` cursor in try-finally. -4. **B9** — add `this.trackId = trackId` in `Track.setTrackId()`. -5. **B6** — add `volatile` to `currentSegmentId`. -6. **B7** — replace `link.equals("null")` with `"null".equals(link)`, remove try-catch. -7. **B8** — switch to `ThreadLocal` or `DateTimeFormatter`. From 4a10b405c17f873193b9b9c93d349c87de2d4a22 Mon Sep 17 00:00:00 2001 From: Milton Barrera Date: Sun, 29 Mar 2026 00:37:12 -0600 Subject: [PATCH 5/5] removing testing md --- docs/TESTING.md | 403 ------------------------------------------------ 1 file changed, 403 deletions(-) delete mode 100644 docs/TESTING.md diff --git a/docs/TESTING.md b/docs/TESTING.md deleted file mode 100644 index fc6518d8..00000000 --- a/docs/TESTING.md +++ /dev/null @@ -1,403 +0,0 @@ -# Testing Guide — OSMTracker Android - -This document explains how to run the test suite, generate a code-coverage report, and -interpret the coverage results. - ---- - -## Table of Contents - -1. [Prerequisites](#1-prerequisites) -2. [Test types at a glance](#2-test-types-at-a-glance) -3. [Running unit tests](#3-running-unit-tests) -4. [Running a single test class or method](#4-running-a-single-test-class-or-method) -5. [Running instrumented tests](#5-running-instrumented-tests) -6. [Generating the coverage report](#6-generating-the-coverage-report) -7. [Reading the coverage report](#7-reading-the-coverage-report) -8. [Understanding coverage percentages](#8-understanding-coverage-percentages) -9. [Current coverage baseline](#9-current-coverage-baseline) -10. [Troubleshooting](#10-troubleshooting) - ---- - -## 1. Prerequisites - -| Tool | Required version | Notes | -|------|-----------------|-------| -| **JDK** | 17 (recommended) | CI uses Eclipse Temurin 17. JDK 24 is **not** compatible with Gradle 8.11.1 (see [Troubleshooting](#10-troubleshooting)). | -| **Android SDK** | API 25–35 | Set `sdk.dir` in `local.properties`. | -| **Gradle wrapper** | 8.11.1 | Use `./gradlew` — do **not** install Gradle manually. | - -> **No emulator or physical device is needed to run unit tests.** -> Instrumented tests (Section 5) do require one. - ---- - -## 2. Test types at a glance - -| Type | Framework | Location | Device needed? | -|------|-----------|----------|---------------| -| **Unit tests** | Robolectric + Mockito | `app/src/test/` | No | -| **Instrumented tests** | Espresso | `app/src/androidTest/` | Yes (API ≥ 26) | - -Unit tests use [Robolectric](https://robolectric.org/) to simulate the Android framework -in the JVM, so they run fast and require no connected hardware. - ---- - -## 3. Running unit tests - -From the **project root directory**, run: - -```bash -./gradlew testDebugUnitTest -``` - -Gradle compiles the sources, runs all unit tests, and prints a summary: - -``` -BUILD SUCCESSFUL in 1m 42s -``` - -If any test fails, Gradle prints the failing test name and the assertion error, then exits -with a non-zero code (`BUILD FAILED`). - -### Where results are written - -| Artifact | Path | -|----------|------| -| XML results (machine-readable) | `app/build/test-results/testDebugUnitTest/TEST-*.xml` | -| HTML results (human-readable) | `app/build/reports/tests/testDebugUnitTest/index.html` | - -Open the HTML report in a browser for a navigable view of all test outcomes: - -```bash -xdg-open app/build/reports/tests/testDebugUnitTest/index.html # Linux -open app/build/reports/tests/testDebugUnitTest/index.html # macOS -``` - ---- - -## 4. Running a single test class or method - -### Run one test class - -```bash -./gradlew testDebugUnitTest --tests "net.osmtracker.db.DataHelperTest" -./gradlew testDebugUnitTest --tests "net.osmtracker.service.gps.GPSLoggerTest" -``` - -### Run one specific test method - -```bash -./gradlew testDebugUnitTest \ - --tests "net.osmtracker.db.DataHelperTest.E1_wayPoint_throwsNPEWhenLocationIsNull" -``` - -### Run multiple classes at once - -```bash -./gradlew testDebugUnitTest \ - --tests "net.osmtracker.db.DataHelperTest" \ - --tests "net.osmtracker.service.gps.GPSLoggerTest" -``` - -The `--tests` flag supports wildcards: - -```bash -# All tests in the db package -./gradlew testDebugUnitTest --tests "net.osmtracker.db.*" -``` - ---- - -## 5. Running instrumented tests - -Instrumented tests require a **running emulator** or **physical device** with API level -≥ 26 connected via ADB. - -### Start an emulator (command line) - -```bash -# List available AVDs -$ANDROID_HOME/emulator/emulator -list-avds - -# Start one (replace NAME with an AVD name from the list above) -$ANDROID_HOME/emulator/emulator -avd NAME -``` - -Wait until the emulator is fully booted, then run: - -```bash -./gradlew connectedCheck -``` - -CI runs connected tests against API 26 using the -[android-emulator-runner](https://github.com/ReactiveCircus/android-emulator-runner) -GitHub Action. - ---- - -## 6. Generating the coverage report - -The project uses **JaCoCo** for coverage. The task `jacocoTestReport` depends on -`testDebugUnitTest`, so running the report task also runs all unit tests: - -```bash -./gradlew testDebugUnitTest jacocoTestReport -``` - -Or simply: - -```bash -./gradlew jacocoTestReport # runs tests first automatically -``` - -### Where the report is written - -| Format | Path | -|--------|------| -| **XML** (for CI / Coveralls) | `app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml` | -| **HTML** (for local reading) | `app/build/reports/jacoco/jacocoTestReport/html/index.html` | - -> The XML report is what the CI pipeline uploads to -> [Coveralls](https://coveralls.io/) for the badge on the README. - -Open the HTML report: - -```bash -xdg-open app/build/reports/jacoco/jacocoTestReport/html/index.html # Linux -open app/build/reports/jacoco/jacocoTestReport/html/index.html # macOS -``` - ---- - -## 7. Reading the coverage report - -The HTML report is a navigable tree that mirrors the package structure. - -``` -index.html -└── net.osmtracker - ├── db/ - │ ├── DataHelper ← click to see line-by-line coverage - │ ├── TrackContentProvider - │ └── model/ - │ ├── Track - │ ├── WayPoint - │ └── TrackPoint - ├── service/gps/ - │ └── GPSLogger - └── gpx/ - └── ExportTrackTask -``` - -Each row shows four coverage metrics for the class: - -| Column | What it measures | -|--------|-----------------| -| **Missed Instructions** | Individual JVM bytecode instructions not executed | -| **Cov.** (Coverage %) | Percentage of instructions that were executed | -| **Missed Branches** | `if`/`switch` branches not taken | -| **Branch Cov.** | Percentage of branches exercised | -| **Missed Lines** | Source lines with no covered instruction | -| **Missed Methods** | Methods never called during tests | -| **Missed Classes** | Classes with zero covered instructions | - -### Line colours in source view - -Click any class name to open the line-by-line annotated source: - -| Colour | Meaning | -|--------|---------| -| **Green** | Line fully covered (all branches taken) | -| **Yellow** | Line partially covered (some branches missed) | -| **Red** | Line not covered at all | -| No colour | Not executable (comments, blank lines, declarations) | - ---- - -## 8. Understanding coverage percentages - -### What the percentage means - -``` -Instruction coverage = (executed instructions) / (total instructions) × 100 -``` - -A 70 % instruction coverage means 30 % of the compiled bytecode was never reached by any -test. Higher is better, but **coverage alone does not measure test quality** — a test can -execute a line without asserting anything meaningful about its result. - -### Meaningful thresholds (industry guidance) - -| Coverage | Interpretation | -|----------|---------------| -| < 40 % | Low — critical paths likely untested | -| 40 – 60 % | Moderate — main flows covered, edge cases missing | -| 60 – 80 % | Good — most paths exercised | -| > 80 % | High — edge cases and error paths covered | -| 100 % | Theoretical maximum — rarely practical or necessary | - -> Prioritise covering **error paths** (null inputs, missing records, provider failures) -> over trivially-exercised happy paths. The bugs catalogued in -> [`docs/BUGS.md`](BUGS.md) were all in code that had 0 % coverage before the -> `new-test-cases` branch. - -### Extracting the percentage from the XML report - -The XML report lists coverage per package, class, and method. To extract the overall -instruction coverage quickly: - -```bash -# Overall instruction coverage from the XML -python3 - << 'EOF' -import xml.etree.ElementTree as ET -tree = ET.parse("app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml") -root = tree.getroot() -for counter in root.findall("counter"): - if counter.attrib["type"] == "INSTRUCTION": - missed = int(counter.attrib["missed"]) - covered = int(counter.attrib["covered"]) - total = missed + covered - pct = covered / total * 100 - print(f"Overall instruction coverage: {pct:.1f}% ({covered}/{total})") -EOF -``` - -Or use `grep` for a quick look: - -```bash -grep -o 'type="INSTRUCTION"[^/]*/>' \ - app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml | tail -1 -``` - ---- - -## 9. Current coverage baseline - -The table below shows the coverage of the two modules targeted by the `new-test-cases` -audit (branch `new-test-cases`). Numbers are approximate and will change as more tests -are added or bugs are fixed. - -| Module / Class | Before audit | After `new-test-cases` | Notes | -|----------------|-------------|------------------------|-------| -| `DataHelper` | ~0 % | ~65 % | Groups A–H in `DataHelperTest` | -| `GPSLogger` | ~0 % | ~55 % | Groups I–L in `GPSLoggerTest` | -| Overall project | ~30 % | ~40 % | Existing tests + new tests | - -> Coverage will improve further once the bugs documented in `docs/BUGS.md` are fixed, -> because several `assertThrows` tests will be rewritten as positive assertions that -> exercise the now-correct code paths. - -### Test file map - -| Test class | Covers | # Tests | -|------------|--------|---------| -| `DataHelperTest` | `DataHelper` (all public methods) | 23 | -| `GPSLoggerTest` | `GPSLogger` (lifecycle, receiver, location) | 13 | -| `DataHelperNoteTest` | `DataHelper.trackNote`, `deleteNote` | 1 | -| `ExportToStorageTaskTest` | `ExportToStorageTask` GPX output | 9 | -| `ExportToTempFileTaskTest` | `ExportToTempFileTask` | 1 | -| `TrackTest` | `Track.build()` | 1 | -| `OSMVisibilityTest` | `Track.OSMVisibility` | 4 | -| `MercatorProjectionTest` | `MercatorProjection` util | 10 | -| `FileSystemUtilsTest` | `FileSystemUtils` | 10 | -| `ArrayUtilsTest` | `ArrayUtils` | 4 | -| `URLCreatorTest` | `URLCreator` | 5 | -| `CustomLayoutsUtilsTest` | `CustomLayoutsUtils` | 6 | -| `ThemeValidatorTest` | `ThemeValidator` | 2 | -| `ButtonsPresetsTest` | `ButtonsPresets` activity | 3 | -| `TrackDetailEditorTest` | `TrackDetailEditor` activity | 2 | -| `OpenStreetMapNotesUploadTest` | `OpenStreetMapNotesUpload` | 2 | -| `URLValidatorTaskTest` | `URLValidatorTask` | 1 | -| `DownloadCustomLayoutTaskTest` | `DownloadCustomLayoutTask` | 1 | -| **Total** | | **98** | - ---- - -## 10. Troubleshooting - -### `[JAVA_COMPILER] not present` error - -``` -Toolchain installation '/usr/lib/jvm/java-21-openjdk' does not provide the required -capabilities: [JAVA_COMPILER] -``` - -**Cause:** The system has a JRE (runtime only) instead of a full JDK (which includes -`javac`). Gradle needs `javac` to compile the project. - -**Fix (local machines):** Tell Gradle which JDK to use by adding one line to -`gradle.properties` in the project root (do **not** commit this file change): - -```properties -# gradle.properties ← add this line, never commit it -org.gradle.java.home=/path/to/your/jdk17 -``` - -Common locations: - -| Environment | Path | -|------------|------| -| Android Studio bundled JBR (Linux Flatpak) | `/var/lib/flatpak/app/com.google.AndroidStudio/.../files/extra/jbr` | -| SDKMAN JDK 17 | `~/.sdkman/candidates/java/17.0.x-tem` | -| Homebrew (macOS) | `/opt/homebrew/opt/openjdk@17` | -| CI (GitHub Actions) | Handled automatically by `actions/setup-java@v4` | - -To find the exact path on your machine: - -```bash -# Linux -find /var /usr /opt /home -name "javac" 2>/dev/null | grep -v "build/" - -# macOS -/usr/libexec/java_home -v 17 -``` - -### `Type T not present` error with JDK 24 - -``` -Could not create an instance of type org.gradle.api.internal.tasks.testing.DefaultTestTaskReports. - > Type T not present -``` - -**Cause:** JDK 24 is not compatible with Android Gradle Plugin 8.7.3 / Gradle 8.11.1. - -**Fix:** Use JDK 17 or 21. See the table above. - -### Tests compile but none run - -``` -BUILD SUCCESSFUL (0 tests executed) -``` - -**Cause:** Gradle cached a previous run. Force re-execution: - -```bash -./gradlew testDebugUnitTest --rerun-tasks -``` - -### Out of memory during test run - -```bash -./gradlew testDebugUnitTest \ - -Dorg.gradle.jvmargs="-Xmx4g -XX:MaxMetaspaceSize=1g" -``` - -Or add permanently to `gradle.properties`: - -```properties -org.gradle.jvmargs=-Xmx4g -XX:MaxMetaspaceSize=1g -``` - -### Coverage report is empty / 0 % - -Ensure you run both tasks together: - -```bash -./gradlew testDebugUnitTest jacocoTestReport -``` - -Running `jacocoTestReport` alone (without fresh test execution data) produces an empty -report because JaCoCo needs the `.exec` file generated during the test run.