SettingsNavigationTest: googleBooksApiKeyRow_saveAndClearAreWiredToRealCallbacks Fails Due to Race Between waitForIdle() and Async Room/Flow Propagation

Summary

The instrumented test SettingsNavigationTest > googleBooksApiKeyRow_saveAndClearAreWiredToRealCallbacks fails on physical devices running Android 12 against a clean main branch. The AssertionError is thrown from the finally cleanup block, which masks the original failure site and makes the true root cause ambiguous between two candidates: a mid-test assertion failure (with the finally throw suppressing it) or a genuine timing race between waitForIdle() returning and the status text catching up to an async Room write plus Flow emission. Either path risks leaving a bogus API key persisted in the device's app_settings database after the run.

Root-Cause Analysis

Confirmed Evidence

• The AssertionError is thrown at SettingsNavigationTest.kt:118, which is inside the finally block, asserting 'No key saved' is displayed.
• The test was run against an unmodified main branch on a physical device (SM-G975U1, Android 12); 3 of 4 tests in the class passed.
• The test's own KDoc documents that the status line is asserted rather than the text field precisely because the status line is downstream of a real write to app_settings and a subsequent Flow emission through the repository.

Reasonable Inference — Candidate A: Timing Race (Most Likely)

waitForIdle() synchronizes Compose's recomposition queue and the main-thread message queue. It does not wait for:

1. A Room write to complete on its I/O dispatcher.
2. The resulting Flow to emit on its collection coroutine.
3. The StateFlow or collectAsState in the composable to trigger a recomposition.

The sequence after Clear key is clicked is approximately:

UI click → ViewModel call → Room write (IO thread) → Flow emit → StateFlow update → Compose recomposition → text change

waitForIdle() can return after step 1 is processed on the main thread, while steps 2–7 are still in flight. The subsequent bare assert then observes stale text and throws. Because this throw happens in finally, JUnit discards the original exception (if any) from the try block and surfaces only the finally exception — making both the true failure site and the true failure reason invisible.

Reasonable Inference — Candidate B: try-Block Failure with Masked Exception

If the try block itself threw an AssertionError (e.g., the "Save" path failed and the status did not update to reflect the saved key), the finally block would still execute. Any throw from finally replaces the original Throwable, and standard JUnit/test-runner reporting shows only the finally exception. The original failure is then silently suppressed. This would mean the feature is broken rather than the test being flaky.

Evidence Insufficient to Confirm Either Candidate

The current failure report cannot distinguish Candidate A from Candidate B because:

• The suppressed exception is not captured or logged.
• There is no intermediate assertion between the "Save" click and the "Clear" click to confirm the save path succeeded independently.
• The test failure is intermittent enough (or device-specific enough) that it has not been reproduced with additional diagnostic instrumentation.

Additional evidence required: Capture the suppressed exception explicitly (see Resolution Steps) and re-run to determine whether the try block was healthy before the finally throw.

Side-Effect Risk

If the failure occurs during the "Save" or "Clear" click path itself (rather than in the assertion), the finally block may have executed but its own assertion failure does not confirm the clear actually succeeded. test-key-not-a-real-one may remain persisted in app_settings on that device installation, causing subsequent real Google Books lookups to use the bogus key until the app is reinstalled or the key is manually cleared.

Resolution Steps

Address in order of priority:

1. Disambiguate the failure site. Refactor the try/finally so that the suppressed exception is not silently discarded. Use Kotlin's runCatching or split the block with an explicit intermediate variable to capture the original failure, then rethrow it with the finally exception added as a suppressed cause.

1. Replace waitForIdle() + bare assert with waitUntil on the target condition. Compose's test API provides waitUntil specifically to handle cases where recomposition is driven by asynchronous state. Replace the pattern:

   composeRule.waitForIdle()
   composeRule.onNodeWithText("No key saved").assertIsDisplayed()

with a condition-based wait (see CLI Commands / Configuration Snippets).

1. Move the cleanup to an @After method. Any assertion that executes inside finally can itself fail and leave side effects. Database cleanup (clearing the API key) is a teardown concern and belongs in a JUnit @After function, which the test runner guarantees will execute regardless of how the test body terminates. This separates the assertion contract from the cleanup contract.

1. After fixing the timing, verify device state. On any device where this test has already failed in its current form, manually navigate to the Settings screen and confirm the Google Books API key field shows No key saved. If it shows a saved value, clear it there or reinstall the debug build.

1. Add the status assertion to the "Save" path as well. Insert an intermediate assertion after clicking "Save" to confirm the status line updated to reflect the saved key before proceeding to "Clear". This makes the two halves independently observable and prevents one half's failure from masking the other's.

CLI Commands

Run only the failing test class to reproduce:

./gradlew :app:connectedDebugAndroidTest \
  -Pandroid.testInstrumentationRunnerArguments.class=com.github.maskedkunisquat.mediatracker.ui.SettingsNavigationTest

