hotfix: Remote tool send remote message fix and add proper unit test (#17935)

Signed-off-by: guillaume-haerinck <[email protected]>

Gems/RemoteTools/Code/CMakeLists.txt

@@ -101,6 +101,7 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
                     Source
             BUILD_DEPENDENCIES
                 PRIVATE
+                    AZ::AzCoreTestCommon
                     AZ::AzTest
                     AZ::AzFramework
                     Gem::${gem_name}.Private.Static

Gems/RemoteTools/Code/Source/RemoteToolsSystemComponent.cpp

@@ -332,14 +332,11 @@ namespace RemoteTools
         AZStd::vector<char, AZ::OSStdAllocator> msgBuffer;
         AZ::IO::ByteContainerStream<AZStd::vector<char, AZ::OSStdAllocator>> outMsg(&msgBuffer);
 
-        if (!msgBuffer.empty())
+        AZ::ObjectStream* objStream = AZ::ObjectStream::Create(&outMsg, *serializeContext, AZ::ObjectStream::ST_BINARY);
+        objStream->WriteClass(&msg);
+        if (!objStream->Finalize())
         {
-            AZ::ObjectStream* objStream = AZ::ObjectStream::Create(&outMsg, *serializeContext, AZ::ObjectStream::ST_BINARY);
-            objStream->WriteClass(&msg);
-            if (!objStream->Finalize())
-            {
-                AZ_Assert(false, "ObjectStream failed to serialize outbound TmMsg!");
-            }
+            AZ_Assert(false, "ObjectStream failed to serialize outbound TmMsg!");
         }
 
         const size_t customBlobSize = msg.GetCustomBlobSize();
@@ -348,13 +345,6 @@ namespace RemoteTools
             outMsg.Write(customBlobSize, msg.GetCustomBlob().data());
         }
 
-        // If sent message is empty, add some dull data into it so that it can be deserialized anyway
-        if (msgBuffer.empty() && customBlobSize == 0)
-        {
-            AZStd::vector<AZStd::byte> dullBuffer{ 1 };
-            outMsg.Write(dullBuffer.size(), dullBuffer.data());
-        }
-
         // Messages targeted at our own application are also serialized/deserialized then moved onto the inbox
         const size_t totalSize = msgBuffer.size();
         if (target.IsSelf())
@@ -432,7 +422,6 @@ namespace RemoteTools
                 m_entryRegistry[key].m_availableTargets.insert_key(persistentId);
             AzFramework::RemoteToolsEndpointInfo& ti = ret.first->second;
             ti.SetInfo(packet.GetDisplayName(), persistentId, static_cast<uint32_t>(connection->GetConnectionId()));
-            m_entryRegistry[key].m_lastTarget = ti;
             m_entryRegistry[key].m_endpointJoinedEvent.Signal(ti);
         }
         return true;
@@ -468,7 +457,6 @@ namespace RemoteTools
 
                 AzFramework::RemoteToolsEndpointInfo& ti = ret.first->second;
                 ti.SetInfo("Host", packet.GetPersistentId(), static_cast<uint32_t>(connection->GetConnectionId()));
-                m_entryRegistry[key].m_lastTarget = ti;
                 m_entryRegistry[key].m_endpointJoinedEvent.Signal(ti);
             }
 
@@ -555,12 +543,6 @@ namespace RemoteTools
                     {
                         AzFramework::RemoteToolsEndpointInfo ti = endpointIt->second;
                         registryIt->second.m_endpointLeftEvent.Signal(ti);
-                        if (registryIt->second.m_lastTarget.GetPersistentId() == ti.GetPersistentId())
-                        {
-                            // Stored as unordered map, we can't know what was the target registered before this
-                            registryIt->second.m_lastTarget = AzFramework::RemoteToolsEndpointInfo();
-                        }
-
                         container.extract(endpointIt);
                         break;
                     }

Gems/RemoteTools/Code/Tests/RemoteToolsTest.cpp

