-
Notifications
You must be signed in to change notification settings - Fork 461
fix: Don't let exceptions in OnNetworkSpawn/OnNetworkDespawn block processing of the next callback. [MTT-1378] #1739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
NoelStephensUnity
merged 17 commits into
develop
from
fix/catch_exceptions_in_OnNetworkSpawn
Aug 25, 2022
Merged
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
6b3e1ab
fix: Don't let exceptions in OnNetworkSpawn/OnNetworkDespawn block pr…
ShadauxCat d502aa1
Standards and changelog.
ShadauxCat ef344d2
Standards fixes
ShadauxCat 4749f46
It would be good to check in the right file.
ShadauxCat 23d4b15
I swear these tests passed before but I have no idea how.
ShadauxCat 012aa98
Address review feedback.
ShadauxCat 530e728
Switched to LogAssert.Expect
ShadauxCat 84b195a
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
ShadauxCat 032202b
Updated tests to NetcodeIntegrationTest style
ShadauxCat cede7c3
Standards fix
ShadauxCat 2ece99f
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
ShadauxCat 8a7c8b8
Rewrote tests according to feedback.
ShadauxCat a70c778
Test the behavior on both server and client
ShadauxCat 3753803
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
ShadauxCat 5017321
style
NoelStephensUnity 0fe6471
Update com.unity.netcode.gameobjects/CHANGELOG.md
ShadauxCat 2ba24c6
Merge branch 'develop' into fix/catch_exceptions_in_OnNetworkSpawn
NoelStephensUnity File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
122 changes: 122 additions & 0 deletions
122
testproject/Assets/Tests/Runtime/OnNetworkSpawnExceptionTests.cs
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| using System; | ||
| using System.Collections; | ||
| using System.Text.RegularExpressions; | ||
| using Unity.Netcode; | ||
| using Unity.Netcode.RuntimeTests; | ||
| using Unity.Netcode.TestHelpers.Runtime; | ||
| using UnityEngine; | ||
| using UnityEngine.Assertions; | ||
| using UnityEngine.TestTools; | ||
|
|
||
| namespace TestProject.RuntimeTests | ||
| { | ||
|
|
||
| public class OnNetworkSpawnThrowsExceptionComponent : NetworkBehaviour | ||
| { | ||
| public static int NumClientSpawns = 0; | ||
| public override void OnNetworkSpawn() | ||
| { | ||
| if (IsClient) | ||
| { | ||
| ++NumClientSpawns; | ||
| if (NumClientSpawns > 2) | ||
| { | ||
| throw new Exception("I'm misbehaving"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| public class OnNetworkDespawnThrowsExceptionComponent : NetworkBehaviour | ||
| { | ||
| public static int NumClientDespawns = 0; | ||
| public override void OnNetworkDespawn() | ||
| { | ||
| if (IsClient) | ||
| { | ||
| ++NumClientDespawns; | ||
| if (NumClientDespawns > 2) | ||
| { | ||
| throw new Exception("I'm misbehaving"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
| public class OnNetworkSpawnExceptionTests : NetcodeIntegrationTest | ||
| { | ||
| private GameObject m_Prefab; | ||
| private GameObject[] m_Objects = new GameObject[5]; | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator WhenOnNetworkSpawnThrowsException_FutureOnNetworkSpawnsAreNotPrevented() | ||
| { | ||
| //Spawning was done during setup | ||
| Assert.AreEqual(5, OnNetworkSpawnThrowsExceptionComponent.NumClientSpawns); | ||
| yield return null; | ||
| } | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator WhenOnNetworkDespawnThrowsException_FutureOnNetworkDespawnsAreNotPrevented() | ||
| { | ||
| for (var i = 0; i < 3; ++i) | ||
| { | ||
| LogAssert.Expect(LogType.Exception, new Regex("I'm misbehaving")); | ||
| } | ||
| //Spawning was done during setup. Now we despawn. | ||
| for (var i = 0; i < 5; ++i) | ||
| { | ||
| m_Objects[i].GetComponent<NetworkObject>().Despawn(); | ||
| } | ||
|
|
||
| var result = new TimeoutHelper(); | ||
| yield return WaitForConditionOrTimeOut( | ||
| () => OnNetworkDespawnThrowsExceptionComponent.NumClientDespawns == 5, result); | ||
| Assert.IsFalse(result.TimedOut); | ||
| } | ||
|
|
||
| protected override IEnumerator OnSetup() | ||
| { | ||
| m_UseHost = false; | ||
|
|
||
| for (var i = 0; i < 3; ++i) | ||
| { | ||
| LogAssert.Expect(LogType.Exception, new Regex("I'm misbehaving")); | ||
| } | ||
| OnNetworkSpawnThrowsExceptionComponent.NumClientSpawns = 0; | ||
| OnNetworkDespawnThrowsExceptionComponent.NumClientDespawns = 0; | ||
| yield return null; | ||
| } | ||
|
|
||
| protected override IEnumerator OnServerAndClientsConnected() | ||
| { | ||
| m_Prefab = new GameObject(); | ||
| var networkObject = m_Prefab.AddComponent<NetworkObject>(); | ||
| m_Prefab.AddComponent<OnNetworkSpawnThrowsExceptionComponent>(); | ||
| m_Prefab.AddComponent<OnNetworkDespawnThrowsExceptionComponent>(); | ||
| NetcodeIntegrationTestHelpers.MakeNetworkObjectTestPrefab(networkObject); | ||
|
|
||
| m_ServerNetworkManager.NetworkConfig.ForceSamePrefabs = false; | ||
| m_ServerNetworkManager.AddNetworkPrefab(m_Prefab); | ||
| foreach (var client in m_ClientNetworkManagers) | ||
| { | ||
| client.NetworkConfig.ForceSamePrefabs = false; | ||
| client.AddNetworkPrefab(m_Prefab); | ||
| } | ||
|
|
||
| for (var i = 0; i < 5; ++i) | ||
| { | ||
| var obj = UnityEngine.Object.Instantiate(m_Prefab); | ||
| m_Objects[i] = obj; | ||
| obj.GetComponent<NetworkObject>().NetworkManagerOwner = m_ServerNetworkManager; | ||
| obj.GetComponent<NetworkObject>().Spawn(); | ||
| } | ||
|
|
||
| var result = new TimeoutHelper(); | ||
| yield return WaitForConditionOrTimeOut( | ||
| () => OnNetworkSpawnThrowsExceptionComponent.NumClientSpawns == 5, result); | ||
| Assert.IsFalse(result.TimedOut); | ||
| } | ||
|
|
||
| protected override int NumberOfClients => 1; | ||
| } | ||
| } | ||
3 changes: 3 additions & 0 deletions
3
testproject/Assets/Tests/Runtime/OnNetworkSpawnExceptionTests.cs.meta
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests feel pretty rough in their current state.
if (IsClient)?)if (NumClientSpawns > 2)guard in the test components.While it's a very contentious topic, generally speaking, the more self-contained a test is, the better for understanding and lower the risk for future false positives. I'm not personally a fan of having to read 3 classes and 6 methods to understand what's actually happening in this 1 line test case.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While throwing an exception in a component added to the player prefab is valid, I think this test should add more NetworkBehaviour derived components because the real test is to validate that all NetworkBehaviour components on a GameObject get OnNetworkSpawn and OnNetworkDespawn invoked even if one of the components throws and exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've addressed everything you mentioned here, except for the use of multi-instance tests. I think it's valuable to test that the behavior is the same on both server and client. I did, however, update the test to make sure it isn't exclusively a client-side test, since OnNetworkSpawn and OnNetworkDespawn are called on the server too.