Isolate net messages in integration tests. (space-wizards#4838)

* Isolate net messages in integration tests.

Integration tests don't use Lidgren to connect client and send, instead they just use some in-process channels to communicate. Because of this, the original implementation of net messages *directly* passed the net message instances between client and server instances. This caused issues whenever content would mutate data in a NetMessage after it "passed through".

Now we run the messages through WriteToBuffer() and ReadFromBuffer() so they pass through binary serialization. This means there's no more implicit sharing of the objects.

Note that this requires some trickery: Lidgren's message types have internal constructors. Really ideally we'd change the engine to make this more testable... but that's a content breaking change. Instead I just added InternalsVisibleTo to Lidgren so we can mess with it. We maintain the library ourselves anyways I can do what I want.

Fixes space-wizards#4836

* Register Robust.UnitTesting as assembly for reflection.

This is necessary so that serialized types in the assembly can be picked up by NetSerializer.

Have to disable automatic reflection on all entity systems/components that tests register manually right now, because otherwise tests break.

* Stop shallow cloning specific net messages in integration tests.

This isn't necessary anymore now that we have a thorough fix.

* Wow I really forgot to copy-paste that line to the other side huh.

* Add test that serializer hash matches.

* Another test one I missed earlier.

* Changelog

Lidgren.Network/CursedHorrorsBeyondOurWildestImagination.cs

@@ -0,0 +1,9 @@
+using System.Runtime.CompilerServices;
+
+// So I wanted to mess with NetIncomingMessage and NetOutgoingMessage from tests.
+// Now.. the instructors are internal...
+// Unless...
+// I mean we have this project here from the weird way we're compiling Lidgren.
+// I could just put this in here... it wouldn't touch the main Lidgren repo at all...
+[assembly: InternalsVisibleTo("Robust.UnitTesting")]
+

Lidgren.Network/Lidgren.Network.csproj

@@ -16,6 +16,8 @@
   </PropertyGroup>
 
   <ItemGroup>
+    <Compile Include="CursedHorrorsBeyondOurWildestImagination.cs" />
+
     <Compile Include="Lidgren.Network\Lidgren.Network\**\*.cs">
      <Link>%(RecursiveDir)%(Filename)%(Extension)</Link>
     </Compile>

RELEASE-NOTES.md

@@ -47,7 +47,7 @@ END TEMPLATE-->
 
 ### Other
 
-*None yet*
+* Integration tests now run `NetMessage`s through serialization rather than passing the objects between client and server. This causes tests that missed `[NetSerializer]` attributes on any objects that need them to fail.
 
 ### Internal
 

Robust.Shared/Serialization/RobustSerializer.cs

@@ -100,6 +100,11 @@ public void Initialize()
         public byte[] GetSerializableTypesHash() => Convert.FromHexString(_serializer.GetSHA256());
         public string GetSerializableTypesHashString() => _serializer.GetSHA256();
 
+        internal void GetHashManifest(Stream stream, bool writeNewline=false)
+        {
+            _serializer.GetHashManifest(stream, writeNewline);
+        }
+
         public (byte[] Hash, byte[] Package) GetStringSerializerPackage() => MappedStringSerializer.GeneratePackage();
 
         public Dictionary<Type, uint> GetTypeMap() => _serializer.GetTypeMap();

Robust.UnitTesting/RobustIntegrationTest.NetManager.cs

@@ -1,14 +1,17 @@
 using System;
+using System.Buffers;
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Linq;
 using System.Net;
 using System.Threading.Channels;
 using System.Threading.Tasks;
+using Lidgren.Network;
 using Robust.Shared.Asynchronous;
 using Robust.Shared.IoC;
 using Robust.Shared.Network;
 using Robust.Shared.Network.Messages;
+using Robust.Shared.Serialization;
 using Robust.Shared.Timing;
 using Robust.Shared.Utility;
 
@@ -20,6 +23,8 @@ internal sealed class IntegrationNetManager : IClientNetManager, IServerNetManag
         {
             [Dependency] private readonly IGameTiming _gameTiming = default!;
             [Dependency] private readonly ITaskManager _taskManager = default!;
+            [Dependency] private readonly IRobustSerializer _robustSerializer = default!;
+
             public bool IsServer { get; private set; }
             public bool IsClient => !IsServer;
             public bool IsRunning { get; private set; }
@@ -53,6 +58,10 @@ internal sealed class IntegrationNetManager : IClientNetManager, IServerNetManag
             /// </summary>
             public ChannelWriter<object>? NextConnectChannel { get; set; }
 
+            // Used for faking NetMessage.ReadFromBuffer and WriteToBuffer.
+            private readonly NetOutgoingMessage _serializationMessage = new();
+            private readonly NetIncomingMessage _deserializationMessage = new();
+
             private int _genConnectionUid()
             {
                 return ++_connectionUidTracker;
@@ -171,7 +180,7 @@ async Task DoConnect()
                                 channel = ServerChannel;
                             }
 
-                            var message = data.Message;
+                            var message = DeserializeNetMessage(data);
                             message.MsgChannel = channel;
                             if (_callbacks.TryGetValue(message.GetType(), out var callback))
                             {
@@ -249,21 +258,8 @@ public void ServerSendMessage(NetMessage message, INetChannel recipient)
             {
                 DebugTools.Assert(IsServer);
 
-                if (message is MsgState stateMsg)
-                {
-                    // MsgState sending method depends on the size of the possible compressed buffer. But tests bypass buffer read/write.
-                    stateMsg._hasWritten = true;
-
-                    // Need to duplicate the state to avoid the server & client storing references to the same collections.
-                    stateMsg.State = stateMsg.State.Clone();
-                }
-                else if (message is MsgStateLeavePvs leaveMsg)
-                {
-                    leaveMsg.Entities = leaveMsg.Entities.ShallowClone();
-                }
-
                 var channel = (IntegrationNetChannel) recipient;
-                channel.OtherChannel.TryWrite(new DataMessage(message, channel.RemoteUid));
+                channel.OtherChannel.TryWrite(SerializeNetMessage(message, channel.RemoteUid));
             }
 
             public void ServerSendToMany(NetMessage message, List<INetChannel> recipients)
@@ -380,7 +376,7 @@ public void ClientSendMessage(NetMessage message)
                     throw new InvalidOperationException("Not connected.");
                 }
 
-                channel.OtherChannel.TryWrite(new DataMessage(message, channel.RemoteUid));
+                channel.OtherChannel.TryWrite(SerializeNetMessage(message, channel.RemoteUid));
             }
 
             public void DispatchLocalNetMessage(NetMessage message)
@@ -391,6 +387,42 @@ public void DispatchLocalNetMessage(NetMessage message)
                 }
             }
 
