Skip to content

chore: enable unified hybrid tests - #4122

Open
NoelStephensUnity wants to merge 7 commits into
develop-3.x.xfrom
chore/enable-unified-hybrid-tests
Open

chore: enable unified hybrid tests#4122
NoelStephensUnity wants to merge 7 commits into
develop-3.x.xfrom
chore/enable-unified-hybrid-tests

Conversation

@NoelStephensUnity

Copy link
Copy Markdown
Member

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 UnifiedNetworkTransformTest from passing.

A "hybrid prefab" is an NGO prefab that also carries a GhostObject. When one or more are present in
the NetworkManager's prefab list, NGO hands transform synchronization to N4E's snapshot system and
tunnels its own batched messages over UnifiedNetcodeTransport instead of a NetworkTransport.
UnifiedNetworkTransformTest is the validation test that this path works end to end.

Why a separate job rather than folding this into the existing ones

  1. It needs an editor bundling a com.unity.netcode with the unified API. That editor is not one of
    the validation_editors, and NGO still has to build and test against editors with no unified API.
  2. It needs com.unity.netcode in the testproject so UNIFIED_NETCODE is defined (via the
    versionDefines in the asmdefs). The committed manifest.json deliberately does not reference it,
    so the job swaps in testproject/Packages/manifest-unified.json.
  3. Everything unified is experimental, so the run is restricted to unified tests only — nothing else
    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.netcode 6.7.0, so the
package version does not distinguish them; only the trunk snapshot does. Verified by reading the
sources bundled in each editor:

symbol a2 a5
IOutOfBandRpcCommand 0 2
OutgoingOutOfBandRpcDataStreamBuffer 0 20

UnifiedNetcodeTransport is gated on #if UNIFIED_NETCODE && OUT_OF_BAND_RPC and needs both.
Separately, a2 still names the component GhostAdapter; a5 renamed it to GhostObject, which is what
the test helpers call. Two independent blockers, so a2 is off the table regardless of debugger quality.

Runtime fixes

  • UnifiedNetcodeUpdateSystem.OnCreate used the ISystem signature OnCreate(ref SystemState) on a
    SystemBase. Without override that is a new method Entities never calls, so neither
    RequireForUpdate took effect and OnUpdate ran from the first world tick — before
    StartClient/StartServer assign Transport and NetworkManager. CreateSingleWorldHost calls
    AppendWorldToCurrentPlayerLoop, so the world is live immediately and any tick in that window was a
    NullReferenceException. Corrected to the parameterless override, plus a null guard in OnUpdate.
  • NetcodeIntegrationTest.SpawnObject set Netcode.Instance.m_ActiveWorld after
    Object.Instantiate. The hybrid prefab is active, so the clone's GhostObject.Awake runs
    synchronously inside Instantiate; the clone is not a prefab, so Awake acquires an entity reference
    and 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.InitGhost threw when a hybrid prefab was created before any NetworkManager exists,
    which is the normal ordering in integration tests. Now returns early on a null NetworkManager.

CreateHybridPrefab also now calls GhostObject.InitializeAsPrefab() instead of hand-rolling the
GhostPrefabReference setup, which picks up N4E's try/finally reset of s_IsPostProcessing.
UnifiedNetworkTransformTest was hardened: position is validated on initial spawn as well as after
the move, and the fixed WaitForSeconds(1) is replaced with WaitForConditionOrTimeOut reporting
which client diverged and by how much.

Tradeoffs worth reviewer attention

  • Duplicate manifest. manifest-unified.json is a full copy of manifest.json. It differs by more
    than the N4E entry: manifest.json pins 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. That
    means 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.
  • Test selection is a name pattern. unified_test_filter: "*Unified*" matches the NUnit full test
    name, which covers the dedicated fixtures and is intended to also cover shared fixtures
    parameterized with HostOrServer.UnifiedHost/UnifiedServer. An NUnit [Category] would be less
    fragile once more fixtures gain unified variants; there is no [Category] usage in the package today
    and UnifiedTestRunner's --category support is unconfirmed, so this is deliberately deferred.
  • This is in the PR gate. run_all_unified_tests is a dependency of pr_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.
  • Bumping the pin means changing three things together: unified_editors.default,
    unified_netcode_version, and the com.unity.netcode version in manifest-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

  • No documentation changes or additions were necessary.

All changes are either CI configuration, test helpers, or runtime code behind #if UNIFIED_NETCODE,
which cannot be defined without com.unity.netcode installed. There is no public API surface change
and 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 done

