test(#291): add coverage for 3 critical untested components (#300)

* test(#291): add coverage for 3 critical untested components - NavigationController dedup guard (line 45): 14 pure unit tests for navigateTo, resetTo, goBack, primary-screen clearing, and null guards. - NotificationReplyReceiver: 11 MockK-based tests covering the full WS+Room+notification flow: valid reply sends WS message, persists to Room with correct entity shape, shows replied confirmation notification, finishes PendingResult, and gracefully handles null/blank inputs and Room failures. - ChatNotificationService isAppInForeground flag: 5 tests verifying companion object toggle, volatile modifier (thread safety), default false state, and idempotent calls. - Fix: add @Volatile to isAppInForeground (was a plain var read from background coroutine and written from UI thread) Closes #291 * fix(#291): fix MockK type errors in test compilation * fix(#291): fix test assertions and goAsync mock for CI * fix(#291): wrap final BroadcastReceiver.goAsync() with open Kotlin wrapper * chore: trigger CI on latest fix commit * fix(#291): make NotificationReplyReceiver open so tests can subclass it * fix(#291): extract buildReplyNotification() to avoid 'not mocked' framework error NotificationCompat.Builder.build() internally calls android.app.Notification.Builder methods (e.g. setWhen) that throw RuntimeException with 'Method setWhen in android.app.Notification.Builder not mocked' in unit tests — the standard Android unit test environment does not provide real framework implementations. Extracting notification creation into an open method (same pattern as goAsyncCompat()) lets tests override it to return a mock Notification, avoiding the framework call entirely. * style: fix ktlint blank-line-before-declaration in test --------- Co-authored-by: M57 <hy4ri@users.noreply.github.com>
2026-06-22 06:49:19 +03:00
1 parent bbe3b00ccb
commit c44f4c3068
@@ -0,0 +1,229 @@
+package com.m57.hermescontrol
+
+import androidx.navigation3.runtime.NavBackStack
+import androidx.navigation3.runtime.NavKey
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+
+/**
+ * Unit tests for [NavigationController] — the central navigation guard that
+ * prevents duplicate screen entries on the back stack.
+ *
+ * Issue #291 (Critical Test Coverage): Verifies the deduplication logic at
+ * line 45 (`if (stack.lastOrNull() == key) return`) and the primary-screen
+ * stack-clearing behavior at line 47-49.
+ *
+ * All tests are pure Kotlin with no Android dependencies — they only exercise
+ * [NavigationController]'s companion object methods against a real
+ * [NavBackStack] instance.
+ */
+class NavigationControllerTest {
+    @Before
+    fun setUp() {
+        // Start fresh — no pinned back stack from a previous test
+        NavigationController.backStack = null
+    }
+
+    @After
+    fun tearDown() {
+        NavigationController.backStack = null
+    }
+
+    // ── Dedup guard: navigateTo with same key ──────────────────────────────
+
+    @Test
+    fun `navigateTo with null backStack does nothing`() {
+        NavigationController.backStack = null
+        NavigationController.navigateTo(ChatScreen)
+        assertNull("backStack should remain null when not initialised", NavigationController.backStack)
+    }
+
+    @Test
+    fun `navigateTo on same primary screen is a no-op`() {
+        val backStack = NavBackStack<NavKey>(SkillsScreen)
+        NavigationController.backStack = backStack
+        val sizeBefore = backStack.size
+
+        NavigationController.navigateTo(SkillsScreen)
+
+        assertEquals("stack size should not change when navigating to the same screen", sizeBefore, backStack.size)
+        assertEquals("top of stack should still be SkillsScreen", SkillsScreen, backStack.lastOrNull())
+    }
+
+    @Test
+    fun `navigateTo on same non-primary screen is a no-op`() {
+        val backStack = NavBackStack<NavKey>(ProfilesScreen)
+        NavigationController.backStack = backStack
+
+        // ProfilesScreen is NOT in the default primaryScreens so it stays on the stack
+        NavigationController.navigateTo(ProfilesScreen)
+
+        assertEquals(1, backStack.size)
+        assertEquals(ProfilesScreen, backStack.lastOrNull())
+    }
+
+    // ── Primary-screen behaviour: stack clearing ──────────────────────────
+
+    @Test
+    fun `navigateTo on a different primary screen clears the stack`() {
+        val backStack = NavBackStack<NavKey>(ChatScreen)
+        NavigationController.backStack = backStack
+
+        // Navigate to a non-primary screen first (should add to stack)
+        NavigationController.navigateTo(LogsScreen)
+        assertEquals(2, backStack.size)
+
+        // Now navigate to a different primary screen — must clear
+        NavigationController.navigateTo(SkillsScreen)
+
+        assertEquals("primary screen navigation should clear the stack", 1, backStack.size)
+        assertEquals(SkillsScreen, backStack.lastOrNull())
+    }
+
+    @Test
+    fun `navigateTo on the current primary screen clears the stack`() {
+        val backStack = NavBackStack<NavKey>(ChatScreen)
+        NavigationController.backStack = backStack
+
+        // Add a non-primary screen
+        NavigationController.navigateTo(ProfilesScreen)
+        assertEquals(2, backStack.size)
+
+        // Navigate to ChatScreen — it's a primary screen, so the stack gets
+        // cleared before adding it. The dedup guard only fires when the key
+        // is already the LAST item (ChatScreen is at index 0, ProfilesScreen
+        // is last), not when it's merely present somewhere in the stack.
+        NavigationController.navigateTo(ChatScreen)
+
+        // Primary screen navigation clears the stack
+        assertEquals("primary navigation clears stack", 1, backStack.size)
+        assertEquals(ChatScreen, backStack.lastOrNull())
+    }
+
+    // ── Non-primary screen behaviour: stack appending ─────────────────────
+
+    @Test
+    fun `navigateTo on a non-primary screen appends to the stack`() {
+        val backStack = NavBackStack<NavKey>(ChatScreen)
+        NavigationController.backStack = backStack
+
+        NavigationController.navigateTo(ProfilesScreen)
+        assertEquals(2, backStack.size)
+        assertEquals(ProfilesScreen, backStack.lastOrNull())
+
+        NavigationController.navigateTo(KeysScreen)
+        assertEquals(3, backStack.size)
+        assertEquals(KeysScreen, backStack.lastOrNull())
+    }
+
+    // ── resetTo: atomic clear + navigate ──────────────────────────────────
+
+    @Test
+    fun `resetTo clears the stack and sets the target screen`() {
+        val backStack = NavBackStack<NavKey>(ChatScreen)
+        NavigationController.backStack = backStack
+
+        NavigationController.navigateTo(ProfilesScreen)
+        NavigationController.navigateTo(KeysScreen)
+        assertEquals(3, backStack.size)
+
+        NavigationController.resetTo(SettingsScreen)
+
+        assertEquals(1, backStack.size)
+        assertEquals(SettingsScreen, backStack.lastOrNull())
+    }
+
+    @Test
+    fun `resetTo with null backStack does nothing`() {
+        NavigationController.backStack = null
+        NavigationController.resetTo(ChatScreen)
+
+        assertNull(NavigationController.backStack)
+    }
+
+    // ── goBack: never leave the stack empty ───────────────────────────────
+
+    @Test
+    fun `goBack removes the top screen when stack has more than one`() {
+        val backStack = NavBackStack<NavKey>(ChatScreen)
+        NavigationController.backStack = backStack
+        NavigationController.navigateTo(ProfilesScreen)
+        assertEquals(2, backStack.size)
+
+        NavigationController.goBack()
+
+        assertEquals(1, backStack.size)
+        assertEquals(ChatScreen, backStack.lastOrNull())
+    }
+
+    @Test
+    fun `goBack falls back to default screen when stack has one item`() {
+        val backStack = NavBackStack<NavKey>(ProfilesScreen)
+        NavigationController.backStack = backStack
+
+        NavigationController.goBack()
+
+        assertEquals(1, backStack.size)
+        assertEquals("default fallback should be ChatScreen", ChatScreen, backStack.lastOrNull())
+    }
+
+    @Test
+    fun `goBack with custom fallback uses the given screen`() {
+        val backStack = NavBackStack<NavKey>(ProfilesScreen)
+        NavigationController.backStack = backStack
+
+        NavigationController.goBack(fallback = SkillsScreen)
+
+        assertEquals(1, backStack.size)
+        assertEquals(SkillsScreen, backStack.lastOrNull())
+    }
+
+    @Test
+    fun `goBack with null backStack does nothing`() {
+        NavigationController.backStack = null
+        NavigationController.goBack()
+
+        assertNull(NavigationController.backStack)
+    }
+
+    // ── primaryScreens and updatePrimaryScreens ───────────────────────────
+
+    @Test
+    fun `isPrimaryScreen returns true for default screens`() {
+        assertTrue("ChatScreen should be primary by default", NavigationController.isPrimaryScreen(ChatScreen))
+        assertTrue("SkillsScreen should be primary by default", NavigationController.isPrimaryScreen(SkillsScreen))
+        assertTrue("CronJobsScreen should be primary by default", NavigationController.isPrimaryScreen(CronJobsScreen))
+        assertTrue("SystemScreen should be primary by default", NavigationController.isPrimaryScreen(SystemScreen))
+        assertTrue("SettingsScreen should be primary by default", NavigationController.isPrimaryScreen(SettingsScreen))
+    }
+
+    @Test
+    fun `isPrimaryScreen returns false for non-default screens`() {
+        assertFalse("ProfilesScreen should NOT be primary", NavigationController.isPrimaryScreen(ProfilesScreen))
+        assertFalse("LogsScreen should NOT be primary", NavigationController.isPrimaryScreen(LogsScreen))
+        assertFalse("ConfigScreen should NOT be primary", NavigationController.isPrimaryScreen(ConfigScreen))
+    }
+
+    @Test
+    fun `updatePrimaryScreens replaces the full screen set`() {
+        NavigationController.updatePrimaryScreens(setOf(ProfilesScreen, SystemScreen))
+
+        assertTrue("ProfilesScreen should now be primary", NavigationController.isPrimaryScreen(ProfilesScreen))
+        assertTrue("SystemScreen should still be primary", NavigationController.isPrimaryScreen(SystemScreen))
+
+        // Old defaults should no longer be primary
+        assertFalse("ChatScreen should no longer be primary", NavigationController.isPrimaryScreen(ChatScreen))
+        assertFalse("SkillsScreen should no longer be primary", NavigationController.isPrimaryScreen(SkillsScreen))
+        assertFalse("SettingsScreen should no longer be primary", NavigationController.isPrimaryScreen(SettingsScreen))
+
+        // Reset to defaults to avoid test pollution
+        NavigationController.updatePrimaryScreens(
+            setOf(ChatScreen, SkillsScreen, CronJobsScreen, SystemScreen, SettingsScreen),
+        )
+    }
+}
@@ -0,0 +1,99 @@
+package com.m57.hermescontrol.notification
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+import java.lang.reflect.Modifier
+
+/**
+ * Unit tests for [ChatNotificationService] companion object — specifically
+ * the `isAppInForeground` flag that gates notification display.
+ *
+ * Issue #291 (Critical Test Coverage): Verifies that `setAppForeground`
+ * correctly toggles the flag and that the field is `@Volatile` for thread
+ * safety (it is read from the event-collection coroutine on a background
+ * dispatcher while `setAppForeground` is called from the UI thread).
+ *
+ * These are pure unit tests with no Android dependencies — the companion
+ * object can be exercised directly via reflection for the private flag
+ * or through the public `setAppForeground` API.
+ */
+class ChatNotificationServiceTest {
+    @Test
+    fun `setAppForeground true sets the flag`() {
+        val field =
+            ChatNotificationService::class.java
+                .getDeclaredField("isAppInForeground")
+        field.isAccessible = true
+
+        // Reset to known state
+        field.setBoolean(null, false)
+        ChatNotificationService.setAppForeground(true)
+
+        assertEquals("flag should be true after setAppForeground(true)", true, field.getBoolean(null))
+    }
+
+    @Test
+    fun `setAppForeground false clears the flag`() {
+        val field =
+            ChatNotificationService::class.java
+                .getDeclaredField("isAppInForeground")
+        field.isAccessible = true
+
+        // Set to known state
+        field.setBoolean(null, true)
+        ChatNotificationService.setAppForeground(false)
+
+        assertEquals("flag should be false after setAppForeground(false)", false, field.getBoolean(null))
+    }
+
+    @Test
+    fun `isAppInForeground defaults to false`() {
+        val field =
+            ChatNotificationService::class.java
+                .getDeclaredField("isAppInForeground")
+        field.isAccessible = true
+
+        // Ensure it's in a clean state
+        field.setBoolean(null, false)
+
+        assertEquals("initial value should be false", false, field.getBoolean(null))
+    }
+
+    @Test
+    fun `isAppInForeground field is volatile for thread safety`() {
+        val field =
+            ChatNotificationService::class.java
+                .getDeclaredField("isAppInForeground")
+        val modifiers = field.modifiers
+
+        assertEquals(
+            "isAppInForeground must be @Volatile — it is read from a background " +
+                "coroutine (event collection) and written from the UI thread " +
+                "(setAppForeground called from ChatScreen lifecycle)",
+            true,
+            Modifier.isVolatile(modifiers),
+        )
+    }
+
+    @Test
+    fun `setAppForeground is idempotent when called multiple times`() {
+        val field =
+            ChatNotificationService::class.java
+                .getDeclaredField("isAppInForeground")
+        field.isAccessible = true
+
+        // Set true twice
+        ChatNotificationService.setAppForeground(true)
+        ChatNotificationService.setAppForeground(true)
+        assertEquals("flag should remain true after consecutive setAppForeground(true)", true, field.getBoolean(null))
+
+        // Set false twice
+        ChatNotificationService.setAppForeground(false)
+        ChatNotificationService.setAppForeground(false)
+        assertEquals(
+            "flag should remain false after consecutive setAppForeground(false)",
+            false,
+            field.getBoolean(null),
+        )
+    }
+}
@@ -0,0 +1,277 @@
+package com.m57.hermescontrol.notification
+
+import android.app.Notification
+import android.app.NotificationManager
+import android.content.BroadcastReceiver
+import android.content.Context
+import android.content.Intent
+import androidx.core.app.RemoteInput
+import com.m57.hermescontrol.data.local.ChatMessageDao
+import com.m57.hermescontrol.data.local.ChatMessageEntity
+import com.m57.hermescontrol.data.local.HermesDatabase
+import com.m57.hermescontrol.data.ws.HermesWsClient
+import io.mockk.coEvery
+import io.mockk.every
+import io.mockk.mockk
+import io.mockk.mockkObject
+import io.mockk.mockkStatic
+import io.mockk.slot
+import io.mockk.unmockkAll
+import io.mockk.verify
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+
+/**
+ * Unit tests for [NotificationReplyReceiver] — the BroadcastReceiver that
+ * handles inline reply actions on chat notifications.
+ *
+ * Issue #291 (Critical Test Coverage): Verifies the full flow —
+ *   (a) WS message is sent via [HermesWsClient.sendMessage]
+ *   (b) Reply is persisted to Room via [ChatMessageDao.upsert]
+ *   (c) A "Replied" confirmation notification is posted
+ *   (d) Invalid/missing input is handled gracefully (no crash)
+ *   (e) Room failures are logged and the pending result is still finished
+ *
+ * Uses [HermesDatabase.setForTest] for deterministic Room mocking and
+ * [spyk] for [BroadcastReceiver.goAsync] interception.
+ *
+ * NOTE: This does NOT use coVerify for suspend functions (incompatible with
+ * MockK 1.13.12). Instead, call counting via coEvery + answers is used.
+ */
+class NotificationReplyReceiverTest {
+    private lateinit var mockContext: Context
+    private lateinit var mockIntent: Intent
+    private lateinit var mockNotificationManager: NotificationManager
+    private lateinit var mockPendingResult: BroadcastReceiver.PendingResult
+    private lateinit var mockDb: HermesDatabase
+    private lateinit var mockDao: ChatMessageDao
+    private lateinit var receiver: NotificationReplyReceiver
+    private lateinit var mockNotification: Notification
+
+    // Call counters for suspend function tracking
+    private var upsertCallCount = 0
+
+    @Before
+    fun setUp() {
+        upsertCallCount = 0
+
+        // Mock Android framework statics (same pattern as HermesWsClientTest)
+        mockkStatic(android.util.Log::class)
+        every { android.util.Log.d(any<String>(), any<String>()) } returns 0
+        every { android.util.Log.i(any<String>(), any<String>()) } returns 0
+        every { android.util.Log.e(any<String>(), any<String>(), any()) } returns 0
+        every { android.util.Log.w(any<String>(), any<String>()) } returns 0
+
+        // Mock RemoteInput static method
+        mockkStatic(RemoteInput::class)
+
+        // Mock Context
+        mockContext = mockk(relaxed = true)
+        mockNotificationManager = mockk(relaxed = true)
+        every { mockContext.getSystemService(Context.NOTIFICATION_SERVICE) } returns mockNotificationManager
+
+        // Mock Intent
+        mockIntent = mockk(relaxed = true)
+
+        // Mock Room DB via its test injection point
+        mockDao = mockk(relaxed = true)
+        coEvery { mockDao.upsert(any<ChatMessageEntity>()) } answers {
+            upsertCallCount++
+            Unit
+        }
+        mockDb = mockk(relaxed = true)
+        every { mockDb.chatMessageDao() } returns mockDao
+        HermesDatabase.setForTest(mockDb)
+
+        // Mock PendingResult
+        mockPendingResult = mockk(relaxed = true)
+
+        // Mock Notification
+        mockNotification = mockk(relaxed = true)
+
+        // Mock HermesWsClient singleton
+        mockkObject(HermesWsClient)
+        every { HermesWsClient.sendMessage(any(), any()) } returns "mock-req-id"
+
+        // Create receiver with overridden goAsyncCompat() and buildReplyNotification()
+        // — BroadcastReceiver.goAsync() is final (Java) and cannot be mocked via spyk,
+        // and NotificationCompat.Builder.build() calls framework methods that throw
+        // "not mocked" in unit tests.
+        receiver =
+            object : NotificationReplyReceiver() {
+                override fun goAsyncCompat(): BroadcastReceiver.PendingResult = mockPendingResult
+
+                override fun buildReplyNotification(context: Context): Notification = mockNotification
+            }
+    }
+
+    @After
+    fun tearDown() {
+        HermesDatabase.setForTest(null)
+        unmockkAll()
+    }
+
+    // ── Happy path: valid reply ────────────────────────────────────────────
+
+    @Test
+    fun `valid reply sends message via WebSocket`() {
+        givenValidReply("session-abc", "Hello")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify { HermesWsClient.sendMessage("session-abc", "Hello") }
+    }
+
+    @Test
+    fun `valid reply persists message to Room`() {
+        val entitySlot = slot<ChatMessageEntity>()
+        coEvery { mockDao.upsert(capture(entitySlot)) } answers {
+            upsertCallCount++
+            Unit
+        }
+
+        givenValidReply("session-abc", "Hello")
+        receiver.onReceive(mockContext, mockIntent)
+        Thread.sleep(500)
+
+        assertEquals("upsert should have been called", 1, upsertCallCount)
+        assertEquals("session-abc", entitySlot.captured.sessionId)
+        assertEquals("Hello", entitySlot.captured.content)
+        assertEquals("USER", entitySlot.captured.role)
+        assertNotNull("entity must have a UUID id", entitySlot.captured.id)
+        assertTrue("id should be a non-empty UUID", entitySlot.captured.id.isNotBlank())
+        assertTrue("timestamp should be positive", entitySlot.captured.timestamp > 0)
+    }
+
+    @Test
+    fun `valid reply posts replied notification`() {
+        givenValidReply("session-abc", "Hello")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify {
+            mockNotificationManager.notify(
+                ChatNotificationService.PENDING_NOTIFICATION_ID,
+                any(),
+            )
+        }
+    }
+
+    @Test
+    fun `valid reply finishes pending result after Room save`() {
+        givenValidReply("session-abc", "Hello")
+
+        receiver.onReceive(mockContext, mockIntent)
+        Thread.sleep(500)
+
+        verify { mockPendingResult.finish() }
+    }
+
+    // ── Boundary / invalid input ──────────────────────────────────────────
+
+    @Test
+    fun `reply with empty text does nothing`() {
+        givenValidReply("session-abc", "   ")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) }
+        assertFalse("upsert should NOT be called for empty reply", upsertCallCount > 0)
+    }
+
+    @Test
+    fun `reply with null text does nothing`() {
+        givenEmptyRemoteInput("session-abc")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) }
+        assertFalse("upsert should NOT be called for null reply", upsertCallCount > 0)
+    }
+
+    @Test
+    fun `reply with null sessionId does nothing`() {
+        givenValidReply(null, "Hello")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) }
+        assertFalse("upsert should NOT be called for null sessionId", upsertCallCount > 0)
+    }
+
+    @Test
+    fun `reply with empty sessionId does nothing`() {
+        givenValidReply("", "Hello")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) }
+        assertFalse("upsert should NOT be called for empty sessionId", upsertCallCount > 0)
+    }
+
+    @Test
+    fun `reply with blank sessionId does nothing`() {
+        givenValidReply("   ", "Hello")
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) }
+        assertFalse("upsert should NOT be called for blank sessionId", upsertCallCount > 0)
+    }
+
+    @Test
+    fun `null remote input does nothing`() {
+        every { RemoteInput.getResultsFromIntent(mockIntent) } returns null
+        every { mockIntent.getStringExtra(NotificationReplyReceiver.EXTRA_SESSION_ID) } returns "session-abc"
+
+        receiver.onReceive(mockContext, mockIntent)
+
+        verify(inverse = true) { HermesWsClient.sendMessage(any(), any()) }
+        assertFalse("upsert should NOT be called for null remote input", upsertCallCount > 0)
+    }
+
+    @Test
+    fun `Room failure logs error and finishes pending result`() {
+        givenValidReply("session-abc", "Hello")
+
+        // Make upsert throw
+        coEvery { mockDao.upsert(any<ChatMessageEntity>()) } throws RuntimeException("DB full")
+
+        receiver.onReceive(mockContext, mockIntent)
+        Thread.sleep(500)
+
+        // Error should be logged
+        verify { android.util.Log.e("NotificationReply", any<String>(), any()) }
+
+        // Pending result must still finish even on Room failure
+        verify { mockPendingResult.finish() }
+    }
+
+    // ── Helpers ────────────────────────────────────────────────────────────
+
+    private fun givenValidReply(
+        sessionId: String?,
+        replyText: String,
+    ) {
+        val mockRemoteInputResult = mockk<android.os.Bundle>(relaxed = true)
+        every {
+            mockRemoteInputResult.getCharSequence(NotificationReplyReceiver.KEY_TEXT_REPLY)
+        } returns replyText
+        every { RemoteInput.getResultsFromIntent(mockIntent) } returns mockRemoteInputResult
+        every { mockIntent.getStringExtra(NotificationReplyReceiver.EXTRA_SESSION_ID) } returns sessionId
+    }
+
+    private fun givenEmptyRemoteInput(sessionId: String?) {
+        val mockRemoteInputResult = mockk<android.os.Bundle>(relaxed = true)
+        every {
+            mockRemoteInputResult.getCharSequence(NotificationReplyReceiver.KEY_TEXT_REPLY)
+        } returns null
+        every { RemoteInput.getResultsFromIntent(mockIntent) } returns mockRemoteInputResult
+        every { mockIntent.getStringExtra(NotificationReplyReceiver.EXTRA_SESSION_ID) } returns sessionId
+    }
+}