+            private DataMessage SerializeNetMessage(NetMessage netMessage, int remoteUid)
+            {
+                byte[] pooledBuffer;
+                int length;
+                lock (_serializationMessage)
+                {
+                    netMessage.WriteToBuffer(_serializationMessage, _robustSerializer);
+                    length = _serializationMessage.LengthBytes;
+                    pooledBuffer = ArrayPool<byte>.Shared.Rent(length);
+                    _serializationMessage.Data.AsSpan(0, length).CopyTo(pooledBuffer);
+                    _serializationMessage.Position = 0;
+                    _serializationMessage.LengthBytes = 0;
+                }
+
+                return new DataMessage(pooledBuffer, length, netMessage.GetType(), remoteUid);
+            }
+
+            private NetMessage DeserializeNetMessage(DataMessage message)
+            {
+                var buffer = message.PooledNetBuffer;
+                var netMessage = (NetMessage) Activator.CreateInstance(message.MessageType)!;
+                lock (_deserializationMessage)
+                {
+                    _deserializationMessage.m_data = buffer;
+                    _deserializationMessage.LengthBytes = message.Length;
+                    _deserializationMessage.Position = 0;
+
+                    netMessage.ReadFromBuffer(_deserializationMessage, _robustSerializer);
+
+                    _deserializationMessage.m_data = null;
+                }
+
+                ArrayPool<byte>.Shared.Return(buffer);
+                return netMessage;
+            }
+
             private sealed class IntegrationNetChannel : INetChannel
             {
                 private readonly IntegrationNetManager _owner;
@@ -486,17 +518,7 @@ private sealed class DeniedConnectMessage
             {
             }
 
-            private sealed class DataMessage
-            {
-                public DataMessage(NetMessage message, int connection)
-                {
-                    Message = message;
-                    Connection = connection;
-                }
-
-                public NetMessage Message { get; }
-                public int Connection { get; }
-            }
+            private sealed record DataMessage(byte[] PooledNetBuffer, int Length, Type MessageType, int Connection);
 
             private sealed class DisconnectMessage
             {

Robust.UnitTesting/RobustIntegrationTest.cs

@@ -34,6 +34,7 @@
 using Robust.Shared.Network;
 using Robust.Shared.Player;
 using Robust.Shared.Prototypes;
+using Robust.Shared.Reflection;
 using Robust.Shared.Serialization;
 using Robust.Shared.Timing;
 using ServerProgram = Robust.Server.Program;
@@ -679,6 +680,7 @@ private BaseServer Init()
                 deps.BuildGraph();
                 //ServerProgram.SetupLogging();
                 ServerProgram.InitReflectionManager(deps);
+                deps.Resolve<IReflectionManager>().LoadAssemblies(typeof(RobustIntegrationTest).Assembly);
 
                 var server = DependencyCollection.Resolve<BaseServer>();
 
@@ -860,6 +862,7 @@ private GameController Init()
                 deps.BuildGraph();
 
                 GameController.RegisterReflection(deps);
+                deps.Resolve<IReflectionManager>().LoadAssemblies(typeof(RobustIntegrationTest).Assembly);
 
                 var client = DependencyCollection.Resolve<GameController>();
 

Robust.UnitTesting/Server/GameObjects/ComponentMapInitTest.cs

@@ -2,6 +2,7 @@
 using NUnit.Framework;
 using Robust.Shared.GameObjects;
 using Robust.Shared.Map;
+using Robust.Shared.Reflection;
 
 namespace Robust.UnitTesting.Server.GameObjects;
 
@@ -38,6 +39,7 @@ public void ComponentMapInit()
         mapManager.DeleteMap(mapId);
     }
 
+    [Reflect(false)]
     private sealed class MapInitTestSystem : EntitySystem
     {
         public override void Initialize()
@@ -52,6 +54,7 @@ private void OnMapInitTestMapInit(EntityUid uid, MapInitTestComponent component,
         }
     }
 
+    [Reflect(false)]
     private sealed partial class MapInitTestComponent : Component
     {
         public int Count = 0;

Robust.UnitTesting/Shared/GameObjects/DeferredEntityDeletionTest.cs

@@ -3,6 +3,7 @@
 using Robust.Shared.GameObjects;
 using Robust.Shared.IoC;
 using Robust.Shared.Map;
+using Robust.Shared.Reflection;
 
 namespace Robust.UnitTesting.Shared.GameObjects;
 
@@ -89,6 +90,7 @@ await server.WaitAssertion(() =>
         await server.WaitIdleAsync();
     }
 
+    [Reflect(false)]
     private sealed class DeferredDeletionTestSystem : EntitySystem
     {
         public override void Initialize()
@@ -104,6 +106,7 @@ private void OnTestEvent(EntityUid uid, DeferredDeletionTestComponent component,
         }
     }
 
+    [Reflect(false)]
     private sealed class OtherDeferredDeletionTestSystem : EntitySystem
     {
         public override void Initialize() => SubscribeLocalEvent<OtherDeferredDeletionTestComponent, DeferredDeletionTestEvent>(OnTestEvent);
@@ -117,10 +120,12 @@ private void OnTestEvent(EntityUid uid, OtherDeferredDeletionTestComponent compo
     }
 
     [RegisterComponent]
+    [Reflect(false)]
     private sealed partial class DeferredDeletionTestComponent : Component
     {
     }
 
+    [Reflect(false)]
     private sealed partial class OtherDeferredDeletionTestComponent : Component
     {
     }

Robust.UnitTesting/Shared/GameObjects/EntityEventBusTests.OrderedEvents.cs

@@ -2,6 +2,7 @@
 using NUnit.Framework;
 using Robust.Shared.GameObjects;
 using Robust.Shared.Map;
+using Robust.Shared.Reflection;
 using Robust.UnitTesting.Server;
 
 namespace Robust.UnitTesting.Shared.GameObjects;
@@ -41,6 +42,7 @@ public void TestDifferentComponentsOrderedSameKeySub()
         Assert.That(foo.EventOrder, Is.EquivalentTo(new[]{"Foo", "Transform", "Metadata"}).Or.EquivalentTo(new[]{"Foo", "Metadata", "Transform"}));
     }
 
+    [Reflect(false)]
     private sealed class DifferentComponentsSameKeySubSystem : EntitySystem
     {
         public override void Initialize()
@@ -50,6 +52,7 @@ public override void Initialize()
         }
     }
 
+    [Reflect(false)]
     private sealed class DifferentComponentsSameKeySubSystem2 : EntitySystem
     {
         public override void Initialize()
@@ -60,7 +63,7 @@ public override void Initialize()
         }
     }
 
