test

Did a bunch of cleanup on several connection tests to improve stability and use more recent integration test features.
ClientOnlyConnectionTests reduced the transport connection timeout and increased the timeout helper.
ConnectionApprovalTimeoutTests needed to have the server and client timeout values vary depending upon the test type.
PeerDisconnectCallbackTests needed to trap for the peer disconnecting.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Tests/Runtime/ClientOnlyConnectionTests.cs

@@ -32,9 +32,9 @@ public void Setup()
             // Default is 1000ms per connection attempt and 60 connection attempts (60s)
             // Currently there is no easy way to set these values other than in-editor
             var unityTransport = m_NetworkManagerGameObject.AddComponent<UnityTransport>();
-            unityTransport.ConnectTimeoutMS = 1000;
+            unityTransport.ConnectTimeoutMS = 500;
             unityTransport.MaxConnectAttempts = 1;
-            m_TimeoutHelper = new TimeoutHelper(2);
+            m_TimeoutHelper = new TimeoutHelper(4);
             m_ClientNetworkManager.NetworkConfig.NetworkTransport = unityTransport;
         }
 
@@ -49,7 +49,6 @@ public IEnumerator ClientFailsToConnect()
 
             // Unity Transport throws an error when it times out
             LogAssert.Expect(LogType.Error, "Failed to connect to server.");
-
             yield return NetcodeIntegrationTest.WaitForConditionOrTimeOut(() => m_WasDisconnected, m_TimeoutHelper);
             Assert.False(m_TimeoutHelper.TimedOut, "Timed out waiting for client to timeout waiting to connect!");
 

com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApproval.cs

@@ -92,17 +92,23 @@ protected override void OnServerAndClientsCreated()
         private void Client_OnClientDisconnectCallback(ulong clientId)
         {
             m_ClientNetworkManagers[0].OnClientDisconnectCallback -= Client_OnClientDisconnectCallback;
-            m_ClientDisconnectReasonValidated = m_ClientNetworkManagers[0].LocalClientId == clientId && m_ClientNetworkManagers[0].DisconnectReason.Contains(k_InvalidToken);
+            m_ClientDisconnectReasonValidated = m_ClientNetworkManagers[0].LocalClientId == clientId && m_ClientNetworkManagers[0].ConnectionManager.ServerDisconnectReason.Contains(k_InvalidToken);
         }
 
