test: manual connection approval to fully automated connection approval testing #839
Conversation
This is the first pass version with a duplicated MultiInstanceHelper. This includes SIPTransport updates that prevent issues with multiple SIPTransport unit tests as the S_Server was never getting reset. So, upon the host-server disconnecting it resets the value back to null so the next unit test can initialize the SIPTransport again. This includes a fix for unit tests that may result in the SnapshotSystem attempting to send before m_NetworkManager is set.
…/Unity-Technologies/com.unity.multiplayer.mlapi into test/manualRpcTests-to-Automated
…rt.cs Co-authored-by: Albin Corén <[email protected]>
…rt.cs Co-authored-by: Albin Corén <[email protected]>
Co-authored-by: M. Fatih MAR <[email protected]>
…rt.cs Co-authored-by: Albin Corén <[email protected]>
Co-authored-by: M. Fatih MAR <[email protected]>
| Buffer.BlockCopy(data.Array, data.Offset, copy, 0, data.Count); | ||
|
|
||
| m_Clients[clientId].IncomingBuffer.Enqueue(new Event | ||
| if (m_LocalConnection != null) |
There was a problem hiding this comment.
If a player is disconnected m_LocalConnection becomes null, but can still be called since the NetworkManager is not notified of the client disconnecting. This resolves this issue.
There was a problem hiding this comment.
this "skip if null" logic might simply make us blind to some issues happening in the future.
please log a warning on the else branch at least.
There was a problem hiding this comment.
@TwoTenPvP Please review the above changes.
SIPTransport.DisconnectLocalClient sets m_LocalConnection to null but never notifies the relative NetworkManager that the client has disconnected which can cause other systems (i.e. SnapShotSystem) to cause a barrage of exceptions when trying to send snapshots from a disconnected client.
There was a problem hiding this comment.
It is meant to notify all peers that it disconnected.
There was a problem hiding this comment.
I don't know why this would happen or what's going on behind the scenes but I still want to see a warning log printed so that a dev wouldn't be blind and execution wouldn't fail silently. do you guys think my request is still reasonable?
There was a problem hiding this comment.
Let me DM you on this... there are other things to consider here.
| public override NetworkEvent PollEvent(out ulong clientId, out NetworkChannel channel, out ArraySegment<byte> payload, out float receiveTime) | ||
| { | ||
| if (m_LocalConnection.IncomingBuffer.Count == 0) | ||
| if (m_LocalConnection != null) |
There was a problem hiding this comment.
If a player is disconnected m_LocalConnection becomes null, but can still be called since the NetworkManager is not notified of the client disconnecting. The resolution for this was to return back NetworkEvent.Nothing for the still running NetworkManager that is attempting to poll the SIPTransport for the client in question.
There was a problem hiding this comment.
similar to above, I'd warn dev here and even expect them to look at their call-stack & code-path when this happens.
please put something into the else branch.
There was a problem hiding this comment.
@TwoTenPvP Please review the above changes.
SIPTransport.DisconnectLocalClient sets m_LocalConnection to null but never notifies the relative NetworkManager that the client has disconnected which can cause other systems (i.e. SnapShotSystem) to cause a barrage of exceptions when trying to send snapshots from a disconnected client.
…tance-connection-approval
Did some final clean up on the code to prepare for review. Cleaned up/added comments. Added an Application.targetFrameRate check that will assure this test, for all platforms, runs at the same frame update rate.
…tance-connection-approval
…tance-connection-approval
…hub.com/Unity-Technologies/com.unity.multiplayer.mlapi into test/multi-instance-connection-approval
…player-object-get * develop: test: manual connection approval to fully automated connection approval testing (#839) test: manual rpc tests to automated fixed revision (#841) fix: CleanDiffedSceneObjects was not clearing PendingSoftSyncObjects (#834) feat: NetworkTransform now uses NetworkVariables instead of RPCs (#826) fix: gracefully handle exceptions in an RPC invoke (#846) fix: Fixing utp license file for legal (#843) ci: enable standards check on UTP too (#837) refactor: NetworkBehaviour.NetworkObject no longer throws an exception (#838) fix: revert #830 (#840) test: converting the manual rpc tests over to an automated unit test (#830) # Conflicts: # com.unity.multiplayer.mlapi/Tests/Runtime/MultiInstance/MultiInstanceHelpers.cs
This is the conversion of the manual connection approval test to an automated unit test using the MultiInstanceHelpers class.
This is dependent upon the changes in PR-841.
The reference Jira Ticket for this is MTT-778