test: converting the manual rpc tests over to an automated unit test #830

NoelStephensUnity · 2021-05-13T22:03:49Z

This work is based off of MTT-764.
It includes the conversion of the manual RpcTests over to an automated unit test with 1 Host and 7 Clients (8 clients total).
Summary
This task is to convert the manual RpcTesting tests over to an automated unit test leveraging from the new MultiInstanceHelpers class. This includes the refactoring of the RpcQueueManualTests component to be able to handle both Manual and Automated testing.

Acceptance Criteria
The unit test should perform at least one full cycle of the various RPC tests performed by the manual test using 1 Host and 7 clients for a total of 1 Server and 8 clients. The RPC tests performed can be verified by the final output of the unit test to assure all runtime RPC related counters reflect that each type of test has been counted for each client and server multiple times (it varies depending upon the test type). The only value that will not be conveyed is the progress for each multi-client striped test. That is verified by the fact that the test completed successfully.

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.

fixing the spaces.

Removing the temporary MultiInstanceHelpers class and migrating the changes over to the original MultiInstanceHelpers class. Renamed the UnityTest to ManualRpcTestsAutomated.

Including the assembly definition file... :)

Increasing the total number of clients to 10. 1 Host and 9 clients.

Updating comment to reflect the proper number of clients (changed from 7 and 1 host to 9 and 1 host)

…/Unity-Technologies/com.unity.multiplayer.mlapi into test/manualRpcTests-to-Automated

Adding the ability to run this same test under different conditions. This pass provides the ability to specify the number of clients (always will be a host) and whether it will be using batched RPCs or not.

Creating a HybridScripts folder to distinguish between manual tests that have yet to be automated and manual tests that can be run in both a unit test and as a manual test.

com.unity.multiplayer.mlapi/Runtime/Core/SnapshotSystem.cs

com.unity.multiplayer.mlapi/Tests/Runtime/MultiInstance/MultiInstanceHelpers.cs

com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs

…rt.cs

com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs

…rt.cs

com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs

…rt.cs

testproject/Assets/Tests/Runtime/RpcTestsAutomated.cs

0xFA11 · 2021-05-14T09:43:31Z

com.unity.multiplayer.mlapi/Tests/Runtime/MultiInstance/MultiInstanceHelpers.cs

@@ -62,6 +68,25 @@ public static bool Create(int clientCount, out NetworkManager server, out Networ
            return true;
        }

+
+        public static void ShutdownAndClean()


minor: I'd name this as Destroy to keep it similar to Create/Start formats.

Suggested change

public static void ShutdownAndClean()

public static void Destroy()

I really don't have a preference to what we name this...
@TwoTenPvP What do you want this to be named?

0xFA11 · 2021-05-14T09:49:38Z

testproject/Assets/Tests/Runtime/testproject.runtimetests.asmdef

+        "Unity.Multiplayer.MLAPI.Runtime",
+        "UnityEngine.TestRunner",
+        "UnityEditor.TestRunner",
+        "Unity.Multiplayer.MLAPI.RuntimeTests",
+        "TestProject.ManualTests"