Run only the specific failing method:

./gradlew :app:connectedDebugAndroidTest \
  "-Pandroid.testInstrumentationRunnerArguments.class=com.github.maskedkunisquat.mediatracker.ui.SettingsNavigationTest#googleBooksApiKeyRow_saveAndClearAreWiredToRealCallbacks"

Confirm the persisted key state via ADB after a failing run:

# Inspect app_settings database to verify the key was or was not cleared.
adb shell run-as com.github.maskedkunisquat.mediatracker \
  sqlite3 databases/app_settings.db \
  "SELECT * FROM settings WHERE key='google_books_api_key';"

> Replace app_settings.db and the table/column names with those matching the actual Room schema if they differ.

Configuration Snippets

Replace the racy waitForIdle + assert pattern:

// Before — races against async Room write + Flow emission
composeRule.waitForIdle()
composeRule.onNodeWithText("No key saved").assertIsDisplayed()

// After — polls until the condition is true or the timeout elapses
composeRule.waitUntil(timeoutMillis = 5_000L) {
    composeRule
        .onAllNodesWithText("No key saved", substring = true)
        .fetchSemanticsNodes()
        .isNotEmpty()
}
composeRule.onNodeWithText("No key saved", substring = true).assertIsDisplayed()

Move cleanup to @After and capture suppressed exceptions in the test body:

@After
fun clearApiKey() {
    // Unconditionally clear the key so no bogus credential survives a test failure.
    // This replaces the finally-block clear inside the test body.
    composeRule.onNodeWithContentDescription("Clear key").performClick()
    composeRule.waitUntil(timeoutMillis = 5_000L) {
        composeRule
            .onAllNodesWithText("No key saved", substring = true)
            .fetchSemanticsNodes()
            .isNotEmpty()
    }
}

@Test
fun googleBooksApiKeyRow_saveAndClearAreWiredToRealCallbacks() {
    // try body only — no finally needed; @After handles cleanup unconditionally
    composeRule.onNodeWithContentDescription("API key field").performTextInput("test-key-not-a-real-one")
    composeRule.onNodeWithContentDescription("Save key").performClick()

    // Assert the save path independently before proceeding to clear
    composeRule.waitUntil(timeoutMillis = 5_000L) {
        composeRule
            .onAllNodesWithText("Key saved", substring = true)
            .fetchSemanticsNodes()
            .isNotEmpty()
    }
    composeRule.onNodeWithText("Key saved", substring = true).assertIsDisplayed()
}

> Adjust content descriptions, node matchers, and status strings to match the actual composable implementation.

Verification

1. Re-run the isolated test method after applying the waitUntil change. It should pass consistently across multiple consecutive runs on the same device.

1. Confirm no suppressed exception surfaces. With the test body restructured and cleanup in @After, any failure in the save path or the clear path will be reported as a first-class exception, not masked.

1. Inspect the database after a passing run:

adb shell run-as com.github.maskedkunisquat.mediatracker \
  sqlite3 databases/app_settings.db \
  "SELECT * FROM settings WHERE key='google_books_api_key';"

Expected result after a clean pass: the row is absent or its value is empty/null, confirming the @After teardown cleared the persisted key.

1. Run the full SettingsNavigationTest class to confirm the other three tests remain green.

./gradlew :app:connectedDebugAndroidTest \
  -Pandroid.testInstrumentationRunnerArguments.class=com.github.maskedkunisquat.mediatracker.ui.SettingsNavigationTest

Expected: 4 passed, 0 failed.

1. Rollback indicator. If the test still fails after waitUntil is applied, the timeout expiry will surface a more informative error than the current bare AssertionError. A persistent failure after a 5-second waitUntil indicates Candidate B (feature broken, not flaky test) and warrants investigation of the ViewModel/repository write path.

Prevention

• Do not use waitForIdle() to gate assertions on state driven by off-main-thread work (Room I/O, coroutine dispatchers, Flow emissions). Document this constraint in the project's instrumented-test authoring guide or AGENTS.md.

• Enforce @After for all database/preference teardown in instrumented tests. Add a lint rule or code-review checklist item that flags finally blocks in test functions that perform state cleanup — this is a structural antipattern that masks failures.

• Add the connectedDebugAndroidTest suite to CI on at least a scheduled basis using an Android emulator (e.g., macos-latest GitHub Actions runner with AVD cache, or Firebase Test Lab). Keeping this suite invisible to CI allows regressions to accumulate silently and normalizes ignoring red device runs, exactly the habit AGENTS.md §7 flags.

• Consider idling resources for Room. If the project uses CountingIdlingResource or integrates RoomIdlingResource, Compose's test infrastructure can automatically wait for Room writes to settle before returning from synchronization primitives, eliminating this class of race entirely.

• Add a status assertion after each distinct user action (save, clear) as a test-structure convention. Intermediate assertions make each step independently verifiable and prevent a failure in step N from appearing as a failure in step N+1.