chore: enable unified hybrid tests

.yamato/_run-all.yml

@@ -368,6 +368,21 @@ run_all_project_tests_console_standalone_default:
 {% endfor -%}
 
 
+# Runs all unified (NGO + N4E) tests
+# These run on their own pinned editor (unified_editors) rather than the validation_editors, because
+# they need an editor that bundles a com.unity.netcode with the unified API. See unified-tests.yml.
+run_all_unified_tests:
+  name: Run All Unified Tests
+  dependencies:
+{% for project in projects.default -%}
+{% for platform in unified_test_platforms -%}
+{% for editor in unified_editors.default -%}
+    - .yamato/unified-tests.yml#unified_test_{{ project.name }}_{{ platform.name }}_{{ editor }}
+{% endfor -%}
+{% endfor -%}
+{% endfor -%}
+
+
 # Runs all CMB service tests
 run_all_project_tests_cmb_service:
   name: Run All CMB Service Tests

.yamato/_triggers.yml

@@ -90,6 +90,13 @@ pr_code_changes_checks:
     # Note that our daily tests will anyway run both test configurations in "minimal supported" and "trunk" configurations
     - .yamato/desktop-standalone-tests.yml#desktop_standalone_test_testproject_ubuntu_il2cpp_{{ pinnedTrunk }}
     - .yamato/cmb-service-standalone-tests.yml#cmb_service_standalone_test_testproject_ubuntu_il2cpp_{{ pinnedTrunk }}
+
+    # Run the unified (NGO + N4E) tests. Unlike every other job here this one runs on a pinned Unity
+    # alpha (unified_editors in project.metafile) rather than a supported editor, because it needs an
+    # editor that bundles a com.unity.netcode with the unified API. Expect it to need a pin bump
+    # whenever N4E lands breaking changes in trunk. See .yamato/unified-tests.yml.
+    - .yamato/_run-all.yml#run_all_unified_tests
+
     # Run code coverage test (PRs use the pinned "safe" trunk)
     - .yamato/code-coverage.yml#code_coverage_project_test_testproject_ubuntu_{{ pinnedTrunk }}
   triggers:
@@ -120,6 +127,21 @@ pr_code_changes_checks:
 
 
 
+# Unified (NGO + N4E) validation, on demand.
+# The unified tests run automatically as part of pr_code_changes_checks and develop_nightly (same as
+# the CMB service tests do). This job only exists so they can also be kicked off by commenting
+# "/ci unified", for PRs that pr_code_changes_checks does not cover (drafts, doc-only changes, or
+# branches targeting something other than develop / develop-3.x.x / release).
+unified_pr_checks:
+  name: Unified (NGO + N4E) checks [on demand]
+  dependencies:
+    - .yamato/_run-all.yml#run_all_unified_tests
+  triggers:
+    expression: |-
+      pull_request.comment eq "unified"
+    cancel_old_ci: true
+
+
 # Run all tests on nightly basis.
 # Same subset as pull_request_trigger with addition of mobile/desktop/console tests and webgl builds
 # Those tests are all running on trunk and the default editor (since it's daily and running all of them would add a lot of overhead)
@@ -156,6 +178,8 @@ develop_nightly:
     # Run Runtime tests against cmb service on trunk and default editors
     - .yamato/_run-all.yml#run_all_project_tests_cmb_service_trunk
     - .yamato/_run-all.yml#run_all_project_tests_cmb_service_default
+    # Run the unified (NGO + N4E) tests on their own pinned editor (see .yamato/unified-tests.yml)
+    - .yamato/_run-all.yml#run_all_unified_tests
     # Build player for webgl platform on trunk and default editors
     - .yamato/project-updated-dependencies-test.yml#updated-dependencies_testproject_NGO_ubuntu_trunk
     - .yamato/project-updated-dependencies-test.yml#updated-dependencies_testproject_NGO_win_{{ validation_editors.default }}

.yamato/project.metafile

@@ -190,6 +190,55 @@ validation_editors:
 pinnedTrunk:  5fe7931aab8c4fff9274e15ef0800125c68b8d6a
 
 