A standalone player build containing only UnifiedNetworkTransformTest runs green locally
(Windows, Mono, 6000.7.0a5). Before the three fixes above this failed with
SetUp : Failed to start instances plus a NullReferenceException immediately after
"Starting a world for Host".

Also unverified: that com.unity.netcode: 6.7.0 resolves to the builtin copy from
manifest-unified.json on a clean checkout (locally it was added by hand to manifest.json), and that
Yamato accepts the new YAML.

Automated tests:

  • Covered by existing automated tests

UnifiedNetworkTransformTest already existed; this PR fixes what it was failing on and strengthens its
assertions. The new coverage is at the CI level — the test now actually runs in CI, which it never did
before, since UNIFIED_NETCODE was 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.x directly. The unified API does not exist in NGO v2.x, and
all 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.

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.
Adding the modified NetcodeConfig specific to NGO.
Adding the unified manifest file for running unified tests.
Unified specific settings
@NoelStephensUnity
NoelStephensUnity requested review from a team and EmandM as code owners August 16, 2026 20:26

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "pins" the NGO version of the unified NetcodeConfig to avoid having one auto-created.

Formatting of the unified test filter.
One more time... just running the UnifiedNetworkTransformTest for this phase.
A more integrated one will be coming in the next PR for this effort.
@codecov-github-com

codecov-github-com Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

All 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           
Flag Coverage Δ
NGOv2_project_testproject_ubuntu_pinnedTrunk 77.67% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
....netcode.gameobjects/Runtime/Core/NetworkObject.cs 75.29% <ø> (ø)
Components Coverage Δ
com.unity.netcode.gameobjects 78.01% <ø> (ø)

ℹ️ Need help interpreting these results?

Comment thread .yamato/_triggers.yml
Comment thread .yamato/_triggers.yml
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@NoelStephensUnity NoelStephensUnity Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines above it follow a very similar format. Why is it superfluous to follow that same patten just below this pattern:
# Run standalone test. We run it only on Ubuntu since it's the fastest machine, and it was noted that for example distribution on macOS is taking 40m since we switched to Apple Silicon
# Coverage on other standalone machines is present in Nightly job so it's enough to not run all of them for PRs
# desktop_standalone_test and cmb_service_standalone_test are both reusing desktop_standalone_build dependency so we run those in the same configuration on PRs to reduce waiting time.
# Note that our daily tests will anyway run both test configurations in "minimal supported" and "trunk" configurations

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because those comments relate to the specific jobs that were chosen to run in this trigger. They document that the choice was intentional and that changing that choice has yamato implications.

The comment you have here is adding detail about how the interior of that job is running. That detail is not relevant when looking at the PR triggers. The assumption at the PR triggers level is that the job is an isolated box. A comment is only needed if that assumption is not true.

Comment thread .yamato/_triggers.yml Outdated
Comment thread .yamato/_triggers.yml
Comment thread .yamato/project.metafile
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;
}


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if check should be combined with the check below

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit?
The above is 3 lines of code.

This would be the straight forward approach:

            if (NetworkManager == null || !NetworkManager.IsListening)
            {
                if ((NetworkManager != null && NetworkManager.LogLevel == LogLevel.Developer)
                {
                    Debug.LogWarning($"[{nameof(NetworkObject)}] Did not register because there is no session in progress!");
                }
                return;
            }

This is the only way I could think of to remove the 2nd check:

           var isNetworkManagerNull = NetworkManager == null;

           if (isNetworkManagerNull || !NetworkManager.IsListening)
           {
               if ((!isNetworkManagerNull && NetworkManager.LogLevel == LogLevel.Developer)
               {
                   Debug.LogWarning($"[{nameof(NetworkObject)}] Did not register because there is no session in progress!");
               }
               return;
           }

Both seem a bit more complicated to read than just:

            if (NetworkManager == null)
            {
                return;
            }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the NetworkManager is not listening, you get a nice descriptive error message. When the NetworkManager is null, you have no information about what happened. Feels bad to me

// 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love a TODO-FixMe. It's not a pattern we have in the codebase today.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow... there are 83 uses of TODO that use various formats.

Image

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe TODO-UNIFIED?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! Attach it to a pattern we already have. Specifically TODO-FIXME is introducing a new pattern we don't use.

Co-authored-by: Emma <emma.mcmillan@unity3d.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants