test: Address review feedback on the render time and tick latency tests

- Drop the redundant HostOrServer fixture argument and the UseCMBService
  override; Host is the default and a client-server fixture never runs
  under the CMB service.
- Use WaitForSpawnedOnAllOrTimeOut, GetNonAuthorityNetworkManager and
  WaitForTicks instead of hand rolled equivalents.
- There is only ever one connected client, so drop the collections and
  refer to the single non-authority instance directly.
- Fold the two tick latency tests into one and drop the assertion that
  recomputed the implementation's own formula. What is left is what can
  actually regress: the value does not drift with session time, and it
  grows by exactly the ticks added to the interpolation buffer.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformInterpolationRenderTimeTests.cs

@@ -1,5 +1,4 @@
 using System.Collections;
-using System.Collections.Generic;
 using NUnit.Framework;
 using Unity.Netcode.Components;
 using Unity.Netcode.TestHelpers.Runtime;
@@ -18,8 +17,8 @@ namespace Unity.Netcode.RuntimeTests
     /// can never be less than the tick latency. Deriving the render time from LocalTime eats into that margin by
     /// however far the two clocks are apart, and can push the target past ServerTime entirely.
     /// </remarks>
-    [TestFixture(HostOrServer.Host, NetworkTransform.InterpolationTypes.Lerp)]
-    [TestFixture(HostOrServer.Host, NetworkTransform.InterpolationTypes.SmoothDampening)]
+    [TestFixture(NetworkTransform.InterpolationTypes.Lerp)]
+    [TestFixture(NetworkTransform.InterpolationTypes.SmoothDampening)]
     internal class NetworkTransformInterpolationRenderTimeTests : IntegrationTestWithApproximation
     {
         protected override int NumberOfClients => 1;
@@ -47,19 +46,12 @@ internal class NetworkTransformInterpolationRenderTimeTests : IntegrationTestWit
         private NetworkManager m_AuthorityNetworkManager;
         private NetworkTransform m_AuthorityInstance;
         private Vector3 m_Direction;
-        private int m_TickCount;
 
-        public NetworkTransformInterpolationRenderTimeTests(HostOrServer hostOrServer, NetworkTransform.InterpolationTypes interpolationType) : base(hostOrServer)
+        public NetworkTransformInterpolationRenderTimeTests(NetworkTransform.InterpolationTypes interpolationType)
         {
             m_InterpolationType = interpolationType;
         }
 
-        // TODO: [CmbServiceTests] ServerTime's meaning under a CMB service session has not been verified.
-        protected override bool UseCMBService()
-        {
-            return false;
-        }
-
         protected override void OnServerAndClientsCreated()
         {
             m_TestPrefab = CreateNetworkObjectPrefab("RenderTimeTestObj");
@@ -86,139 +78,70 @@ private static double GetClockLeadInTicks(NetworkManager networkManager)
         /// </summary>
         private void OnNetworkTick()
         {
-            m_TickCount++;
             m_AuthorityInstance.transform.position += m_Direction * k_DistancePerTick;
         }
 
-        private bool AllClientsSpawnedInstance()
-        {
-            foreach (var networkManager in m_NetworkManagers)
-            {
-                if (networkManager == m_AuthorityNetworkManager)
-                {
-                    continue;
-                }
-
-                if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(m_AuthorityInstance.NetworkObject.NetworkObjectId))
-                {
-                    return false;
-                }
-            }
-            return true;
-        }
-
-        private List<NetworkTransform> GetNonAuthorityInstances()
-        {
-            var instances = new List<NetworkTransform>();
-            foreach (var networkManager in m_NetworkManagers)
-            {
-                if (networkManager == m_AuthorityNetworkManager)
-                {
-                    continue;
-                }
-
-                var spawnedObject = networkManager.SpawnManager.SpawnedObjects[m_AuthorityInstance.NetworkObject.NetworkObjectId];
-                instances.Add(spawnedObject.GetComponent<NetworkTransform>());
-            }
-            return instances;
-        }
-
         [UnityTest]
         public IEnumerator RenderTimeTrailsTheServerClock()
         {
             m_AuthorityNetworkManager = GetAuthorityNetworkManager();
             m_AuthorityInstance = SpawnObject(m_TestPrefab, m_AuthorityNetworkManager).GetComponent<NetworkTransform>();
 
-            yield return WaitForConditionOrTimeOut(AllClientsSpawnedInstance);
+            yield return WaitForSpawnedOnAllOrTimeOut(m_AuthorityInstance.NetworkObject);
             AssertOnTimeout($"Not all clients spawned {m_AuthorityInstance.name}!");
 
-            var nonAuthorityInstances = GetNonAuthorityInstances();
-            Assert.IsNotEmpty(nonAuthorityInstances, "There were no non-authority instances to measure!");
+            var nonAuthority = GetNonAuthorityNetworkManager();
+            var nonAuthorityInstance = nonAuthority.SpawnManager.SpawnedObjects[m_AuthorityInstance.NetworkObject.NetworkObjectId].GetComponent<NetworkTransform>();
 
             // Separate the two clocks by a known amount so that which one the render time is derived from is
             // actually distinguishable.
-            foreach (var instance in nonAuthorityInstances)
-            {
-                var networkManager = instance.NetworkManager;
-                networkManager.NetworkTimeSystem.LocalBufferSec = k_LocalBufferTicks * GetTickInterval(networkManager);
-            }
+            nonAuthority.NetworkTimeSystem.LocalBufferSec = k_LocalBufferTicks * GetTickInterval(nonAuthority);
 
             // Start continuous motion on the authority.
             m_Direction = GetRandomVector3(-10, 10).normalized;
-            m_TickCount = 0;
             m_AuthorityNetworkManager.NetworkTickSystem.Tick += OnNetworkTick;
 
             // The offset only moves when the client next receives a time sync, so wait for the separation to
             // actually take hold rather than assuming it has.
-            yield return WaitForConditionOrTimeOut(() =>
-            {
-                foreach (var instance in nonAuthorityInstances)
-                {
-                    if (GetClockLeadInTicks(instance.NetworkManager) < k_RequiredLeadTicks)
-                    {
-                        return false;
-                    }
-                }
-                return true;
-            });
-            AssertOnTimeout($"The client clocks never separated by {k_RequiredLeadTicks} ticks, so this test " +
-                $"cannot tell the two clocks apart and would pass regardless of which one is used.");
+            yield return WaitForConditionOrTimeOut(() => GetClockLeadInTicks(nonAuthority) >= k_RequiredLeadTicks);
+            AssertOnTimeout($"The nonAuthority clock never fell {k_RequiredLeadTicks} ticks behind, so this test " +
+                "cannot tell the two clocks apart and would pass regardless of which one is used.");
 
             // Let the interpolator settle at the new separation before measuring.
-            var warmUpTarget = m_TickCount + k_WarmUpTicks;
-            yield return WaitForConditionOrTimeOut(() => m_TickCount >= warmUpTarget);
-            AssertOnTimeout("Timed out waiting for the authority to keep moving!");
+            yield return WaitForTicks(m_AuthorityNetworkManager, k_WarmUpTicks);
 
             // Sample how far behind ServerTime the state being interpolated towards was sent.
-            var totalTargetLagTicks = new Dictionary<NetworkTransform, double>();
-            var totalBuffered = new Dictionary<NetworkTransform, int>();
-            var samples = new Dictionary<NetworkTransform, int>();
-            foreach (var instance in nonAuthorityInstances)
-            {
-                totalTargetLagTicks.Add(instance, 0.0d);
-                totalBuffered.Add(instance, 0);
-                samples.Add(instance, 0);
-            }
-
+            var interpolator = nonAuthorityInstance.GetPositionInterpolator();
+            var totalTargetLagTicks = 0.0d;
+            var totalBuffered = 0;
+            var samples = 0;
             for (int frame = 0; frame < k_SampledFrames; frame++)
             {
-                foreach (var instance in nonAuthorityInstances)
+                if (interpolator.InterpolateState.Target.HasValue)
                 {
-                    var interpolator = instance.GetPositionInterpolator();
-                    if (!interpolator.InterpolateState.Target.HasValue)
-                    {
-                        continue;
-                    }
-
-                    var networkManager = instance.NetworkManager;
-                    var targetLag = networkManager.ServerTime.Time - interpolator.InterpolateState.Target.Value.TimeSent;
-                    totalTargetLagTicks[instance] += targetLag / GetTickInterval(networkManager);
-                    totalBuffered[instance] += interpolator.m_BufferQueue.Count;
-                    samples[instance]++;
+                    var targetLag = nonAuthority.ServerTime.Time - interpolator.InterpolateState.Target.Value.TimeSent;
+                    totalTargetLagTicks += targetLag / GetTickInterval(nonAuthority);
+                    totalBuffered += interpolator.m_BufferQueue.Count;
+                    samples++;
                 }
                 yield return null;
             }
 
             m_AuthorityNetworkManager.NetworkTickSystem.Tick -= OnNetworkTick;
 
-            foreach (var instance in nonAuthorityInstances)
-            {
-                Assert.Greater(samples[instance], 0, $"{instance.name} never had a state to interpolate towards!");
-
-                var networkManager = instance.NetworkManager;
-                var meanTargetLagTicks = totalTargetLagTicks[instance] / samples[instance];
-                var meanBuffered = totalBuffered[instance] / (float)samples[instance];
-                var tickLatency = networkManager.NetworkTimeSystem.TickLatency;
-
-                // Anything less than the tick latency means the render time came from a clock that leads the one
-                // the states are stamped on.
-                Assert.GreaterOrEqual(meanTargetLagTicks, tickLatency,
-                    $"[{m_InterpolationType}] {instance.name} was interpolating towards a state sent " +
-                    $"{meanTargetLagTicks:F3} ticks behind the server clock, but the render time is the server " +
-                    $"clock minus a tick latency of {tickLatency}, so it should never be less than that. " +
-                    $"(clock lead {GetClockLeadInTicks(networkManager):F3} ticks, mean buffered {meanBuffered:F3}). " +
-                    $"The render time is being derived from a clock that leads the one state updates are stamped on.");
-            }
+            Assert.Greater(samples, 0, $"{nonAuthorityInstance.name} never had a state to interpolate towards!");
+
+            var meanTargetLagTicks = totalTargetLagTicks / samples;
+            var meanBuffered = totalBuffered / (float)samples;
+            var tickLatency = nonAuthority.NetworkTimeSystem.TickLatency;
+
+            // Anything less than the tick latency means the render time came from a clock that leads the one
+            // the states are stamped on.
+            Assert.GreaterOrEqual(meanTargetLagTicks, tickLatency,
+                $"[{m_InterpolationType}] {nonAuthorityInstance.name} was interpolating towards a state sent " +
+                $"{meanTargetLagTicks:F3} ticks behind the server clock, but the render time is the server " +
+                $"clock minus a tick latency of {tickLatency}, so it should never be less than that. " +
+                $"(clock lead {GetClockLeadInTicks(nonAuthority):F3} ticks, mean buffered {meanBuffered:F3})");
         }
     }
 }

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTickLatencyTests.cs