+# UNIFIED (NGO + N4E) CONFIGURATION---------------------------------------------------------------------
+# The unified test job validates NGO running against Netcode for Entities (N4E) through the unified API.
+# It is deliberately kept separate from every other job in this repo because it needs an editor that
+# bundles com.unity.netcode with the unified API, and that editor is NOT one of the validation_editors.
+#
+# WHY THIS IS PINNED TO 6000.7.0a5 (and not 6000.7.0a2):
+#   UnifiedNetcodeTransport is compiled behind "#if UNIFIED_NETCODE && OUT_OF_BAND_RPC" and needs the
+#   N4E out-of-band RPC API (IOutOfBandRpcCommand / OutgoingOutOfBandRpcDataStreamBuffer).
+#   Both 6000.7.0a2 and 6000.7.0a5 bundle com.unity.netcode 6.7.0, but that API only landed in the a5
+#   snapshot. On a2 the unified transport does not compile, so the unified tests cannot run there.
+#   Bump this pin (and unified_netcode_version below) together whenever the required N4E API moves.
+#
+# An explicit alpha version is used instead of a trunk revision hash because published alphas are
+# already immutable, so there is nothing to pin against.
+unified_editors:
+    default:
+        - 6000.7.0a5
+
+# Version of com.unity.netcode (N4E) bundled with unified_editors.default.
+# It resolves as a "builtin" package out of the editor install, so it must match the editor exactly.
+unified_netcode_version: 6.7.0
+
+# The unified job runs in Editor context only, so a single fast platform is enough.
+unified_test_platforms:
+    - name: ubuntu
+      type: Unity::VM
+      image: package-ci/ubuntu-22.04:v4.87.0
+      flavor: b1.large
+
+# Restricts the unified job to unified-only tests so nothing else runs on the alpha editor.
+#
+# This is a REGULAR EXPRESSION, not a glob. UTR passes it to the editor as -testFilter, which ends up
+# in UnityEngine.TestRunner's FullNameFilter -> NUnit ValueMatchFilter -> new Regex(pattern).IsMatch().
+# A glob-style "*Unified*" throws "Quantifier {x,y} following nothing" and fails the whole run.
+# The other jobs' "Unity.Netcode.RuntimeTests.*" works because it is also a valid regex - it just
+# happens to read like a glob.
+#
+# The match is against the NUnit *full* test name, which includes both fixture and method arguments.
+#
+# DO NOT widen this to ".*Unified.*". "Unified" appearing in a test name does NOT mean that test was
+# deliberately converted to run against hybrid prefabs. HostOrServer gained UnifiedServer/UnifiedHost
+# members under UNIFIED_NETCODE, and NUnit expands a bare [Values] on an enum parameter (and
+# enum-typed fixture constructors) to every member. So with N4E installed, much of the existing suite
+# silently grows unified cases - e.g. NetworkVariableTests(Default).AllNetworkVariableTypes(UnifiedHost),
+# which nobody wrote. ".*Unified.*" selected 138 tests, 84 of which failed, against the 1 test this
+# job exists to validate. Deciding which of those should pass, and how they opt in, is Goal-2.
+unified_test_filter: ".*UnifiedNetworkTransformTest.*"
+
+
 # Scripting backends used by Standalone RunTimeTests---------------------------------------------------
 
 scripting_backends:

.yamato/unified-tests.yml

