test: add NetworkObject and NetworkBehaviour EditorTests #607
Conversation
TwoTenPvP
force-pushed
the
tests/network-object-behaviour-tests
branch
from
2b0dce8 to
edf1696
Compare
TwoTenPvP
changed the title
|
Expect this to fail until release/0.1.0 has been merged into develop. |
|
we recently merged |
| var networkObject = gameObject.AddComponent<NetworkObject>(); | ||
|
|
||
| // TODO: Maybe not hardcode message? | ||
| LogAssert.Expect(LogType.Error, $"[MLAPI] Behaviour index was out of bounds. Did you mess up the order of your {nameof(NetworkBehaviour)}s?"); |
There was a problem hiding this comment.
Like you, I'm not crazy about this relying on the error message in NetworkObject.cs lines up exactly with this hard-coded string. Since we already check for null below, is this just making sure we log an error message?
There was a problem hiding this comment.
This is actually not a check, but it's just saying "dont fail the test just because this error message comes through".
By default the test runner will fail if a error log is sent.
There was a problem hiding this comment.
does the test fail due to logs here without this trick currently?
There was a problem hiding this comment.
@TwoTenPvP now that I know the truth, I feel no better :)
| var networkBehaviour = gameObject.AddComponent<EmptyNetworkBehaviour>(); | ||
|
|
||
| // TODO: Maybe not hardcode message? | ||
| LogAssert.Expect(LogType.Error, $"[MLAPI] Behaviour index was out of bounds. Did you mess up the order of your {nameof(NetworkBehaviour)}s?"); |
com.unity.multiplayer.mlapi/Tests/Editor/NetworkBehaviourTests.cs
Outdated
Show resolved
Hide resolved
|
|
||
| Object.DestroyImmediate(networkObject); | ||
|
|
||
| Assert.That(networkBehaviour.HasNetworkObject, Is.False); |
There was a problem hiding this comment.
wouldn't NetworkBehaviour instance also be destroyed as part of NetworkObject/GameObject we created above after we DestroyImmediate()?
There was a problem hiding this comment.
No? We just remove the NetworkObject component.
There was a problem hiding this comment.
oh I see, then we should also destroy the main GameObject either within this test method or on [TearDown] :)
com.unity.multiplayer.mlapi/Tests/Editor/NetworkBehaviourTests.cs
Outdated
Show resolved
Hide resolved
| // TODO: Do we really want to throw here? | ||
| // Future API change: return null | ||
| Assert.Throws<NullReferenceException>(() => | ||
| { | ||
| var x = networkBehaviour.NetworkObject; | ||
| }); |
There was a problem hiding this comment.
yeah, we should probably not throw but return null there instead but obviously not in this PR
There was a problem hiding this comment.
I created a quick JIRA ticket so as to not rely on our tribal knowledge & likelihood we'll scan for TODOs someday
| // TODO: Do we really want to throw here? | ||
| // Future API change: return null | ||
| Assert.Throws<NullReferenceException>(() => | ||
| { | ||
| var x = networkBehaviour.NetworkObject; | ||
| }); |
There was a problem hiding this comment.
similar to above, I wonder how safe it is to operate on a NetworkBehaviour instance after DestroyImmediate()ing the main GameObject that holds it...
There was a problem hiding this comment.
We don't destroy the GameObject, only the NetworkObject component.
| [Test] | ||
| public void GetBehaviourIndexNone() | ||
| { | ||
| var gameObject = new GameObject(nameof(GetBehaviourIndexNone)); |
There was a problem hiding this comment.
I believe we should destroy this gameObject since its scope looks super local and specific to this test.
| [Test] | ||
| public void GetBehaviourIndexOne() | ||
| { | ||
| var gameObject = new GameObject(nameof(GetBehaviourIndexOne)); |
There was a problem hiding this comment.
ditto my gameobject destruction comment above.
0xFA11
changed the title
There was a problem hiding this comment.
I'm mainly concerned about the GameObject lifecycle in these test method implementations. We should probably cleanup after tests, either within the test method with DestroyImmediate() or on [TearDown].
Also please summarize the PR in the PR description next time so that it'd guide reviewers to quickly get into the context.
The test runner automatically cleans all of this up! |
my point is, cleaning-up would prevent any unexpected side-effects caused by objects left around until the editor cleans everything up for us. |
|
Tests now cleans up itself |
No description provided.