refactor: InternalMessageHandler is no longer static#706
Conversation
NoelStephensUnity
left a comment
There was a problem hiding this comment.
Other than instantiating the InternalMessageHandler within the Init method of the NetworkManager, it all looks good to me.
|
Beyond keeping NetworkingManager slimmer, is there a purpose for having InternalMessageHandler as its own class? Perhaps later @TwoTenPvP you were thinking one could swap in another message handler? Because it's a little odd...we have a sizable class with no state that's dependent heavily on another class that does have state...hence it's full of NetworkManager.singleton references. If this code was just folded into NetworkManager then those .singleton references would just vanish and I think it would be overall much easier to reason about. |
mattwalsh-unity
left a comment
There was a problem hiding this comment.
Seems to me this would be cleaner if we folded this into NetworkManager per my comment in the conversation tab.
|
|
||
| // Should cause log (server only) | ||
| LogAssert.Expect(LogType.Log, nameof(MessageHandlerReceivedMessageServerClient) + " " + nameof(DummyMessageHandler.HandleConnectionRequest)); | ||
| using (var messageStream = MessagePacker.WrapMessage(NetworkConstants.CONNECTION_REQUEST, inputBuffer)) |
There was a problem hiding this comment.
I'd encourage each one of these to be a separate test case. Throwing them all into one test means that if one fails, none of the following ones even get tested. Also makes it harder to maintain the test coverage.
|
|
||
| // Should cause log (server only) | ||
| LogAssert.Expect(LogType.Log, nameof(MessageHandlerReceivedMessageServerClient) + " " + nameof(DummyMessageHandler.HandleNetworkLog)); | ||
| using (var messageStream = MessagePacker.WrapMessage(NetworkConstants.SERVER_LOG, inputBuffer)) |
There was a problem hiding this comment.
This is another case where there are other conditionals we'd probably want to verify functionality for eventually (not necessarily in this PR)... if (IsServer && NetworkConfig.EnableNetworkLogs) provides at least 2 cases to consider testing.
|
|
||
| // Should cause log (server and client) | ||
| LogAssert.Expect(LogType.Log, nameof(MessageHandlerReceivedMessageServerClient) + " " + nameof(DummyMessageHandler.HandleNetworkVariableDelta)); | ||
| using (var messageStream = MessagePacker.WrapMessage(NetworkConstants.NETWORK_VARIABLE_DELTA, inputBuffer)) |
There was a problem hiding this comment.
You don't need to implement comprehensive coverage in this PR (we have other time scheduled for test debt), but consider how you validate some of the extra functionality that some of these methods have and what it might do to test case organization.
e.g. this one has a prebufferpreset - may be worth thinking about all the permutations of that which we'd want to verify functionality on. Would we want to somehow throw the permutations into this one monolith test case, or would separate test cases with separate setup stages be helpful?
| public class InternalMessageHandlerTests | ||
| { | ||
| [Test] | ||
| public void MessageHandlerReceivedMessageServerClient() |
There was a problem hiding this comment.
This is valuable and it's always nice to see test coverage added! I don't think you need to block the PR on this, but I'd encourage you to do some reading about AAA (arrange-act-assert) pattern testing and try to apply it going forward.
It takes some time to see what's being verified in this test, and how. Part of that is because it's so large (testing multiple things), but part of it is because the "assert" is happening inside of the "act".
com.unity.multiplayer.mlapi/Tests/Editor/InternalMessageHandlerTests.cs
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| internal class DummyMessageHandler : IInternalMessageHandler |
No description provided.