@@ -0,0 +1,78 @@
+{% metadata_file .yamato/project.metafile %} # All configuration that is used to create different configurations (used in for loops) is taken from this file.
+---
+
+# DESCRIPTION--------------------------------------------------------------------------
+  # This job validates NGO running against Netcode for Entities (N4E) through the unified API.
+  # A "hybrid prefab" is an NGO prefab (NetworkObject at the root) that also carries a GhostObject.
+  # When one or more hybrid prefabs are in the NetworkManager's prefab list, NGO hands transform
+  # synchronization to N4E's snapshot system and tunnels its own batched messages over
+  # UnifiedNetcodeTransport (N4E's out-of-band RPC) instead of using a NetworkTransport such as UTP.
+  # UnifiedNetworkTransformTest is the validation test that this path works end to end.
+
+# WHY THIS JOB IS SEPARATE FROM EVERY OTHER JOB------------------------------------------
+  # 1. It needs an editor that bundles a com.unity.netcode with the unified API. That editor
+  #    (unified_editors.default in project.metafile) is not one of the validation_editors, and NGO
+  #    still has to keep building and testing against editors that have no unified API at all.
+  # 2. It needs com.unity.netcode in the testproject so that the UNIFIED_NETCODE define is set
+  #    (see the versionDefines in Unity.Netcode.Runtime.asmdef). The committed
+  #    testproject/Packages/manifest.json deliberately does NOT reference it, so this job swaps in
+  #    testproject/Packages/manifest-unified.json instead.
+  # 3. It is wired into pr_code_changes_checks and develop_nightly the same way the CMB service tests
+  #    are, so it runs automatically on PRs targeting develop / develop-3.x.x / release. Be aware that
+  #    this makes an unsupported alpha editor part of the PR gate: when N4E lands breaking changes in
+  #    trunk this job goes red and the pin has to be bumped to unblock PRs.
+  #    _triggers.yml also has unified_pr_checks, so it can be kicked off with "/ci unified" on PRs
+  #    that pr_code_changes_checks does not cover.
+
+# CONFIGURATION STRUCTURE--------------------------------------------------------------
+  # Jobs are generated using nested loops:
+    # 1. For all unified test platforms (currently Ubuntu only, see project.metafile)
+    # 2. For all unified editors (currently a single pinned alpha, see project.metafile)
+
+# TECHNICAL CONSIDERATIONS---------------------------------------------------------------
+  # This job runs in Editor context only (no player builds required), like project-tests.yml.
+  # Only playmode is run: every unified test is an integration test and there are no unified EditMode tests.
+  # The run is restricted to unified tests via unified_test_filter so that nothing else is exercised
+  # on the alpha editor. Non-unified tests are covered by the regular jobs on the supported editors.
+  # packages-lock.json is removed because the committed lock was resolved against a much older editor
+  # and its builtin package versions do not exist in the unified editor.
+
+# QUALITY CONSIDERATIONS--------------------------------------------------------------------
+  # TODO: the manifest swap means testproject/Packages/manifest-unified.json has to be kept in sync
+  #       with manifest.json by hand. Revisit once N4E is a hard dependency of NGO and one manifest
+  #       can cover both cases.
+  # TODO: unified_test_filter is a test-name pattern. Replacing it with an NUnit category (for
+  #       example [Category("Unified")]) would be less fragile once more fixtures gain unified variants.
+
+#------------------------------------------------------------------------------------
+
+{% for project in projects.default -%}
+{% for platform in unified_test_platforms -%}
+{% for editor in unified_editors.default -%}
+unified_test_{{ project.name }}_{{ platform.name }}_{{ editor }}:
+  name : Unified Test - NGO {{ project.name }} [{{ platform.name }}, {{ editor }}]
+  agent:
+    type: {{ platform.type }}
+    image: {{ platform.image }}
+    flavor: {{ platform.flavor }}
+{% if platform.model %}
+    model: {{ platform.model }} # This is set only in platforms where we want non-default model to use (more information in project.metafile)
+{% endif %}
+  commands:
+    # Swap in the manifest that pulls in com.unity.netcode (N4E). This is what causes UNIFIED_NETCODE
+    # to be defined and therefore what makes the unified tests compile at all.
+    - cp {{ project.path }}/Packages/manifest-unified.json {{ project.path }}/Packages/manifest.json
+    # The committed lock was resolved against a supported editor, not the unified alpha.
+    - rm -f {{ project.path }}/Packages/packages-lock.json
+
+    - unity-downloader-cli --fast --wait -u {{ editor }} -c Editor # Installing basic editor for tests execution
+    - UnifiedTestRunner --testproject={{ project.path }} --suite=playmode --artifacts-path=test-results --editor-location=.Editor --testfilter="{{ unified_test_filter }}" --rerun-strategy=Test --retry={{ num_test_retries }} --clean-library-on-rerun --timeout={{ test_timeout }}
+  artifacts:
+    logs:
+      paths:
+        - "test-results/**/*"
+  dependencies:
+    - .yamato/_run-all.yml#run_quick_checks # initial checks to perform fast validation of common errors
+{% endfor -%}
+{% endfor -%}
+{% endfor -%}

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -3968,6 +3968,17 @@ private void Start()
 
         private void InitGhost()
         {
+            // Note: If hybrid prefabs are created prior to any NetworkManager instances,
+            // then the next line throws and exception. This avoids that issue.
+            // We might come up with some global way to verify if we are running integration
+            // tests and add additional logic within to determine if we should log an error
+            // or not.
+            if (NetworkManager == null)
+            {
+                return;
+            }
+
+
             if (!NetworkManager.IsListening)
             {
                 if (NetworkManager.LogLevel == LogLevel.Developer)

com.unity.netcode.gameobjects/Runtime/Transports/Unified/UnifiedNetcodeTransport.cs

@@ -124,10 +124,18 @@ public void OnUpdate(ref SystemState state)
     [UpdateBefore(typeof(RpcSystem))]
     internal partial class UnifiedNetcodeUpdateSystem : SystemBase
     {
-        public void OnCreate(ref SystemState state)
+        // Note: this must be the parameterless ComponentSystemBase.OnCreate override, not the
+        // ISystem-style OnCreate(ref SystemState). With the ISystem signature this is just a new
+        // method that Entities never calls, so neither RequireForUpdate takes effect and OnUpdate
+        // runs from the very first world tick - which is before StartClient/StartServer have
+        // assigned Transport and NetworkManager below, and therefore a NullReferenceException.
+        // CreateSingleWorldHost calls AppendWorldToCurrentPlayerLoop, so the world can tick in the
+        // window between world creation and the transport being started.
+        protected override void OnCreate()
         {
-            state.RequireForUpdate<RpcCollection>();
-            state.RequireForUpdate<NetworkId>();
+            RequireForUpdate<RpcCollection>();
+            RequireForUpdate<NetworkId>();
+            base.OnCreate();
         }
 
         public UnifiedNetcodeTransport Transport;
@@ -155,6 +163,14 @@ public void SendRpc(TransportRpc rpc, Entity connectionEntity)
 
         protected override void OnUpdate()
         {
+            // Belt and braces alongside the RequireForUpdate gating in OnCreate: these are only
+            // assigned once the transport is started but the world can already be in the player loop
+            // before that happens. Exit early under this scenario.
+            if (NetworkManager == null || Transport == null)
+            {
+                return;
+            }
+
             NetworkManager.MessageManager.ProcessSendQueues();
 
             using var commandBuffer = new EntityCommandBuffer(Allocator.Temp);

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

@@ -1,5 +1,6 @@
 #if UNIFIED_NETCODE
 using System.Collections;
+using System.Text;
 using NUnit.Framework;
 using Unity.Netcode.Components;
 using Unity.Netcode.TestHelpers.Runtime;
@@ -52,38 +53,45 @@ protected override IEnumerator OnSetup()
             return base.OnSetup();
         }
 
+        private bool ValidatePosition(StringBuilder errorLog)
+        {
+            var authorityPosition = m_AuthorityInstanceTransform.position;
+            foreach (var client in m_ClientNetworkManagers)
+            {
+                var instancePosition = client.SpawnManager.SpawnedObjects[m_Instance.NetworkObjectId].transform.position;
+                if (!Approximately(authorityPosition, instancePosition))
+                {
+                    errorLog.AppendLine($"[Client-{client.LocalClientId}] Clone instance position {GetVector3Values(instancePosition)} " +
+                        $"does not equal the authority's {GetVector3Values(authorityPosition)}");
+                }
+            }
+            return errorLog.Length == 0;
+        }
+
+        private Transform m_AuthorityInstanceTransform;
+
         [UnityTest]
         public IEnumerator BasicMovementTest()
         {
             var authority = GetAuthorityNetworkManager();
             m_Instance = SpawnObject(m_Prefab, m_ServerNetworkManager).GetComponent<NetworkObject>();
-
-            // Wait 5 seconds so we will dump any deferred messages if it failed on clients
-            // when checking to see if it spawned or not on the clients next.
-            // Enable this to debug deferred
-            //yield return new WaitForSeconds(5);
+            m_AuthorityInstanceTransform = m_Instance.transform;
 
             yield return WaitForSpawnedOnAllOrTimeOut(m_Instance);
             AssertOnTimeout($"Failed to spawn {m_Instance.name} on all clients!");
 
             VerboseDebug("All clients spawned instance!");
 
+            yield return WaitForConditionOrTimeOut(ValidatePosition);
+            AssertOnTimeout($"A client failed to synchronize the position on the initial spawn!");
+
             var originalPos = authority.LocalClient.PlayerObject.transform.position;
             var newPos = originalPos + new Vector3(1, 1, 1);
 
             m_Instance.transform.position = newPos;
 
-            foreach (var client in m_ClientNetworkManagers)
-            {
-                Assert.IsTrue(Approximately(originalPos, s_GlobalNetworkObjects[client.LocalClientId][m_Instance.NetworkObjectId].transform.position));
-            }
-
-            yield return new WaitForSeconds(1);
-
-            foreach (var client in m_ClientNetworkManagers)
-            {
-                Assert.IsTrue(Approximately(newPos, s_GlobalNetworkObjects[client.LocalClientId][m_Instance.NetworkObjectId].transform.position));
-            }
+            yield return WaitForConditionOrTimeOut(ValidatePosition);
+            AssertOnTimeout($"A client failed to synchronize the changed position: {GetVector3Values(newPos)}!");
             VerboseDebug("Test Passed!");
         }
     }