@@ -11,10 +11,13 @@
 #include <AzCore/Console/LoggerSystemComponent.h>
 #include <AzCore/Interface/Interface.h>
 #include <AzCore/Name/NameDictionary.h>
+#include <AzCore/Serialization/SerializeContext.h>
 #include <AzCore/Time/TimeSystem.h>
+#include <AzCore/UnitTest/MockComponentApplication.h>
 #include <AzCore/UnitTest/TestTypes.h>
 #include <AzCore/UnitTest/UnitTest.h>
 #include <AzFramework/Network/IRemoteTools.h>
+#include <AzFramework/Script/ScriptDebugMsgReflection.h>
 #include <AzNetworking/Framework/NetworkingSystemComponent.h>
 
 #include <RemoteToolsSystemComponent.h>
@@ -36,12 +39,25 @@ namespace UnitTest
             m_timeSystem = AZStd::make_unique<AZ::TimeSystem>();
             m_networkingSystemComponent = AZStd::make_unique<AzNetworking::NetworkingSystemComponent>();
             m_remoteToolsSystemComponent = AZStd::make_unique<RemoteToolsSystemComponent>();
+            m_serializeContext = AZStd::make_unique<AZ::SerializeContext>();
             m_remoteTools = m_remoteToolsSystemComponent.get();
+            m_applicationMock = AZStd::make_unique<testing::NiceMock<UnitTest::MockComponentApplication>>();
+
+            ON_CALL(*m_applicationMock, GetSerializeContext())
+                .WillByDefault(
+                    [this]()
+                    {
+                        return m_serializeContext.get();
+                    });
+
+            AzFramework::ReflectScriptDebugClasses(m_serializeContext.get());
         }
 
         void TearDown() override
         {
+            m_applicationMock.reset();
             m_remoteTools = nullptr;
+            m_serializeContext.reset();
             m_remoteToolsSystemComponent.reset();
             m_networkingSystemComponent.reset();
             m_timeSystem.reset();
@@ -50,10 +66,12 @@ namespace UnitTest
             LeakDetectionFixture::SetUp();
         }
 
+        AZStd::unique_ptr<AZ::TimeSystem> m_timeSystem;
         AZStd::unique_ptr<AzNetworking::NetworkingSystemComponent> m_networkingSystemComponent;
         AZStd::unique_ptr<RemoteToolsSystemComponent> m_remoteToolsSystemComponent;
-        AZStd::unique_ptr<AZ::TimeSystem> m_timeSystem;
+        AZStd::unique_ptr<AZ::SerializeContext> m_serializeContext;
         AzFramework::IRemoteTools* m_remoteTools = nullptr;
+        AZStd::unique_ptr<testing::NiceMock<UnitTest::MockComponentApplication>> m_applicationMock;
     };
 
     TEST_F(RemoteToolsTests, TEST_RemoteToolsEmptyRegistry)
@@ -91,13 +109,21 @@ namespace UnitTest
         EXPECT_FALSE(m_remoteTools->IsEndpointOnline(TestToolsKey, TestToolsKey));
 
         {
-            AzFramework::RemoteToolsMessage msg;
+            AzFramework::ScriptDebugBreakpointRequest msg(1, "test", 2);
             msg.SetSenderTargetId(TestToolsKey);
             m_remoteTools->SendRemoteToolsMessage(endpointInfo, msg);
         }
+
         const AzFramework::ReceivedRemoteToolsMessages* receiveMsgs = m_remoteTools->GetReceivedMessages(TestToolsKey);
         EXPECT_NE(receiveMsgs, nullptr);
         EXPECT_EQ(receiveMsgs->size(), 1);
+
+        auto msg = azrtti_cast<AzFramework::ScriptDebugBreakpointRequest*>(receiveMsgs->at(0));
+        EXPECT_TRUE(msg != nullptr);
+        EXPECT_EQ(msg->m_request, 1);
+        EXPECT_STREQ(msg->m_context.c_str(), "test");
+        EXPECT_EQ(msg->m_line, 2);
+
         m_remoteTools->ClearReceivedMessages(TestToolsKey);
     }