refactor!: Unified Shutdown #1108
Conversation
BREAKING CHANGE: NetworkManager.StopX is replaced with NetworkManager.Shutdown
| { | ||
| var disconnectedIds = new HashSet<ulong>(); | ||
|
|
||
| //Don't know if I have to disconnect the clients. I'm assuming the NetworkTransport does all the cleaning on shtudown. But this way the clients get a disconnect message from server (so long it does't get lost) |
There was a problem hiding this comment.
some grammar mistakes there and also in the summary section.
++ I'd like to get @andrews-unity quick thoughts about this PR :)
There was a problem hiding this comment.
(also the comment should not exist and merged in, probably)
There was a problem hiding this comment.
I moved what was in the old Stop methods, I will clean it up a bit.
There was a problem hiding this comment.
I think the comment should continue to exist. Because it's a quirk of the current transport implementation. Perhaps rephrased.
There was a problem hiding this comment.
So unfortunately given the way StartHost/Client/Server is setup it calls Initialize which assumes that Shutdown is called which means that we have code that assumes that this PR is doing what its doing.
So for me I don't think we have much choice with the current way things are to not have this PR without potentially having errors thrown etc..
| public class StartStopTests | ||
| { | ||
| [Test] | ||
| public void TestStopAndRestartForExceptions() |
There was a problem hiding this comment.
You're testing between 2 and (arguably) 12 things in this test.
- Are you trying to test IsServer, IsClient, IsHost are set properly for StartServer? If so, it should be its own test (asserts)
- Are you trying to test IsServer, IsClient, IsHost are set to false after Shutdown? If so, it should be its own test (asserts)
- Are you trying to test the flags are set properly on restart? If so, its own test (asserts)
- Are you trying to test multiple shutdowns don't throw an exception (as the test is named)? If so, should be its own test (asserts)
- Are you trying to ensure other values are properly set/cleared during a shutdown? Separate test... etc.
There was a problem hiding this comment.
Also, for this type of code, consider using a multi assert pattern (though be careful not to abuse the purpose of this)
https://docs.nunit.org/articles/nunit/writing-tests/assertions/multiple-asserts.html
There was a problem hiding this comment.
Thanks, will update!
| } | ||
|
|
||
| [Test] | ||
| public void TestStopAndRestartForExceptions() |
There was a problem hiding this comment.
These look much better, good job!
nit: The team just had a conversation about this, and still isn't aligned quite yet so feel free to disregard, but I'm a fan of more descriptive test names... e.g. either SUTShouldDoExpectedThingWhenSpecificState or SUT_SpecificState_ExpectedResult so in this case it might be something like NetworkManagerShouldNotThrowExceptionsWhenRestarted, or NetworkManager_StartAfterShutdown_DoesNotThrow
…hub.com:Unity-Technologies/com.unity.multiplayer.mlapi into sam/feature/interpolation-for-network-transform * 'sam/feature/interpolation-for-network-transform' of github.com:Unity-Technologies/com.unity.multiplayer.mlapi: fix: networkscenemanager not releasing buffers from pool (#1132) test: fixed-length strings in netvars (#1119) fix: snapshot system. last fixes for release (#1129) refactor!: Unified Shutdown (#1108) chore: Fill out unity project for integration test project (#1128) feat: make ServerRpc ownership check an error log instead of warning log (#1126)
ghost
mentioned this pull request
BREAKING CHANGE: NetworkManager.StopX is replaced with NetworkManager.Shutdown
BREAKING CHANGE: NetworkManager.StopX is replaced with NetworkManager.Shutdown