-        private bool ClientAndHostValidated()
+        private bool ClientAndHostValidated(StringBuilder errorLog)
         {
             if (!m_Validated.ContainsKey(m_ServerNetworkManager.LocalClientId) || !m_Validated[m_ServerNetworkManager.LocalClientId])
             {
+                errorLog.AppendLine($"Server does not contain a validation for Client-{m_ServerNetworkManager.LocalClientId}!");
                 return false;
             }
             if (m_PlayerCreation == PlayerCreation.FailValidation)
             {
+                if (!m_ClientDisconnectReasonValidated)
+                {
+                    errorLog.AppendLine($"{nameof(m_ClientDisconnectReasonValidated)} is false!");
+                }
+                
                 return m_ClientDisconnectReasonValidated;
             }
             else
@@ -111,6 +117,7 @@ private bool ClientAndHostValidated()
                 {
                     if (!m_Validated.ContainsKey(client.LocalClientId) || !m_Validated[client.LocalClientId])
                     {
+                        errorLog.AppendLine($"Client-{client.LocalClientId} was not in the validated list!");
                         return false;
                     }
                 }
@@ -163,6 +170,7 @@ private void NetworkManagerObject_ConnectionApprovalCallback(NetworkManager.Conn
             {
                 response.Approved = false;
                 response.Reason = "Invalid validation token!";
+                return;
             }
 
             response.CreatePlayerObject = ShouldCheckForSpawnedPlayers();

com.unity.netcode.gameobjects/Tests/Runtime/ConnectionApprovalTimeoutTests.cs

@@ -28,7 +28,7 @@ public ConnectionApprovalTimeoutTests(ApprovalTimedOutTypes approvalFailureType)
 
         // Must be >= 5 since this is an int value and the test waits for timeout - 1 to try to verify it doesn't
         // time out early
-        private const int k_TestTimeoutPeriod = 5;
+        private const int k_TestTimeoutPeriod = 3;
 
         private Regex m_ExpectedLogMessage;
         private LogType m_LogType;
@@ -48,21 +48,47 @@ protected override IEnumerator OnTearDown()
 
         protected override void OnServerAndClientsCreated()
         {
-            m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
+            // The timeouts (client-server relative) need to be skewed such that either the server or the client will timeout prior to the other timing out.
+            var serverTimeout = (int)(m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond ? k_TestTimeoutPeriod * 1.5f : k_TestTimeoutPeriod * 0.75f);
+            var clientTimeout = (int)(m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond ? k_TestTimeoutPeriod * 0.75f : k_TestTimeoutPeriod * 1.5f);
+
+            m_ServerNetworkManager.ConnectionApprovalCallback = ConnectionApproval;
+
+            m_ServerNetworkManager.NetworkConfig.ConnectionApproval = true;
+            m_ServerNetworkManager.NetworkConfig.ClientConnectionBufferTimeout = serverTimeout;
             m_ServerNetworkManager.LogLevel = LogLevel.Developer;
-            m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = k_TestTimeoutPeriod;
+
+            m_ClientNetworkManagers[0].NetworkConfig.ClientConnectionBufferTimeout = clientTimeout;
             m_ClientNetworkManagers[0].LogLevel = LogLevel.Developer;
+            m_ClientNetworkManagers[0].NetworkConfig.ConnectionApproval = true;
+
             base.OnServerAndClientsCreated();
         }
 
+        private void ConnectionApproval(NetworkManager.ConnectionApprovalRequest connectionApprovalRequest, NetworkManager.ConnectionApprovalResponse connectionApprovalResponse)
+        {
+            if (connectionApprovalRequest.ClientNetworkId != NetworkManager.ServerClientId)
+            {
+                // For these tests we just always place the newly connecting client into pending
+                connectionApprovalResponse.Pending = true;
+            }
+            else
+            {
+                // We always approve the host
+                connectionApprovalResponse.Approved = true;
+            }
+        }
+
+        private const string k_ExpectedLogWhenServerDoesNotRespond = "Timed out waiting for the server to approve the connection request.";
+        private const string k_ExpectedLogWhenClientDoesNotRequest = "Server detected a transport connection from Client-1, but timed out waiting for the connection request message.";
+
         protected override IEnumerator OnStartedServerAndClients()
         {
             if (m_ApprovalFailureType == ApprovalTimedOutTypes.ServerDoesNotRespond)
             {
-                m_ServerNetworkManager.ConnectionManager.MockSkippingApproval = true;
                 // We catch (don't process) the incoming approval message to simulate the server not sending the approved message in time
                 m_ClientNetworkManagers[0].ConnectionManager.MessageManager.Hook(new MessageCatcher<ConnectionApprovedMessage>(m_ClientNetworkManagers[0]));
-                m_ExpectedLogMessage = new Regex("Timed out waiting for the server to approve the connection request.");
+                m_ExpectedLogMessage = new Regex(k_ExpectedLogWhenServerDoesNotRespond);
                 m_LogType = LogType.Log;
             }
             else
@@ -72,7 +98,7 @@ protected override IEnumerator OnStartedServerAndClients()
                 m_ServerNetworkManager.ConnectionManager.MessageManager.Hook(new MessageCatcher<ConnectionRequestMessage>(m_ServerNetworkManager));
 
                 // For this test, we know the timed out client will be Client-1
-                m_ExpectedLogMessage = new Regex("Server detected a transport connection from Client-1, but timed out waiting for the connection request message.");
+                m_ExpectedLogMessage = new Regex(k_ExpectedLogWhenClientDoesNotRequest);
                 m_LogType = LogType.Warning;
             }
             yield return null;
@@ -87,16 +113,14 @@ public IEnumerator ValidateApprovalTimeout()
             // Verify we haven't received the time out message yet
             NetcodeLogAssert.LogWasNotReceived(LogType.Log, m_ExpectedLogMessage);
 
-            yield return new WaitForSeconds(k_TestTimeoutPeriod * 1.25f);
+            yield return new WaitForSeconds(k_TestTimeoutPeriod * 1.55f);
 
             // We should have the test relative log message by this time.
             NetcodeLogAssert.LogWasReceived(m_LogType, m_ExpectedLogMessage);
 
             VerboseDebug("Checking connected client count");
             // It should only have the host client connected
             Assert.AreEqual(1, m_ServerNetworkManager.ConnectedClients.Count, $"Expected only one client when there were {m_ServerNetworkManager.ConnectedClients.Count} clients connected!");
-
-
             Assert.AreEqual(0, m_ServerNetworkManager.ConnectionManager.PendingClients.Count, $"Expected no pending clients when there were {m_ServerNetworkManager.ConnectionManager.PendingClients.Count} pending clients!");
             Assert.True(!m_ClientNetworkManagers[0].LocalClient.IsApproved, $"Expected the client to not have been approved, but it was!");
         }

com.unity.netcode.gameobjects/Tests/Runtime/PeerDisconnectCallbackTests.cs

@@ -1,6 +1,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
+using System.Text;
 using NUnit.Framework;
 using Unity.Netcode.TestHelpers.Runtime;
 using UnityEngine.TestTools;
@@ -46,16 +47,15 @@ protected override void OnServerAndClientsCreated()
             // Adjusting client and server timeout periods to reduce test time
             // Get the tick frequency in milliseconds and triple it for the heartbeat timeout
             var heartBeatTimeout = (int)(300 * (1.0f / m_ServerNetworkManager.NetworkConfig.TickRate));
-            var unityTransport = m_ServerNetworkManager.NetworkConfig.NetworkTransport as Transports.UTP.UnityTransport;
-            if (unityTransport != null)
-            {
-                unityTransport.HeartbeatTimeoutMS = heartBeatTimeout;
-            }
 
-            unityTransport = m_ClientNetworkManagers[0].NetworkConfig.NetworkTransport as Transports.UTP.UnityTransport;
-            if (unityTransport != null)
+            foreach (var networkManager in m_NetworkManagers)
             {
-                unityTransport.HeartbeatTimeoutMS = heartBeatTimeout;
+                var unityTransport = networkManager.NetworkConfig.NetworkTransport as Transports.UTP.UnityTransport;
+                if (unityTransport != null)
+                {
+                    unityTransport.HeartbeatTimeoutMS = heartBeatTimeout;
+                }
+                networkManager.OnConnectionEvent += OnConnectionEventCallback;
             }
 
             base.OnServerAndClientsCreated();
@@ -73,18 +73,29 @@ private void OnConnectionEventCallback(NetworkManager networkManager, Connection
             switch (data.EventType)
             {
                 case ConnectionEvent.ClientDisconnected:
-                    Assert.IsFalse(data.PeerClientIds.IsCreated);
-                    ++m_ClientDisconnectCount;
-                    break;
+                    {
+                        Assert.IsFalse(data.PeerClientIds.IsCreated);
+                        if (data.ClientId == m_TargetClientId)
+                        {
+                            ++m_ClientDisconnectCount;
+                        }
+                        break;
+                    }
                 case ConnectionEvent.PeerDisconnected:
-                    Assert.IsFalse(data.PeerClientIds.IsCreated);
-                    ++m_PeerDisconnectCount;
-                    break;
+                    {
+                        if (data.ClientId == m_TargetClientId)
+                        {
+                            Assert.IsFalse(data.PeerClientIds.IsCreated);
+                            ++m_PeerDisconnectCount;
+                        }
+                        break;
+                    }
             }
         }
 
         private bool m_TargetClientShutdown;
         private NetworkManager m_TargetClient;
+        private ulong m_TargetClientId;
 
         private void ClientToDisconnect_OnClientStopped(bool wasHost)
         {
@@ -97,10 +108,10 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie
         {
             m_TargetClientShutdown = false;
             m_TargetClient = m_ClientNetworkManagers[disconnectedClient - 1];
+            m_TargetClientId = m_TargetClient.LocalClientId;
             m_TargetClient.OnClientStopped += ClientToDisconnect_OnClientStopped;
-            foreach (var client in m_ClientNetworkManagers)
+            foreach (var client in m_NetworkManagers)
             {
-                client.OnConnectionEvent += OnConnectionEventCallback;
                 if (m_UseHost)
                 {
                     Assert.IsTrue(client.ConnectedClientsIds.Contains(0ul));
@@ -110,15 +121,6 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie
                 Assert.IsTrue(client.ConnectedClientsIds.Contains(3ul));
                 Assert.AreEqual(client.ServerIsHost, m_UseHost);
             }
-            m_ServerNetworkManager.OnConnectionEvent += OnConnectionEventCallback;
-            if (m_UseHost)
-            {
-                Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(0ul));
-            }
-            Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(1ul));
-            Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(2ul));
-            Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(3ul));
-            Assert.AreEqual(m_ServerNetworkManager.ServerIsHost, m_UseHost);
 
             // Set up a WaitForMessageReceived hook.
             // In some cases the message will be received during StopOneClient, but it is not guaranteed
