Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen

- Fixed NetworkManager to cleanup connected client lists after stopping (#1945)
- Fixed: NetworkHide followed by NetworkShow on the same frame works correctly (#1940)
- Fixed throwing an exception in OnNetworkSpawn/OnNetworkDespawn causing other OnNetworkSpawn/OnNetworkDespawn calls to not be executed. (#1739)


## [1.0.0-pre.8] - 2022-04-27

Expand Down
18 changes: 16 additions & 2 deletions com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs
Original file line number Diff line number Diff line change
Expand Up @@ -435,14 +435,28 @@ internal void InternalOnNetworkSpawn()
IsSpawned = true;
InitializeVariables();
UpdateNetworkProperties();
OnNetworkSpawn();
try
{
OnNetworkSpawn();
}
catch (Exception e)
{
Debug.LogException(e);
}
}

internal void InternalOnNetworkDespawn()
{
IsSpawned = false;
UpdateNetworkProperties();
OnNetworkDespawn();
try
{
OnNetworkDespawn();
}
catch (Exception e)
{
Debug.LogException(e);
}
}

/// <summary>
Expand Down
122 changes: 122 additions & 0 deletions testproject/Assets/Tests/Runtime/OnNetworkSpawnExceptionTests.cs
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()
Copy link
Copy Markdown
Contributor

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.

  • There are lots of magic numbers. There are even magic numbers that depend on other magic numbers (2, 3, and 5)
  • You have 1 setup for 2 different tests. Is all the code in the setup necessary for both?
  • You have log asserts in your test setup. Are these asserts actual tests, or are they an attempt to verify that setup has done what it's expected to?
  • You are using the multi-instance tests and have a client but it's unclear if this is actually necessary for this test (don't these virtual methods get invoked on the server too? Can't you write a much simpler/more contained UnityTest that doesn't involve a client or waiting for frames with timeout asserts and if (IsClient)?)
  • It's unclear (to me) why you have 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.

Copy link
Copy Markdown
Member

@NoelStephensUnity NoelStephensUnity May 23, 2022

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.

Copy link
Copy Markdown
Collaborator Author

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.

{
//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;
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.