chore: enable unified hybrid tests - #4122
Open
NoelStephensUnity wants to merge 6 commits into
Open
Conversation
Adding an additional unified hybrid spawning test pass.
Fixed: - Issue with NetworkObject throwing an exception during hybrid integration test. - Issue with setting the active world prior to spawning a hybrid prefab during integration tests. - Issue with UnifiedNetcodeUpdateSystem not overriding the OnCreate method along with checks for a valid NetworkManager and/or transport within OnUpdate. - Issues (minor) with the original UnifiedNetworkTransformTest. - Adjustments to the NetcodeIntegrationTest that assures the correct active world is assigned when spawning.
Member
Author
There was a problem hiding this comment.
This "pins" the NGO version of the unified NetcodeConfig to avoid having one auto-created.
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop-3.x.x #4122 +/- ##
==============================================
Coverage 78.01% 78.01%
==============================================
Files 153 153
Lines 26254 26254
==============================================
Hits 20483 20483
Misses 5771 5771
Flags with carried forward coverage won't be shown. Click here to find out more.
|
EmandM
reviewed
Aug 17, 2026
| # 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 |
Member
There was a problem hiding this comment.
We don't want to run all unified tests on the PR trigger. I know there's only one job there for now, but we should point to that job directly. Each yamato job can slow down landing PRs when yamato is being slow
Comment on lines
+93
to
+99
|
|
||
| # 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 | ||
|
|
Member
There was a problem hiding this comment.
Also, this comment is superfluous
Suggested change
| # 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 | |
| - .yamato/_run-all.yml#run_all_unified_tests |
Comment on lines
+131
to
+134
| # 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). |
Member
There was a problem hiding this comment.
Suggested change
| # 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). | |
| # This job allows the Unified tests to be kicked off by commenting "/ci unified". | |
| # This is useful for PRs where pr_code_changes_checks doesn't trigger. |
Comment on lines
+181
to
+182
| # Run the unified (NGO + N4E) tests on their own pinned editor (see .yamato/unified-tests.yml) | ||
| - .yamato/_run-all.yml#run_all_unified_tests |
Member
There was a problem hiding this comment.
Context is clear from the upper two lines.
Suggested change
| # Run the unified (NGO + N4E) tests on their own pinned editor (see .yamato/unified-tests.yml) | |
| - .yamato/_run-all.yml#run_all_unified_tests | |
| - .yamato/_run-all.yml#run_all_unified_tests |
| pinnedTrunk: 5fe7931aab8c4fff9274e15ef0800125c68b8d6a | ||
|
|
||
|
|
||
| # UNIFIED (NGO + N4E) CONFIGURATION--------------------------------------------------------------------- |
Member
There was a problem hiding this comment.
I don't love this duplication of the already existing structure. it'd be much nicer if this could be integrated with the existing metafile structure.
Comment on lines
+3971
to
+3981
| // 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; | ||
| } | ||
|
|
||
|
|
Member
There was a problem hiding this comment.
This if check should be combined with the check below
Comment on lines
+127
to
+133
| // 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. |
Member
There was a problem hiding this comment.
Code is self-explanatory. No comment necessary.
Suggested change
| // 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. |
Comment on lines
+166
to
+168
| // 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. |
Member
There was a problem hiding this comment.
Same here.
Suggested change
| // 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. |
| // N4E's rate managers reassign that singleton on every world update, so by the time a test body runs | ||
| // it points at whichever world updated last - typically a client world - and the spawn is rejected with | ||
| // "You can only spawn a ghost on a server or during prediction on a client." | ||
| // TODO-FixMe: NetCode.Netcode.Instance is a singleton and might cause issues assigning this. |
Member
There was a problem hiding this comment.
I don't love a TODO-FixMe. It's not a pattern we have in the codebase today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
Stands up a dedicated CI job that runs the unified (NGO + N4E) hybrid-prefab integration tests, and
fixes the three bugs that were preventing
UnifiedNetworkTransformTestfrom passing.A "hybrid prefab" is an NGO prefab that also carries a
GhostObject. When one or more are present inthe
NetworkManager's prefab list, NGO hands transform synchronization to N4E's snapshot system andtunnels its own batched messages over
UnifiedNetcodeTransportinstead of aNetworkTransport.UnifiedNetworkTransformTestis the validation test that this path works end to end.Why a separate job rather than folding this into the existing ones
com.unity.netcodewith the unified API. That editor is not one ofthe
validation_editors, and NGO still has to build and test against editors with no unified API.com.unity.netcodein the testproject soUNIFIED_NETCODEis defined (via theversionDefinesin the asmdefs). The committedmanifest.jsondeliberately does not reference it,so the job swaps in
testproject/Packages/manifest-unified.json.is exercised on the alpha editor.
The pin is 6000.7.0a5, not 6000.7.0a2
The original plan was to pin a2 (breakpoints are more reliable there). a2 turns out to be unusable —
it cannot compile the unified code at all. Both alphas bundle
com.unity.netcode6.7.0, so thepackage version does not distinguish them; only the trunk snapshot does. Verified by reading the
sources bundled in each editor:
IOutOfBandRpcCommandOutgoingOutOfBandRpcDataStreamBufferUnifiedNetcodeTransportis gated on#if UNIFIED_NETCODE && OUT_OF_BAND_RPCand needs both.Separately, a2 still names the component
GhostAdapter; a5 renamed it toGhostObject, which is whatthe test helpers call. Two independent blockers, so a2 is off the table regardless of debugger quality.
Runtime fixes
UnifiedNetcodeUpdateSystem.OnCreateused theISystemsignatureOnCreate(ref SystemState)on aSystemBase. Withoutoverridethat is a new method Entities never calls, so neitherRequireForUpdatetook effect andOnUpdateran from the first world tick — beforeStartClient/StartServerassignTransportandNetworkManager.CreateSingleWorldHostcallsAppendWorldToCurrentPlayerLoop, so the world is live immediately and any tick in that window was aNullReferenceException. Corrected to the parameterless override, plus a null guard inOnUpdate.NetcodeIntegrationTest.SpawnObjectsetNetcode.Instance.m_ActiveWorldafterObject.Instantiate. The hybrid prefab is active, so the clone'sGhostObject.Awakerunssynchronously inside
Instantiate; the clone is not a prefab, so Awake acquires an entity referenceand resolves the target world from that singleton. N4E's rate managers reassign it on every world
update, so it pointed at whichever client world updated last and the spawn was rejected with
"You can only spawn a ghost on a server or during prediction on a client." Assignment hoisted above
Instantiate.NetworkObject.InitGhostthrew when a hybrid prefab was created before anyNetworkManagerexists,which is the normal ordering in integration tests. Now returns early on a null
NetworkManager.CreateHybridPrefabalso now callsGhostObject.InitializeAsPrefab()instead of hand-rolling theGhostPrefabReferencesetup, which picks up N4E'stry/finallyreset ofs_IsPostProcessing.UnifiedNetworkTransformTestwas hardened: position is validated on initial spawn as well as afterthe move, and the fixed
WaitForSeconds(1)is replaced withWaitForConditionOrTimeOutreportingwhich client diverged and by how much.
Tradeoffs worth reviewer attention
manifest-unified.jsonis a full copy ofmanifest.json. It differs by morethan the N4E entry:
manifest.jsonpins the builtin versions that exist on 6000.6(
addressables 2.11.1,timeline 6.6.0,ugui 2.6.0), which do not resolve on 6000.7.0a5. Thatmeans hand-syncing two manifests until N4E becomes a hard NGO dependency, at which point this goes
away. I could not find a way to avoid the duplication without dropping 6000.6 support.
unified_test_filter: "*Unified*"matches the NUnit full testname, which covers the dedicated fixtures and is intended to also cover shared fixtures
parameterized with
HostOrServer.UnifiedHost/UnifiedServer. An NUnit[Category]would be lessfragile once more fixtures gain unified variants; there is no
[Category]usage in the package todayand
UnifiedTestRunner's--categorysupport is unconfirmed, so this is deliberately deferred.run_all_unified_testsis a dependency ofpr_code_changes_checks,wired the same way as the CMB service tests. That puts an unsupported alpha editor into the mandatory
gate: when N4E lands breaking changes in trunk this goes red and the pin has to be bumped before any
PR can go green. Chosen deliberately over an opt-in trigger so unified breakage is noticed
immediately.
unified_pr_checks(/ci unified) remains for PRs the main gate does not cover.unified_editors.default,unified_netcode_version, and thecom.unity.netcodeversion inmanifest-unified.json.Known gap: the CI job runs editor playmode; verification so far is a standalone player run.
See Testing & QA below.
Jira ticket
MTT-XXXX
Documentation
All changes are either CI configuration, test helpers, or runtime code behind
#if UNIFIED_NETCODE,which cannot be defined without
com.unity.netcodeinstalled. There is no public API surface changeand no behaviour change for any current NGO user.
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Manual testing :
Manual testing doneA standalone player build containing only
UnifiedNetworkTransformTestruns green locally(Windows, Mono, 6000.7.0a5). Before the three fixes above this failed with
SetUp : Failed to start instancesplus aNullReferenceExceptionimmediately after"Starting a world for Host".
Also unverified: that
com.unity.netcode: 6.7.0resolves to the builtin copy frommanifest-unified.jsonon a clean checkout (locally it was added by hand tomanifest.json), and thatYamato accepts the new YAML.
Automated tests:
Covered by existing automated testsUnifiedNetworkTransformTestalready existed; this PR fixes what it was failing on and strengthens itsassertions. The new coverage is at the CI level — the test now actually runs in CI, which it never did
before, since
UNIFIED_NETCODEwas never defined in any existing job.Does the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?No QA involvement requested: this is CI infrastructure plus an experimental code path that is compiled
out of every shipping configuration.
Up-port
Not needed. This PR targets
develop-3.x.xdirectly. The unified API does not exist in NGO v2.x, andall runtime changes are behind
UNIFIED_NETCODE, which cannot be defined there.Backports
Not needed, for the same reason — this is specific to the NGOv3.X unified work.