-
+    [Reflect(false)]
     private sealed partial class FooComponent : Component
     {
 

Robust.UnitTesting/Shared/GameObjects/EntitySystemManagerOrderTest.cs

@@ -27,6 +27,7 @@ private sealed class Counter
             public int X;
         }
 
+        [Reflect(false)]
         private abstract class TestSystemBase : IEntitySystem
         {
             public Counter? Counter;
@@ -47,21 +48,25 @@ public void FrameUpdate(float frameTime) { }
 
         // Expected update order is is A -> D -> C -> B
 
+        [Reflect(false)]
         private sealed class TestSystemA : TestSystemBase
         {
 
         }
 
+        [Reflect(false)]
         private sealed class TestSystemB : TestSystemBase
         {
             public override IEnumerable<Type> UpdatesAfter => new[] {typeof(TestSystemA)};
         }
 
+        [Reflect(false)]
         private sealed class TestSystemC : TestSystemBase
         {
             public override IEnumerable<Type> UpdatesBefore => new[] {typeof(TestSystemB)};
         }
 
+        [Reflect(false)]
         private sealed class TestSystemD : TestSystemBase
         {
             public override IEnumerable<Type> UpdatesAfter => new[] {typeof(TestSystemA)};

Robust.UnitTesting/Shared/GameObjects/Systems/AnchoredSystemTests.cs

@@ -9,6 +9,7 @@
 using Robust.Shared.Physics;
 using Robust.Shared.Physics.Components;
 using Robust.Shared.Physics.Systems;
+using Robust.Shared.Reflection;
 using Robust.UnitTesting.Server;
 
 // ReSharper disable AccessToStaticMemberViaDerivedType
@@ -88,8 +89,10 @@ public void OnAnchored_WorldPosition_TileCenter()
         }
 
         [ComponentProtoName("AnchorOnInit")]
+        [Reflect(false)]
         private sealed partial class AnchorOnInitComponent : Component { };
 
+        [Reflect(false)]
         private sealed class AnchorOnInitTestSystem : EntitySystem
         {
             public override void Initialize()
@@ -99,6 +102,7 @@ public override void Initialize()
             }
         }
 
+        [Reflect(false)]
         internal sealed class MoveEventTestSystem : EntitySystem
         {
             [Dependency] private readonly SharedTransformSystem _transform = default!;