@@ -43,56 +43,38 @@ protected override IEnumerator OnTearDown()
             return base.OnTearDown();
         }
 
-        private static float GetExpectedLatencyInSeconds(NetworkManager networkManager)
-        {
-            var ticksBehind = networkManager.NetworkTimeSystem.TickLatency + NetworkTransform.InterpolationBufferTickOffset;
-            return (float)(ticksBehind * networkManager.ServerTime.FixedDeltaTimeAsDouble);
-        }
-
         [UnityTest]
-        public IEnumerator GetTickLatencyInSecondsReturnsADuration()
+        public IEnumerator GetTickLatencyInSecondsReturnsADurationNotATimestamp()
         {
-            var client = m_ClientNetworkManagers[0];
+            var client = GetNonAuthorityNetworkManager();
+            var tickInterval = (float)client.ServerTime.FixedDeltaTimeAsDouble;
 
-            // Sample repeatedly while the session clock advances. A duration tracks the tick latency, where an
-            // absolute timestamp would climb by roughly one second per second.
-            //
-            // NetworkTimeSystem.TickLatency is adaptive and can legitimately change mid-run, so the value is
-            // only held to being unchanged across samples where the tick latency itself did not change.
+            // A timestamp climbs by roughly a second per second, so sample while the session clock advances
+            // and hold the value to only moving when the tick latency it is derived from moves. That latency
+            // is adaptive and can legitimately change mid-run.
             var previousTicksBehind = -1;
             var previousValue = 0f;
             for (int i = 0; i < k_Samples; i++)
             {
                 var ticksBehind = client.NetworkTimeSystem.TickLatency + NetworkTransform.InterpolationBufferTickOffset;
-                var expected = GetExpectedLatencyInSeconds(client);
-                var actual = NetworkTransform.GetTickLatencyInSeconds(client);
-
-                Assert.AreEqual(expected, actual, k_Tolerance,
-                    $"Expected the tick latency to be {expected}s but it was {actual}s.");
+                var latency = NetworkTransform.GetTickLatencyInSeconds(client);
 
+                Assert.Greater(latency, 0f, "A latency of zero or less is not a duration this can be measured against.");
                 if (ticksBehind == previousTicksBehind)
                 {
-                    Assert.AreEqual(previousValue, actual, k_Tolerance,
-                        $"The reported latency moved from {previousValue}s to {actual}s while the tick latency " +
+                    Assert.AreEqual(previousValue, latency, k_Tolerance,
+                        $"The reported latency moved from {previousValue}s to {latency}s while the tick latency " +
                         $"stayed at {ticksBehind} ticks, so it is tracking elapsed time rather than latency.");
                 }
 
                 previousTicksBehind = ticksBehind;
-                previousValue = actual;
+                previousValue = latency;
                 yield return null;
             }
-        }
-
-        [UnityTest]
-        public IEnumerator GetTickLatencyInSecondsTracksTheBufferTickOffset()
-        {
-            var client = m_ClientNetworkManagers[0];
-            var tickInterval = (float)client.ServerTime.FixedDeltaTimeAsDouble;
 
+            // Buffering more ticks has to lengthen the reported duration by exactly those ticks.
             var latencyBefore = client.NetworkTimeSystem.TickLatency;
             var before = NetworkTransform.GetTickLatencyInSeconds(client);
-
-            // Buffering more ticks has to lengthen the reported duration by exactly those ticks.
             NetworkTransform.InterpolationBufferTickOffset = m_OriginalBufferTickOffset + k_AddedBufferTicks;
             yield return null;