feat: increase hybrid prefab test coverage - #4172
Open
NoelStephensUnity wants to merge 5 commits into
Open
NoelStephensUnity wants to merge 5 commits into
NoelStephensUnity wants to merge 5 commits into
Conversation
N4E worlds and UnifiedNetcodeTransport only start when at least one hybrid prefab is registered. Player prefabs are never hybrid, so a UnifiedHost or UnifiedServer test case that spawns nothing but players silently ran as plain NGO. CreateServerAndClients now registers a UnifiedSessionPrefab when a hybrid test case registered no hybrid prefab of its own.
Adds UnifiedHost/UnifiedServer fixtures and the UseUnifiedTests opt-in to 25 fixtures covering RPCs, NetworkVariables and collections, connection approval, spawn/despawn, ownership, late-join synchronization and NGO parenting of hybrid objects. Ghosts spawn from a snapshot, later than NGO's spawn message, so tests that read SpawnedObjects immediately after a spawn or a late join now wait with WaitForSpawnedOnAllOrTimeOut. ComponentControllerTests' predicates were missing a continue and threw instead of waiting. RpcTests, RpcTypeSerializationTests and ClientApprovalDenied gain explicit Host/Server fixture arguments: NUnit drops a bare [TestFixture] when the class also has parameterized fixtures. NetworkObjectDeserializationFailure is ignored for hybrid prefabs: removing a prefab from one client's NetworkConfig does not unregister its ghost prefab, which all worlds in the process share.
The fixture was expanded into 36 [TestFixture]s (mode x client count x NetVar combination), each starting its own session for both of its two test cases. It now has one fixture per HostOrServer mode, and a single test that walks the NetVar combinations and spawn counts in arrays, adding a client between passes and despawning each case's objects before the next. Failure messages name the case. The 1 and 2 client passes now include a late-joined client, where every case previously ran against a freshly started session.
…n types The collection type only selects which helper component the prefab carries, so it no longer needs its own fixture. Both prefabs are registered up front and each test runs List then Dictionary in one session, despawning the instance from its current authority between them. Failure messages name the collection type. 10 fixtures become 5.
NoelStephensUnity
marked this pull request as ready for review
September 27, 2026 23:57
|
Check the configuration's TOML syntax, permitted settings, and size limit. See the error below. Error message: Configuration content:# This is the configuration for the u-pr bot
# https://github.cds.internal.unity3d.com/unity/u-pr
# For configuration of this file:
# https://developer.portal.internal.unity.com/catalog/default/component/u-pr/docs/configuration/automatic_runs/
[github_app]
handle_pr_actions = ['opened', 'ready_for_review'] # PR events that auto-run pr_commands
pr_commands = ["/harness-review"] # what runs on those events
reviewer_commands = ["/harness-review"] # what runs when @u-pr is added as reviewer
|
There was a problem hiding this comment.
💡 Harness Review
The change expands unified hybrid test coverage and consolidates two test suites; I examined fixture selection, hybrid startup, synchronization waits, and spawn and despawn behavior without finding an actionable issue.
Reviewed commit 5f4c87a
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop-3.x.x #4172 +/- ##
==============================================
Coverage 78.19% 78.19%
==============================================
Files 153 153
Lines 26272 26272
==============================================
Hits 20544 20544
Misses 5728 5728
Flags with carried forward coverage won't be shown. Click here to find out more.
|
EmandM
approved these changes
Sep 28, 2026
This branch has not been deployed
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
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
This PR gets 232 test cases, from 26 of the 117 NGO runtime SDK integration test fixtures (
Tests/Runtime), running in hybrid prefab mode (UnifiedHost/UnifiedServer).PR Scope:
This PR's focuses on covering NGO features that work with the unified API today (i.e. the connection sequence, RPCs, NetworkVariables, spawning and ownership).
Out of this PR's scope:
AttachableBehaviour(parenting consolidation is separate work), owner-authoritativeNetworkTransform, and theNetworkRigidbodytests.Test helper changes
UnifiedNetcodeTransportonly start when at least one hybrid prefab is registered.UnifiedHosttest case that only spawned players runs in "normal" NGO mode.CreateServerAndClientsnow registers aUnifiedSessionPrefabif a hybrid test case does not register at least one of its own.Tests included (hybrid fixtures +
UseUnifiedTests() => true):RpcTypeSerializationTests(132 cases),RpcTests,RpcDuringOnNetworkSpawnNetworkVariableGeneralTests,NetworkListTests,NetworkVariableCollectionsTests,NetworkVariableCollectionsChangingTests,NetworkVariableTraitsTests,NetworkVariablePermissionTests,NetworkVariableInheritanceTests,NetworkVarBufferCopyTest,OwnerModifiedTests,NetworkBehaviourUpdaterTestsConnectionApprovalTests,ConnectionApprovalTimeoutTests,ClientApprovalDenied,NetworkManagerPlayerPrefabNetworkObjectOnNetworkDespawnTests,NetworkObjectOwnershipTests,NetworkBehaviourPrePostSpawnTests,NetworkObjectSynchronizationTests(late join),PlayerSpawnObjectVisibilityTests,ComponentControllerTests,MessageReceiveAllocationTestsNetworkTransformAutoParenting(NGO parenting of hybrid objects)Adjustments needed
SpawnedObjectsimmediately after a spawn or a late join now useWaitForSpawnedOnAllOrTimeOut(NetworkVariablePermissionTests,NetworkVariableInheritanceTests,NetworkObjectSynchronizationTests).ComponentControllerTestspredicates were missing acontinueand threw instead of waiting.RpcTests,RpcTypeSerializationTestsandClientApprovalDeniednow have explicitHost/Serverfixture arguments. NUnit drops a bare[TestFixture]when the class also has parameterized fixtures. This renames their test IDs (e.g.RpcTests.TestRpcs→RpcTests(Host).TestRpcs).NetworkObjectDeserializationFailureis ignored for hybrid prefabs. Removing a prefab from one client'sNetworkConfigdoes not unregister its ghost prefab, because all worlds in the test process share that registration.Tests that were refactored:
Test-specific values that only configure a prefab now use arrays generated inside the test instead of expanded into
[TestFixture]s:NetworkBehaviourUpdaterTests: 36 fixtures → 5. Client count grows during the test, so the 1 and 2 client cases now include a late-joined client instead of a fresh session per case.NetworkVariableCollectionsChangingTests: 10 fixtures → 5. Each test runs List then Dictionary.NetworkBehaviourUpdaterTestsunifiedNetworkBehaviourUpdaterTestsnon-unifiedNetworkVariableCollectionsChangingTestsunifiedNetworkVariableCollectionsChangingTestsnon-unifiedIssues found and will require one (or more) follow-up PR(s)
NetworkManagershutdown callsWorld.DisposeAllWorlds(), so one client shutting down disposes the server's and every other client's world in the same process. Disposing only the NetworkManager's ownNetcodeWorld(local experiment, not in this PR) tookNetworkObjectDontDestroyWithOwnerTestsfrom 0/6 to 5/6 and theNetworkSpawnManagerTestsconnect/disconnect cases from 2/4 to 4/4.DisconnectTests,PeerDisconnectCallbackTestsClientDisconnectsFromServer). This still fails with (1) fixed.NetworkBehaviourReferenceTests,NetworkTransformOrderOfOperations,PlayerObjectTests).NetworkPrefabHandlerSynchronizationTests).NetworkSpawnManager.InstantiateNetworkPrefabdoes not select theNetworkManagerworld before instantiating a hybrid prefab. So, hybrid player prefabs fail when several worlds share a process. (This should be resolved withGhostObject.DelaySpawning/Spawn()as a separate PR).All tests impacted by the above issues were not included in this PR.
Jira ticket
MTT-16201
Documentation
Testing & QA (How your changes can be verified during release Playtest)
The unified result is from the Yamato unified job on this PR (6000.7.0b3, N4E 7.0.0). The non-unified results are headless PlayMode runs on 6000.7.0b1 (bundles N4E 7.0.0) with the unified job's scripting defines, filtered to the touched fixtures. Each result is from one run.
UNIFIED_TESTS=trueUNIFIED_TESTSunset, this branchUNIFIED_TESTSunset,develop-3.x.xtestprojectdocumentation test (DocumentationCodeSamples.CommandLineOptionsDocsTests). The 4 hybridNetworkObjectDeserializationFailurecases are ignored by design and counted as skipped.NetworkBehaviourUpdaterTests42 → 3,NetworkVariableCollectionsChangingTests12 → 6).Functional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testsDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?Up-port
Not needed. Hybrid prefab mode only exists on
develop-3.x.x.Backports
Not needed.