changes could be limited to only 2 added lines and the rest of the diff should not be here (might require manual text edit to prevent Unity's auto gen):

Suggested change

"Unity.Multiplayer.MLAPI.Runtime",

"UnityEngine.TestRunner",

"UnityEditor.TestRunner",

"Unity.Multiplayer.MLAPI.RuntimeTests",

"TestProject.ManualTests"

"Unity.Multiplayer.MLAPI.Runtime",

"Unity.Multiplayer.MLAPI.RuntimeTests",

"TestProject.ManualTests"

also, Testproject.RuntimeTests depending on TestProject.ManualTest + MLAPI.RuntimeTests doesn't sound right to me.
we started to go a little bit crazy in dependencies I think :D
is this because we're not organizing code better or are these dependencies inevitable?

Did you test this change before you suggested it?

here you go:

{ "name": "TestProject.RuntimeTests", "references": [ "Unity.Multiplayer.MLAPI.Runtime", "Unity.Multiplayer.MLAPI.RuntimeTests", "TestProject.ManualTests" ], "optionalUnityReferences": [ "TestAssemblies" ] }

Nice suggestion...

"optionalUnityReferences": [ "TestAssemblies" ]

Assures the test assemblies will be loaded:
"UnityEngine.TestRunner",
"UnityEditor.TestRunner",

testproject/Assets/Tests/Manual/HybridScripts/RpcQueueManualTests.cs

0xFA11 · 2021-05-14T09:54:23Z

testproject/Assets/Tests/Manual/HybridScripts/RpcQueueManualTests.cs

@@ -284,80 +379,101 @@ private void OnClientConnectedCallback(ulong clientId)
        /// </summary>
        private void Update()
        {
-            if (NetworkManager.Singleton && NetworkManager.Singleton.IsListening)
+            if (NetworkManager != null && NetworkManager.IsListening && ((IsServer && NetworkManager.ConnectedClientsList.Count > 1) || IsClient) && m_ContinueToRun && m_BeginTest)


0xFA11 · 2021-05-14T09:57:31Z

testproject/Assets/Tests/Manual/HybridScripts/RpcQueueManualTests.cs

-                        m_ClientProgressBar.fillAmount = Mathf.Clamp((2.0f * (float)m_GlobalDirectCounter) * k_ProgressBarDivisor, 0.01f, 1.0f);
+                        if (m_ClientProgressBar && m_ClientIndices.Contains(NetworkManager.LocalClientId))
+                        {
+                            m_ClientProgressBar.fillAmount = Mathf.Clamp((2.0f * (float)m_GlobalDirectCounter) * k_ProgressBarDivisor, 0.01f, 1.0f);


an innocent operation like this takes a long way in branches to be executed :)
I feel like there could be a logic compression somehow but maybe all those checks are really necessary.

Please take note of the !UnitTesting on line 778

This was a manual test that is now a hybrid:

It can operate in manual mode

It can operate in unit test mode

It will be fine. :)

why go hybrid?

It saves fuel of course!

in the scope of this PR, I'd appreciate that but this reminds me that we need to have a "dependencies and isolation" conversation soon, internally.

In all reality, it was made hybrid so we can perform a visual acceptance test on the acceptance tests. This is why the end of the unit test it logs the count totals for everything, so one can verify that indeed the counters are all being incremented in the same fashion running as a unit test as it does when ran "manually".

if the test was to be fully-automated, it wouldn't need manual testing or visual testing in the first place :)

Well perhaps the task was not defined properly... it was to convert the manual tests over to unit tests.
(that leaves a lot of room for interpretation)

com.unity.multiplayer.mlapi/Tests/Runtime/Transport/SIPTransport.cs

…rt.cs Co-authored-by: Albin Corén <2108U9@gmail.com>

Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

…rt.cs Co-authored-by: Albin Corén <2108U9@gmail.com>

Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

Reverting change to snapshot system.

Condensed assembly definition per suggestion by Fatih.

0xFA11 · 2021-05-18T12:05:36Z

apparently, this PR breaks Ubuntu tests on develop @NoelStephensUnity

…it test (#830)" This reverts commit 31b41ae.

* Revert "test: converting the manual rpc tests over to an automated unit test (#830)" This reverts commit 31b41ae. * make semantic commits check happy

* develop: 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)

…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

NoelStephensUnity added 4 commits May 13, 2021 16:44

style

b785c56

fixing the spaces.

Merge branch 'develop' into test/manualRpcTests-to-Automated

c9f5400

refactor

7d70eaf

Removing the temporary MultiInstanceHelpers class and migrating the changes over to the original MultiInstanceHelpers class. Renamed the UnityTest to ManualRpcTestsAutomated.

NoelStephensUnity requested review from TwoTenPvP, jeffreyrainy and 0xFA11 May 13, 2021 22:03

NoelStephensUnity and others added 3 commits May 13, 2021 17:09

fix

26b26b1

Including the assembly definition file... :)

refactor

2ae5e44

Increasing the total number of clients to 10. 1 Host and 9 clients.

Merge branch 'develop' into test/manualRpcTests-to-Automated

838c881

NoelStephensUnity marked this pull request as ready for review May 13, 2021 23:36

NoelStephensUnity added 4 commits May 13, 2021 18:48

style

85983c3

Updating comment to reflect the proper number of clients (changed from 7 and 1 host to 9 and 1 host)

Merge branch 'test/manualRpcTests-to-Automated' of https://github.com…

3ec4074

…/Unity-Technologies/com.unity.multiplayer.mlapi into test/manualRpcTests-to-Automated

refactor

b369dff

Adding the ability to run this same test under different conditions. This pass provides the ability to specify the number of clients (always will be a host) and whether it will be using batched RPCs or not.

refactor

67e8aea

Creating a HybridScripts folder to distinguish between manual tests that have yet to be automated and manual tests that can be run in both a unit test and as a manual test.