@@ -135,6 +137,7 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie
             // Used to determine if all clients received the CreateObjectMessage
             var hooks = new MessageHooksConditional(messageHookEntriesForSpawn);
 
+
             if (clientDisconnectType == ClientDisconnectType.ServerDisconnectsClient)
             {
                 m_ServerNetworkManager.DisconnectClient(disconnectedClient);
@@ -151,52 +154,44 @@ public IEnumerator TestPeerDisconnectCallback([Values] ClientDisconnectType clie
             yield return WaitForConditionOrTimeOut(() => m_TargetClientShutdown);
             AssertOnTimeout($"Timed out waiting for {m_TargetClient.name} to shutdown!");
 
-            foreach (var client in m_ClientNetworkManagers)
+            // Check that the client is disconnected and all NetworkManagers have registered this
+            yield return WaitForConditionOrTimeOut(CheckClientDisconnected);
+            AssertOnTimeout($"Timed out waiting for {m_TargetClient.name} to register as having shutdown!");
+
+            // If disconnected, the server and the client that disconnected will be notified
+            Assert.AreEqual(2, m_ClientDisconnectCount);
+            // Host receives peer disconnect, dedicated server does not
+            Assert.AreEqual(m_UseHost ? 3 : 2, m_PeerDisconnectCount);
+        }
+
+        /// <summary>
+        /// Conditional method to verify the <see cref="m_TargetClientId"/> is disconnected
+        /// and that identifier is not contained on any <see cref="NetworkManager"/> instance's
+        /// <see cref="NetworkManager.ConnectedClientsIds"/>.
+        /// </summary>
+        private bool CheckClientDisconnected(StringBuilder errorLog)
+        {
+            foreach (var networkManager in m_NetworkManagers)
             {
-                if (!client.IsConnectedClient)
+                if (!networkManager.IsConnectedClient)
                 {
-                    Assert.IsEmpty(client.ConnectedClientsIds);
-                    continue;
-                }
-                if (m_UseHost)
-                {
-                    Assert.IsTrue(client.ConnectedClientsIds.Contains(0ul), $"[Client-{client.LocalClientId}][Connected ({client.IsConnectedClient})] Still has client identifier 0!");
-                }
 
-                for (var i = 1ul; i < 3ul; ++i)
-                {
-                    if (i == disconnectedClient)
-                    {
-                        Assert.IsFalse(client.ConnectedClientsIds.Contains(i), $"[Client-{client.LocalClientId}][Connected ({client.IsConnectedClient})] Still has client identifier {i}!");
-                    }
-                    else
-                    {
-                        Assert.IsTrue(client.ConnectedClientsIds.Contains(i), $"[Client-{client.LocalClientId}][Connected ({client.IsConnectedClient})] Still has client identifier {i}!");
-                    }
+                    continue;
                 }
-            }
-            if (m_UseHost)
-            {
-                Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(0ul));
-            }
-
-            for (var i = 1ul; i < 3ul; ++i)
-            {
-                if (i == disconnectedClient)
+                if (networkManager.LocalClientId == m_TargetClientId && ((networkManager.IsConnectedClient) || (networkManager.IsListening)))
                 {
-                    Assert.IsFalse(m_ServerNetworkManager.ConnectedClientsIds.Contains(i));
+                    errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Is either still connected or still listening!");
+                    return false;
                 }
-                else
+                if (networkManager.ConnectedClientsIds.Contains(m_TargetClientId))
                 {
-                    Assert.IsTrue(m_ServerNetworkManager.ConnectedClientsIds.Contains(i));
+                    errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Is still has {m_TargetClientId} registered as a connected client!");
+                    return false;
                 }
             }
-
-            // If disconnected, the server and the client that disconnected will be notified
-            Assert.AreEqual(2, m_ClientDisconnectCount);
-            // Host receives peer disconnect, dedicated server does not
-            Assert.AreEqual(m_UseHost ? 3 : 2, m_PeerDisconnectCount);
+            return true;
         }
 
     }
 }
+