From af9893317364662d90447f28651781fd489643e9 Mon Sep 17 00:00:00 2001 From: Pieter-Jan Briers Date: Tue, 16 Jan 2024 21:04:39 +0100 Subject: [PATCH] Isolate net messages in integration tests. (#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 #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 --- ...ursedHorrorsBeyondOurWildestImagination.cs | 9 +++ Lidgren.Network/Lidgren.Network.csproj | 2 + NetSerializer | 2 +- RELEASE-NOTES.md | 2 +- .../Serialization/RobustSerializer.cs | 5 ++ .../RobustIntegrationTest.NetManager.cs | 76 ++++++++++++------- Robust.UnitTesting/RobustIntegrationTest.cs | 3 + .../GameObjects/ComponentMapInitTest.cs | 3 + .../GameObjects/DeferredEntityDeletionTest.cs | 5 ++ .../EntityEventBusTests.OrderedEvents.cs | 5 +- .../EntitySystemManagerOrderTest.cs | 5 ++ .../Systems/AnchoredSystemTests.cs | 4 + .../Shared/GameState/ComponentStateTests.cs | 33 ++------ .../Shared/TestRobustSerializerHash.cs | 43 +++++++++++ 14 files changed, 139 insertions(+), 58 deletions(-) create mode 100644 Lidgren.Network/CursedHorrorsBeyondOurWildestImagination.cs create mode 100644 Robust.UnitTesting/Shared/TestRobustSerializerHash.cs diff --git a/Lidgren.Network/CursedHorrorsBeyondOurWildestImagination.cs b/Lidgren.Network/CursedHorrorsBeyondOurWildestImagination.cs new file mode 100644 index 00000000000..de48deb2833 --- /dev/null +++ b/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")] + diff --git a/Lidgren.Network/Lidgren.Network.csproj b/Lidgren.Network/Lidgren.Network.csproj index ae94848aefe..4b4a1ea0772 100644 --- a/Lidgren.Network/Lidgren.Network.csproj +++ b/Lidgren.Network/Lidgren.Network.csproj @@ -16,6 +16,8 @@ + + %(RecursiveDir)%(Filename)%(Extension) diff --git a/NetSerializer b/NetSerializer index 7224829e872..7f51deaecab 160000 --- a/NetSerializer +++ b/NetSerializer @@ -1 +1 @@ -Subproject commit 7224829e872bfa4eb873439f61dda7ced1a2347f +Subproject commit 7f51deaecab8498a8953f7fb3e551b9892c5dff4 diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index dc2f7cbb1c2..be08f44c6e8 100644 --- a/RELEASE-NOTES.md +++ b/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 diff --git a/Robust.Shared/Serialization/RobustSerializer.cs b/Robust.Shared/Serialization/RobustSerializer.cs index e0b1c32129e..14b06f838d7 100644 --- a/Robust.Shared/Serialization/RobustSerializer.cs +++ b/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 GetTypeMap() => _serializer.GetTypeMap(); diff --git a/Robust.UnitTesting/RobustIntegrationTest.NetManager.cs b/Robust.UnitTesting/RobustIntegrationTest.NetManager.cs index 96aaf1720bf..04f822384a6 100644 --- a/Robust.UnitTesting/RobustIntegrationTest.NetManager.cs +++ b/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 /// public ChannelWriter? 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 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.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.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 { diff --git a/Robust.UnitTesting/RobustIntegrationTest.cs b/Robust.UnitTesting/RobustIntegrationTest.cs index 9a1829fdee4..101bcadf9ee 100644 --- a/Robust.UnitTesting/RobustIntegrationTest.cs +++ b/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().LoadAssemblies(typeof(RobustIntegrationTest).Assembly); var server = DependencyCollection.Resolve(); @@ -860,6 +862,7 @@ private GameController Init() deps.BuildGraph(); GameController.RegisterReflection(deps); + deps.Resolve().LoadAssemblies(typeof(RobustIntegrationTest).Assembly); var client = DependencyCollection.Resolve(); diff --git a/Robust.UnitTesting/Server/GameObjects/ComponentMapInitTest.cs b/Robust.UnitTesting/Server/GameObjects/ComponentMapInitTest.cs index ff0c0d60dc2..1d1e07af010 100644 --- a/Robust.UnitTesting/Server/GameObjects/ComponentMapInitTest.cs +++ b/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; diff --git a/Robust.UnitTesting/Shared/GameObjects/DeferredEntityDeletionTest.cs b/Robust.UnitTesting/Shared/GameObjects/DeferredEntityDeletionTest.cs index a996e7aa124..7e3a4dc1544 100644 --- a/Robust.UnitTesting/Shared/GameObjects/DeferredEntityDeletionTest.cs +++ b/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(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 { } diff --git a/Robust.UnitTesting/Shared/GameObjects/EntityEventBusTests.OrderedEvents.cs b/Robust.UnitTesting/Shared/GameObjects/EntityEventBusTests.OrderedEvents.cs index f2bb66c2f71..17a6d8354b2 100644 --- a/Robust.UnitTesting/Shared/GameObjects/EntityEventBusTests.OrderedEvents.cs +++ b/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 { diff --git a/Robust.UnitTesting/Shared/GameObjects/EntitySystemManagerOrderTest.cs b/Robust.UnitTesting/Shared/GameObjects/EntitySystemManagerOrderTest.cs index a6148c20e72..70cee4e8746 100644 --- a/Robust.UnitTesting/Shared/GameObjects/EntitySystemManagerOrderTest.cs +++ b/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 UpdatesAfter => new[] {typeof(TestSystemA)}; } + [Reflect(false)] private sealed class TestSystemC : TestSystemBase { public override IEnumerable UpdatesBefore => new[] {typeof(TestSystemB)}; } + [Reflect(false)] private sealed class TestSystemD : TestSystemBase { public override IEnumerable UpdatesAfter => new[] {typeof(TestSystemA)}; diff --git a/Robust.UnitTesting/Shared/GameObjects/Systems/AnchoredSystemTests.cs b/Robust.UnitTesting/Shared/GameObjects/Systems/AnchoredSystemTests.cs index cd65436c651..b6753ac59fa 100644 --- a/Robust.UnitTesting/Shared/GameObjects/Systems/AnchoredSystemTests.cs +++ b/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!; diff --git a/Robust.UnitTesting/Shared/GameState/ComponentStateTests.cs b/Robust.UnitTesting/Shared/GameState/ComponentStateTests.cs index d31956df02f..0aa713f7ca8 100644 --- a/Robust.UnitTesting/Shared/GameState/ComponentStateTests.cs +++ b/Robust.UnitTesting/Shared/GameState/ComponentStateTests.cs @@ -8,6 +8,7 @@ using Robust.Shared.IoC; using Robust.Shared.Map; using Robust.Shared.Network; +using Robust.Shared.Reflection; using Robust.Shared.Serialization.Manager.Attributes; namespace Robust.UnitTesting.Shared.GameState; @@ -23,20 +24,8 @@ public sealed partial class ComponentStateTests : RobustIntegrationTest public async Task UnknownEntityTest() { // Setup auto-comp-states. I hate this. Someone please fix reflection in RobustIntegrationTest - var compReg = () => IoCManager.Resolve().RegisterClass(); - var sysReg = () => IoCManager.Resolve().LoadExtraSystemType(); - var serverOpts = new ServerIntegrationOptions - { - Pool = false, - BeforeRegisterComponents = compReg, - BeforeStart = sysReg, - }; - var clientOpts = new ClientIntegrationOptions - { - Pool = false, - BeforeRegisterComponents = compReg, - BeforeStart = sysReg, - }; + var serverOpts = new ServerIntegrationOptions { Pool = false }; + var clientOpts = new ClientIntegrationOptions { Pool = false }; var server = StartServer(serverOpts); var client = StartClient(clientOpts); @@ -159,20 +148,8 @@ async Task RunTicks() public async Task UnknownEntityDeleteTest() { // The first chunk of the test just follows UnknownEntityTest - var compReg = () => IoCManager.Resolve().RegisterClass(); - var sysReg = () => IoCManager.Resolve().LoadExtraSystemType(); - var serverOpts = new ServerIntegrationOptions - { - Pool = false, - BeforeRegisterComponents = compReg, - BeforeStart = sysReg, - }; - var clientOpts = new ClientIntegrationOptions - { - Pool = false, - BeforeRegisterComponents = compReg, - BeforeStart = sysReg, - }; + var serverOpts = new ServerIntegrationOptions { Pool = false }; + var clientOpts = new ClientIntegrationOptions { Pool = false }; var server = StartServer(serverOpts); var client = StartClient(clientOpts); diff --git a/Robust.UnitTesting/Shared/TestRobustSerializerHash.cs b/Robust.UnitTesting/Shared/TestRobustSerializerHash.cs new file mode 100644 index 00000000000..1995ed0f923 --- /dev/null +++ b/Robust.UnitTesting/Shared/TestRobustSerializerHash.cs @@ -0,0 +1,43 @@ +using System.IO; +using System.Text; +using System.Threading.Tasks; +using NUnit.Framework; +using Robust.Shared.IoC; +using Robust.Shared.Serialization; +using Robust.Shared.Utility; + +namespace Robust.UnitTesting.Shared; + +[Parallelizable(ParallelScope.All)] +internal sealed class TestRobustSerializerHash : RobustIntegrationTest +{ + /// + /// Test that the serializer hash on client and server matches. + /// + [Test] + public async Task Test() + { + var server = StartServer(); + var client = StartClient(); + + var manifestServerStream = new MemoryStream(); + var manifestClientStream = new MemoryStream(); + + await server.WaitPost(() => + { + var serializer = (RobustSerializer) IoCManager.Resolve(); + serializer.GetHashManifest(manifestServerStream, writeNewline: true); + }); + + await client.WaitPost(() => + { + var serializer = (RobustSerializer) IoCManager.Resolve(); + serializer.GetHashManifest(manifestClientStream, writeNewline: true); + }); + + var manifestServer = Encoding.UTF8.GetString(manifestServerStream.AsSpan()); + var manifestClient = Encoding.UTF8.GetString(manifestClientStream.AsSpan()); + + Assert.That(manifestServer, NUnit.Framework.Is.EqualTo(manifestClient)); + } +}