diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 0cd97cc1c1..b37c5694c6 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -364,6 +364,9 @@ dependencies { testImplementation("com.squareup.okhttp3:mockwebserver:$okhttpVersion") testImplementation("com.google.dagger:hilt-android-testing:2.59.2") testImplementation("org.robolectric:robolectric:4.16.1") + // conscrypt-android provides Android JNI libs only; the openjdk-uber variant bundles + // JVM host natives so Robolectric can initialise the security provider without crashing. + testImplementation("org.conscrypt:conscrypt-openjdk-uber:2.5.2") } tasks.register("installGitHooks") { diff --git a/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt b/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt index 1723ce5ee6..7a928c0a47 100644 --- a/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/activities/CallActivity.kt @@ -2117,9 +2117,10 @@ class CallActivity : CallBaseActivity() { ) { Log.d(TAG, "handleCallParticipantsChanged") - // The signaling session is the same as the Nextcloud session only when the MCU is not used. + // The signaling session is the same as the Nextcloud session only when internal signaling is used. + // With external signaling (HPB), participant session IDs are HPB session IDs regardless of MCU. var currentSessionId = callSession - if (hasMCU) { + if (webSocketClient != null) { currentSessionId = webSocketClient!!.sessionId } Log.d(TAG, " currentSessionId is $currentSessionId") @@ -2424,7 +2425,9 @@ class CallActivity : CallBaseActivity() { signalingMessageReceiver!! ) - localStateBroadcaster!!.handleCallParticipantAdded(callViewModel.getParticipant(sessionId)?.uiState?.value) + // Pass the live StateFlow so LocalStateBroadcasterNoMcu can observe ICE state changes + // and send the local state exactly when the data channel becomes ready. + localStateBroadcaster!!.handleCallParticipantAdded(callViewModel.getParticipant(sessionId)!!.uiState) initPipMode() } diff --git a/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcaster.java b/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcaster.java index 63d336fa5a..ebccf83b06 100644 --- a/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcaster.java +++ b/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcaster.java @@ -13,6 +13,8 @@ import java.util.Objects; +import kotlinx.coroutines.flow.StateFlow; + /** * Helper class to send the local participant state to the other participants in the call. *

@@ -82,6 +84,17 @@ public void destroy() { this.localCallParticipantModel.removeObserver(localCallParticipantModelObserver); } + /** + * Passes the live StateFlow for the given participant so the broadcaster can react to each + * connection-state change. The default implementation takes a snapshot of the current value + * and delegates to {@link #handleCallParticipantAdded(ParticipantUiState)}; subclasses that + * need to observe future emissions (e.g. {@code LocalStateBroadcasterNoMcu}) should override + * this method instead. + */ + public void handleCallParticipantAdded(StateFlow uiStateFlow) { + handleCallParticipantAdded(uiStateFlow.getValue()); + } + public abstract void handleCallParticipantAdded(ParticipantUiState uiState); public abstract void handleCallParticipantRemoved(String sessionId); diff --git a/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcu.kt b/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcu.kt index d52a3078e4..6956e39df1 100644 --- a/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcu.kt +++ b/app/src/main/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcu.kt @@ -14,26 +14,25 @@ package com.nextcloud.talk.call * state when a remote participant is added. * * - * The state is sent when a connection with another participant is first established (which implicitly broadcasts the - * initial state when the local participant joins the call, as a connection is established with all the remote - * participants). Note that, as long as that participant stays in the call, the initial state is not sent again, even - * after a temporary disconnection; data channels use a reliable transport by default, so even if the state changes - * while the connection is temporarily interrupted the normal state update messages should be received by the other - * participant once the connection is restored. + * The state is sent the first time the ICE connection reaches a "connected" state for a given participant + * (isConnected transitions from false/unknown to true). The observer collects the participant's + * uiState StateFlow so that if the connection is briefly lost and then restored (e.g. an ICE restart), + * the state is re-sent on the next false → true transition. * * - * Nevertheless, in case of a failed connection and an ICE restart it is unclear whether the data channel messages - * would be received or not (as the data channel transport may be the one that failed and needs to be restarted). - * However, the state (except the speaking state) is also sent through signaling messages, which need to be - * explicitly fetched from the internal signaling server, so even in case of a failed connection they will be - * eventually received once the remote participant connects again. + * Note that, as long as a participant stays in the call, the state is sent each time the ICE connection + * goes from disconnected to connected; data channels use a reliable transport by default, so even if the + * state changes while the connection is temporarily interrupted the normal state update messages should be + * received by the other participant once the connection is restored. */ import com.nextcloud.talk.activities.ParticipantUiState import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob -import org.webrtc.PeerConnection.IceConnectionState +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.launch import java.util.concurrent.ConcurrentHashMap class LocalStateBroadcasterNoMcu( @@ -42,39 +41,25 @@ class LocalStateBroadcasterNoMcu( private val scope: CoroutineScope = CoroutineScope(Dispatchers.Main.immediate + SupervisorJob()) ) : LocalStateBroadcaster(localCallParticipantModel, messageSender) { - // Map sessionId -> observer wrapper (Flow collector job) private val iceConnectionStateObservers = ConcurrentHashMap() - private inner class IceConnectionStateObserver(val uiState: ParticipantUiState) { - private var job: Job? = null - - init { - handleStateChange(uiState) - } - - private fun handleStateChange(uiState: ParticipantUiState) { - // Determine ICE connection state - val iceState = if (uiState.isConnected) IceConnectionState.CONNECTED else IceConnectionState.NEW - - if (iceState == IceConnectionState.CONNECTED) { - remove() - sendState(uiState.sessionKey) - } - } - - fun remove() { - job?.cancel() - iceConnectionStateObservers.remove(uiState.sessionKey) - } + /** + * Primary entry point called by CallActivity. Starts (or restarts) collecting the live + * StateFlow so that the local state is sent every time the ICE connection transitions from + * disconnected to connected. + */ + override fun handleCallParticipantAdded(uiStateFlow: StateFlow) { + val sessionId = uiStateFlow.value.sessionKey ?: return + iceConnectionStateObservers[sessionId]?.remove() + iceConnectionStateObservers[sessionId] = IceConnectionStateObserver(sessionId, uiStateFlow) } + /** + * Fallback for callers that only have a snapshot (e.g. tests that pre-date the StateFlow API). + * Wraps the snapshot in a single-value StateFlow and delegates to the primary overload. + */ override fun handleCallParticipantAdded(uiState: ParticipantUiState) { - uiState.sessionKey?.let { - iceConnectionStateObservers[it]?.remove() - - iceConnectionStateObservers[it] = - IceConnectionStateObserver(uiState) - } + handleCallParticipantAdded(MutableStateFlow(uiState) as StateFlow) } override fun handleCallParticipantRemoved(sessionId: String) { @@ -83,13 +68,35 @@ class LocalStateBroadcasterNoMcu( override fun destroy() { super.destroy() - // Cancel all collectors safely val observersCopy = iceConnectionStateObservers.values.toList() for (observer in observersCopy) { observer.remove() } } + private inner class IceConnectionStateObserver( + private val sessionId: String, + uiStateFlow: StateFlow + ) { + private val job: Job = scope.launch { + var previousIsConnected: Boolean? = null + uiStateFlow.collect { uiState -> + val currentIsConnected = uiState.isConnected + // Send state on every false → true (or null → true) transition so that the + // remote participant receives the local state as soon as the data channel is ready. + if (currentIsConnected && previousIsConnected != true) { + sendState(uiState.sessionKey) + } + previousIsConnected = currentIsConnected + } + } + + fun remove() { + job.cancel() + iceConnectionStateObservers.remove(sessionId) + } + } + private fun sendState(sessionKey: String?) { messageSender.send(getDataChannelMessageForAudioState(), sessionKey) messageSender.send(getDataChannelMessageForSpeakingState(), sessionKey) diff --git a/app/src/test/java/com/nextcloud/talk/activities/CallActivityAddParticipantTest.kt b/app/src/test/java/com/nextcloud/talk/activities/CallActivityAddParticipantTest.kt new file mode 100644 index 0000000000..12ddc00498 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/activities/CallActivityAddParticipantTest.kt @@ -0,0 +1,136 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Alain Lauzon + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.activities + +import android.app.Application +import com.nextcloud.talk.call.LocalStateBroadcaster +import com.nextcloud.talk.signaling.SignalingMessageReceiver +import kotlinx.coroutines.flow.StateFlow +import org.junit.Assert.assertSame +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.argumentCaptor +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import java.lang.reflect.Field + +/** + * Robolectric test verifying that [CallActivity.addCallParticipant] passes the **live StateFlow** + * from [ParticipantHandler.uiState] to [LocalStateBroadcaster.handleCallParticipantAdded]. + * + * Before the fix, the method passed only the snapshot value [StateFlow.value], meaning + * [LocalStateBroadcasterNoMcu] could not observe future ICE state changes and videoOn/audioOn + * were never sent once the data channel was ready. + * + * This test: + * - creates [CallActivity] via Robolectric WITHOUT calling [Activity.onCreate] (avoiding native + * WebRTC and Dagger initialisation) + * - injects the required fields via reflection + * - calls [addCallParticipant] via reflection + * - verifies that the argument passed to [LocalStateBroadcaster.handleCallParticipantAdded] is + * the exact same [StateFlow] object as [CallViewModel.getParticipant]!!.uiState + * + * If the buggy snapshot pattern is used (`uiState.value` instead of `uiState`), the verification + * fails because the [StateFlow] overload is never called. + */ +@RunWith(RobolectricTestRunner::class) +@Config( + // Use the plain Application to avoid NextcloudTalkApplication.onCreate() which initialises + // Dagger, WebRTC, and WorkManager — none of which are needed for this focused test. + application = Application::class, + sdk = [33] +) +class CallActivityAddParticipantTest { + + private val mockLocalStateBroadcaster: LocalStateBroadcaster = mock() + private val mockSignalingMessageReceiver: SignalingMessageReceiver = mock() + + private lateinit var callViewModel: CallViewModel + private lateinit var activity: CallActivity + + @Before + fun setUp() { + callViewModel = CallViewModel() + + // Build the Activity without triggering onCreate — this avoids Dagger injection, + // EglBase.create() (native), and all network calls. + activity = Robolectric.buildActivity(CallActivity::class.java).get() + + // Inject the minimum set of fields required by addCallParticipant. + setField("callViewModel", callViewModel) + setField("localStateBroadcaster", mockLocalStateBroadcaster) + setField("signalingMessageReceiver", mockSignalingMessageReceiver) + setField("baseUrl", "https://test.example.com") + setField("roomToken", "testRoom") + // hasExternalSignalingServer = true skips the OfferAnswerNickProvider branch + setField("hasExternalSignalingServer", true) + } + + // ----------------------------------------------------------------------------------------- + // Tests + // ----------------------------------------------------------------------------------------- + + /** + * The live [StateFlow] from [ParticipantHandler.uiState] must be passed to + * [LocalStateBroadcaster.handleCallParticipantAdded], not just the current snapshot. + * + * Fails with the buggy code because the snapshot call goes to the + * [handleCallParticipantAdded(ParticipantUiState)] overload, not the StateFlow one. + */ + @Test + fun `addCallParticipant passes the live StateFlow to handleCallParticipantAdded`() { + val sessionId = "robolectric-session-1" + + invokeAddCallParticipant(sessionId) + + val captor = argumentCaptor>() + verify(mockLocalStateBroadcaster).handleCallParticipantAdded(captor.capture()) + + val capturedFlow = captor.firstValue + + // The captured argument must be the SAME StateFlow object that ParticipantHandler + // exposes — not a copy, not a one-shot MutableStateFlow wrapping the snapshot. + assertSame( + "Expected the live ParticipantHandler.uiState StateFlow, got a different object", + callViewModel.getParticipant(sessionId)!!.uiState, + capturedFlow + ) + } + + // ----------------------------------------------------------------------------------------- + // Reflection helpers + // ----------------------------------------------------------------------------------------- + + private fun invokeAddCallParticipant(sessionId: String) { + val method = CallActivity::class.java.getDeclaredMethod("addCallParticipant", String::class.java) + method.isAccessible = true + method.invoke(activity, sessionId) + } + + private fun setField(fieldName: String, value: Any?) { + val field = findField(CallActivity::class.java, fieldName) + ?: error("Field '$fieldName' not found in CallActivity hierarchy") + field.isAccessible = true + field.set(activity, value) + } + + private fun findField(clazz: Class<*>, fieldName: String): Field? { + var current: Class<*>? = clazz + while (current != null) { + try { + return current.getDeclaredField(fieldName) + } catch (e: NoSuchFieldException) { + current = current.superclass + } + } + return null + } +} diff --git a/app/src/test/java/com/nextcloud/talk/activities/CallParticipantStateBroadcastIntegrationTest.kt b/app/src/test/java/com/nextcloud/talk/activities/CallParticipantStateBroadcastIntegrationTest.kt new file mode 100644 index 0000000000..b7d457defe --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/activities/CallParticipantStateBroadcastIntegrationTest.kt @@ -0,0 +1,358 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Alain Lauzon + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.activities + +import com.nextcloud.talk.call.LocalStateBroadcasterNoMcu +import com.nextcloud.talk.call.MessageSenderNoMcu +import com.nextcloud.talk.call.MutableLocalCallParticipantModel +import com.nextcloud.talk.models.json.signaling.DataChannelMessage +import com.nextcloud.talk.models.json.signaling.NCMessagePayload +import com.nextcloud.talk.models.json.signaling.NCSignalingMessage +import com.nextcloud.talk.signaling.SignalingMessageReceiver +import com.nextcloud.talk.webrtc.PeerConnectionWrapper +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.mockito.kotlin.argumentCaptor +import org.mockito.kotlin.clearInvocations +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoInteractions +import org.webrtc.PeerConnection.IceConnectionState + +/** + * Integration tests verifying that the full pipeline — CallViewModel → ParticipantHandler → + * LocalStateBroadcasterNoMcu — correctly sends videoOn/audioOn data channel messages to a + * remote participant after ICE connects. + * + * The bug (commit 2b0f37148): CallActivity called handleCallParticipantAdded only once at + * participant-add time, passing only the current snapshot. Because the PeerConnectionWrapper + * did not exist yet at that moment, the data channel was unavailable and all state messages + * were silently dropped. Subsequent ICE transitions were never observed. + * + * The fix: LocalStateBroadcasterNoMcu now accepts a live StateFlow and + * collects it internally. State is sent on every false→true transition of isConnected — + * i.e. when the data channel is actually ready. + * + * These tests exercise the real interaction between CallViewModel, ParticipantHandler, and + * LocalStateBroadcasterNoMcu, using a mocked PeerConnectionWrapper to avoid native WebRTC. + * + * Unlike the unit tests in LocalStateBroadcasterNoMcuTest, these tests go one level higher + * and verify that the wiring works end-to-end through the real ParticipantHandler StateFlow. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class CallParticipantStateBroadcastIntegrationTest { + + companion object { + private const val SESSION_ID = "theRemoteSessionId" + private const val BASE_URL = "https://cloud.example.com" + private const val ROOM_TOKEN = "abc123" + } + + private val testDispatcher = StandardTestDispatcher() + private val testScope = TestScope(testDispatcher) + + private lateinit var callViewModel: CallViewModel + private lateinit var localStateBroadcasterNoMcu: LocalStateBroadcasterNoMcu + private lateinit var mockedMessageSender: MessageSenderNoMcu + private lateinit var localCallParticipantModel: MutableLocalCallParticipantModel + private lateinit var mockedSignalingMessageReceiver: SignalingMessageReceiver + private lateinit var mockedPeerConnectionWrapper: PeerConnectionWrapper + + @Before + fun setUp() { + kotlinx.coroutines.Dispatchers.setMain(testDispatcher) + + mockedSignalingMessageReceiver = mock() + mockedMessageSender = mock() + mockedPeerConnectionWrapper = mock() + + localCallParticipantModel = MutableLocalCallParticipantModel().apply { + isAudioEnabled = true + isSpeaking = false + isVideoEnabled = true + } + + callViewModel = CallViewModel() + + // No explicit scope: uses default CoroutineScope(Dispatchers.Main.immediate + SupervisorJob()). + // Because Dispatchers.setMain(testDispatcher) is called above, Dispatchers.Main.immediate + // delegates to testDispatcher — so advanceUntilIdle() advances the broadcaster's coroutines + // while they stay outside testScope, avoiding UncompletedCoroutinesError. + localStateBroadcasterNoMcu = LocalStateBroadcasterNoMcu( + localCallParticipantModel, + mockedMessageSender + ) + } + + @After + fun tearDown() { + localStateBroadcasterNoMcu.destroy() + kotlinx.coroutines.Dispatchers.resetMain() + } + + // ----------------------------------------------------------------------------------------- + // Helper: wire up participant (simulates CallActivity.addCallParticipant with the fix) + // ----------------------------------------------------------------------------------------- + + /** + * Adds a participant to the CallViewModel and passes its live StateFlow to the broadcaster — + * exactly as CallActivity does after the fix. + */ + private fun addParticipantAndWire(sessionId: String = SESSION_ID) { + callViewModel.addParticipant(BASE_URL, ROOM_TOKEN, sessionId, mockedSignalingMessageReceiver) + val participantHandler = callViewModel.getParticipant(sessionId)!! + localStateBroadcasterNoMcu.handleCallParticipantAdded(participantHandler.uiState) + } + + /** + * Sets a (mocked) PeerConnection on the participant and captures the PeerConnectionObserver + * registered by ParticipantHandler. The observer lets tests drive ICE transitions. + */ + private fun setPeerConnectionAndCaptureObserver( + sessionId: String = SESSION_ID, + wrapper: PeerConnectionWrapper = mockedPeerConnectionWrapper + ): PeerConnectionWrapper.PeerConnectionObserver { + callViewModel.getParticipant(sessionId)!!.setPeerConnection(wrapper) + val captor = argumentCaptor() + verify(wrapper).addObserver(captor.capture()) + return captor.firstValue + } + + private fun verifyFullStateSent(sessionId: String) { + verify(mockedMessageSender).send(DataChannelMessage("audioOn"), sessionId) + verify(mockedMessageSender).send(DataChannelMessage("stoppedSpeaking"), sessionId) + verify(mockedMessageSender).send(DataChannelMessage("videoOn"), sessionId) + verify(mockedMessageSender).send(expectedSignalingUnmuteAudio(), sessionId) + verify(mockedMessageSender).send(expectedSignalingUnmuteVideo(), sessionId) + } + + private fun expectedSignalingUnmuteAudio(): NCSignalingMessage { + val msg = NCSignalingMessage() + msg.roomType = "video" + msg.type = "unmute" + msg.payload = NCMessagePayload().also { it.name = "audio" } + return msg + } + + private fun expectedSignalingUnmuteVideo(): NCSignalingMessage { + val msg = NCSignalingMessage() + msg.roomType = "video" + msg.type = "unmute" + msg.payload = NCMessagePayload().also { it.name = "video" } + return msg + } + + // ----------------------------------------------------------------------------------------- + // Tests: nominal ICE flow + // ----------------------------------------------------------------------------------------- + + /** + * Happy path: full ICE negotiation cycle NEW → CONNECTED. + * + * Verifies that videoOn and audioOn data channel messages are sent exactly once, + * after ICE reaches CONNECTED — not before. + */ + @Test + fun `videoOn and audioOn sent after ICE CONNECTED`() = + testScope.runTest { + addParticipantAndWire() + advanceUntilIdle() + // Clear the initial-state send (isConnected=true before peer connection = no data channel) + clearInvocations(mockedMessageSender) + + val iceObserver = setPeerConnectionAndCaptureObserver() + advanceUntilIdle() + + iceObserver.onIceConnectionStateChanged(IceConnectionState.NEW) + advanceUntilIdle() + verifyNoInteractions(mockedMessageSender) + + iceObserver.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + + verifyFullStateSent(SESSION_ID) + } + + /** + * ICE reaching COMPLETED (equivalent to CONNECTED for WebRTC) must also trigger the send. + */ + @Test + fun `videoOn sent when ICE reaches COMPLETED`() = + testScope.runTest { + addParticipantAndWire() + advanceUntilIdle() + clearInvocations(mockedMessageSender) + + val iceObserver = setPeerConnectionAndCaptureObserver() + iceObserver.onIceConnectionStateChanged(IceConnectionState.NEW) + advanceUntilIdle() + verifyNoInteractions(mockedMessageSender) + + iceObserver.onIceConnectionStateChanged(IceConnectionState.COMPLETED) + advanceUntilIdle() + + verifyFullStateSent(SESSION_ID) + } + + /** + * Two participants each get an independent observer; state is sent independently. + */ + @Test + fun `two participants each receive videoOn independently`() = + testScope.runTest { + val session1 = "session1" + val session2 = "session2" + + addParticipantAndWire(session1) + advanceUntilIdle() + addParticipantAndWire(session2) + advanceUntilIdle() + clearInvocations(mockedMessageSender) + + // --- Participant 1 --- + val observer1 = setPeerConnectionAndCaptureObserver(session1, mockedPeerConnectionWrapper) + observer1.onIceConnectionStateChanged(IceConnectionState.NEW) + advanceUntilIdle() + verifyNoInteractions(mockedMessageSender) + + observer1.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + verifyFullStateSent(session1) + clearInvocations(mockedMessageSender) + + // --- Participant 2 --- + val mockedWrapper2: PeerConnectionWrapper = mock() + val observer2 = setPeerConnectionAndCaptureObserver(session2, mockedWrapper2) + observer2.onIceConnectionStateChanged(IceConnectionState.NEW) + advanceUntilIdle() + verifyNoInteractions(mockedMessageSender) + + observer2.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + verifyFullStateSent(session2) + } + + // ----------------------------------------------------------------------------------------- + // Tests: local state variants + // ----------------------------------------------------------------------------------------- + + @Test + fun `videoOff sent when local video is disabled`() = + testScope.runTest { + localCallParticipantModel.isVideoEnabled = false + + addParticipantAndWire() + advanceUntilIdle() + clearInvocations(mockedMessageSender) + + val iceObserver = setPeerConnectionAndCaptureObserver() + iceObserver.onIceConnectionStateChanged(IceConnectionState.NEW) + advanceUntilIdle() + iceObserver.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + + verify(mockedMessageSender).send(DataChannelMessage("videoOff"), SESSION_ID) + verify(mockedMessageSender).send(DataChannelMessage("audioOn"), SESSION_ID) + verify(mockedMessageSender).send(DataChannelMessage("stoppedSpeaking"), SESSION_ID) + } + + @Test + fun `audioOff sent when local audio is disabled`() = + testScope.runTest { + localCallParticipantModel.isAudioEnabled = false + + addParticipantAndWire() + advanceUntilIdle() + clearInvocations(mockedMessageSender) + + val iceObserver = setPeerConnectionAndCaptureObserver() + iceObserver.onIceConnectionStateChanged(IceConnectionState.NEW) + advanceUntilIdle() + iceObserver.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + + verify(mockedMessageSender).send(DataChannelMessage("audioOff"), SESSION_ID) + verify(mockedMessageSender).send(DataChannelMessage("videoOn"), SESSION_ID) + verify(mockedMessageSender).send(DataChannelMessage("stoppedSpeaking"), SESSION_ID) + } + + // ----------------------------------------------------------------------------------------- + // Tests: cleanup + // ----------------------------------------------------------------------------------------- + + @Test + fun `no state sent after broadcaster is destroyed`() = + testScope.runTest { + addParticipantAndWire() + advanceUntilIdle() + + val iceObserver = setPeerConnectionAndCaptureObserver() + + localStateBroadcasterNoMcu.destroy() + advanceUntilIdle() + clearInvocations(mockedMessageSender) + + iceObserver.onIceConnectionStateChanged(IceConnectionState.NEW) + iceObserver.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + + verifyNoInteractions(mockedMessageSender) + } + + // ----------------------------------------------------------------------------------------- + // Regression: explicit demonstration of the bug (before the fix) + // ----------------------------------------------------------------------------------------- + + /** + * REGRESSION: Before the fix, CallActivity passed only the snapshot value at add time. + * At that moment: + * - isConnected = true (ParticipantHandler default before any PeerConnection) + * - sendState() was called immediately — but data channel didn't exist yet → dropped + * + * After setPeerConnection(), ICE went NEW → CONNECTED, but nobody called + * handleCallParticipantAdded again → videoOn was never sent. + * + * This test verifies that the old (buggy) snapshot pattern fails to deliver state after + * ICE connects, making the regression explicit and permanent. + */ + @Test + fun `REGRESSION - videoOn never sent with one-shot snapshot handleCallParticipantAdded`() = + testScope.runTest { + callViewModel.addParticipant(BASE_URL, ROOM_TOKEN, SESSION_ID, mockedSignalingMessageReceiver) + val participantHandler = callViewModel.getParticipant(SESSION_ID)!! + + // Buggy pattern: pass only the snapshot (the .value of the StateFlow) + localStateBroadcasterNoMcu.handleCallParticipantAdded(participantHandler.uiState.value) + advanceUntilIdle() + + participantHandler.setPeerConnection(mockedPeerConnectionWrapper) + advanceUntilIdle() + val captor = argumentCaptor() + verify(mockedPeerConnectionWrapper).addObserver(captor.capture()) + + // Clear invocations from the initial (dropped) send + clearInvocations(mockedMessageSender) + + // ICE negotiates and connects + captor.firstValue.onIceConnectionStateChanged(IceConnectionState.NEW) + captor.firstValue.onIceConnectionStateChanged(IceConnectionState.CONNECTED) + advanceUntilIdle() + + // Without a live StateFlow: handleCallParticipantAdded was called with a snapshot that + // wrapped in a never-updating MutableStateFlow. The ICE transition was never observed + // → no state sent after ICE CONNECTED. + verifyNoInteractions(mockedMessageSender) + } +} diff --git a/app/src/test/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcuTest.kt b/app/src/test/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcuTest.kt index eb9989e522..694ce30790 100644 --- a/app/src/test/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcuTest.kt +++ b/app/src/test/java/com/nextcloud/talk/call/LocalStateBroadcasterNoMcuTest.kt @@ -12,21 +12,36 @@ import com.nextcloud.talk.models.json.signaling.NCMessagePayload import com.nextcloud.talk.models.json.signaling.NCSignalingMessage import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain +import org.junit.After import org.junit.Before import org.junit.Test import org.mockito.Mockito +/** + * Unit tests for [LocalStateBroadcasterNoMcu]. + * + * All tests drive state via [MutableStateFlow] — the same mechanism used by [CallActivity] in + * production. This ensures that the tests fail if the StateFlow is not collected internally (i.e. + * if the fix is reverted and only a snapshot is taken at call time). + * + * The broadcaster is created WITHOUT an explicit scope so it uses its default + * `CoroutineScope(Dispatchers.Main.immediate + SupervisorJob())`. Because [Dispatchers.setMain] + * is called in setUp, `Dispatchers.Main.immediate` delegates to [testDispatcher]; this means + * [advanceUntilIdle] advances the broadcaster's coroutines while they remain outside + * `testScope`, avoiding [kotlinx.coroutines.test.UncompletedCoroutinesError]. + */ @OptIn(ExperimentalCoroutinesApi::class) class LocalStateBroadcasterNoMcuTest { private lateinit var localCallParticipantModel: MutableLocalCallParticipantModel private lateinit var mockedMessageSenderNoMcu: MessageSenderNoMcu - private lateinit var localStateBroadcasterNoMcu: LocalStateBroadcasterNoMcu private val testDispatcher = StandardTestDispatcher() @@ -43,6 +58,14 @@ class LocalStateBroadcasterNoMcuTest { mockedMessageSenderNoMcu = Mockito.mock(MessageSenderNoMcu::class.java) } + @After + fun tearDown() { + if (::localStateBroadcasterNoMcu.isInitialized) { + localStateBroadcasterNoMcu.destroy() + } + Dispatchers.resetMain() + } + private fun getExpectedUnmuteAudio(): NCSignalingMessage { val expectedUnmuteAudio = NCSignalingMessage() expectedUnmuteAudio.roomType = "video" @@ -67,34 +90,78 @@ class LocalStateBroadcasterNoMcuTest { return expectedUnmuteVideo } + /** + * Happy path: participant starts disconnected, then ICE connects. + * + * Drives the state via MutableStateFlow — this test FAILS if LocalStateBroadcasterNoMcu + * only takes a snapshot at handleCallParticipantAdded time instead of collecting the flow. + */ @Test fun testStateSentWhenParticipantConnects() = testScope.runTest { localStateBroadcasterNoMcu = LocalStateBroadcasterNoMcu( localCallParticipantModel, - mockedMessageSenderNoMcu, - testScope + mockedMessageSenderNoMcu + // No explicit scope: uses default Dispatchers.Main.immediate = testDispatcher ) - val initialState = createTestParticipantUiState( - sessionId = "theSessionId", - isConnected = false + val uiStateFlow = MutableStateFlow( + createTestParticipantUiState(sessionId = "theSessionId", isConnected = false) ) - localStateBroadcasterNoMcu.handleCallParticipantAdded(initialState) - + localStateBroadcasterNoMcu.handleCallParticipantAdded(uiStateFlow) advanceUntilIdle() - // Verify nothing is sent because isConnected is false + // Not connected yet — nothing should be sent Mockito.verifyNoInteractions(mockedMessageSenderNoMcu) - // State 2: The same participant's state is updated to connected - val connectedState = initialState.copy(isConnected = true) + // ICE connects — state must be sent now + uiStateFlow.value = uiStateFlow.value.copy(isConnected = true) + advanceUntilIdle() - localStateBroadcasterNoMcu.handleCallParticipantAdded(connectedState) + verifyStateSent("theSessionId") + Mockito.verifyNoMoreInteractions(mockedMessageSenderNoMcu) + } - advanceUntilIdle() // Allow the broadcaster to react + /** + * Regression test for the bug where videoOn/audioOn were never sent after ICE connected. + * + * Real sequence in CallActivity: + * 1. addParticipant() → ParticipantHandler.uiState starts with isConnected=true (no peer yet) + * 2. setPeerConnection() → ICE=NEW → isConnected=false + * 3. ICE negotiation completes → isConnected=true + * + * The fix: LocalStateBroadcasterNoMcu collects the StateFlow and sends state on each + * false→true transition. A snapshot taken at step 1 would miss the step-3 transition. + */ + @Test + fun testStateSentAfterTransientDisconnect() = + testScope.runTest { + localStateBroadcasterNoMcu = LocalStateBroadcasterNoMcu( + localCallParticipantModel, + mockedMessageSenderNoMcu + ) + val uiStateFlow = MutableStateFlow( + createTestParticipantUiState(sessionId = "theSessionId", isConnected = true) + ) + + // Step 1: initial state is connected (ParticipantHandler default before peer connection) + localStateBroadcasterNoMcu.handleCallParticipantAdded(uiStateFlow) + advanceUntilIdle() + + // The initial connected state triggers a send (data channel not ready yet in + // production — message will be dropped, but that is acceptable behaviour) + Mockito.clearInvocations(mockedMessageSenderNoMcu) + + // Step 2: setPeerConnection() → ICE=NEW → disconnected + uiStateFlow.value = uiStateFlow.value.copy(isConnected = false) + advanceUntilIdle() + Mockito.verifyNoInteractions(mockedMessageSenderNoMcu) + + // Step 3: ICE completed → data channel now available → state must be sent + uiStateFlow.value = uiStateFlow.value.copy(isConnected = true) + advanceUntilIdle() verifyStateSent("theSessionId") Mockito.verifyNoMoreInteractions(mockedMessageSenderNoMcu) } @@ -104,18 +171,18 @@ class LocalStateBroadcasterNoMcuTest { testScope.runTest { localStateBroadcasterNoMcu = LocalStateBroadcasterNoMcu( localCallParticipantModel, - mockedMessageSenderNoMcu, - testScope + mockedMessageSenderNoMcu ) - val initialState = createTestParticipantUiState( - sessionId = "theSessionId", - isConnected = false + val uiStateFlow = MutableStateFlow( + createTestParticipantUiState(sessionId = "theSessionId", isConnected = false) ) - localStateBroadcasterNoMcu.handleCallParticipantAdded(initialState) + localStateBroadcasterNoMcu.handleCallParticipantAdded(uiStateFlow) localStateBroadcasterNoMcu.handleCallParticipantRemoved("theSessionId") + // State transitions after removal must not trigger a send + uiStateFlow.value = uiStateFlow.value.copy(isConnected = true) advanceUntilIdle() Mockito.verifyNoInteractions(mockedMessageSenderNoMcu) @@ -126,18 +193,18 @@ class LocalStateBroadcasterNoMcuTest { testScope.runTest { localStateBroadcasterNoMcu = LocalStateBroadcasterNoMcu( localCallParticipantModel, - mockedMessageSenderNoMcu, - testScope + mockedMessageSenderNoMcu ) - val initialState = createTestParticipantUiState( - sessionId = "theSessionId", - isConnected = false + val uiStateFlow = MutableStateFlow( + createTestParticipantUiState(sessionId = "theSessionId", isConnected = false) ) - localStateBroadcasterNoMcu.handleCallParticipantAdded(initialState) + localStateBroadcasterNoMcu.handleCallParticipantAdded(uiStateFlow) localStateBroadcasterNoMcu.destroy() + // State transitions after destroy must not trigger a send + uiStateFlow.value = uiStateFlow.value.copy(isConnected = true) advanceUntilIdle() Mockito.verifyNoInteractions(mockedMessageSenderNoMcu)