Tests should always stop (#11338)

Content.IntegrationTests/PoolManager.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Content.Client.IoC;
@@ -60,7 +61,10 @@ public static class PoolManager
 
     private static int PairId;
     private static object PairLock = new();
-    private static List<Pair> Pairs = new();
+
+    // Pair, IsBorrowed
+    private static Dictionary<Pair, bool> Pairs = new();
+    private static bool Dead;
     private static Exception PoolFailureReason;
 
     private static async Task ConfigurePrototypes(RobustIntegrationTest.IntegrationInstance instance,
@@ -124,16 +128,38 @@ public static class PoolManager
     /// </summary>
     public static void Shutdown()
     {
+        List<Pair> localPairs;
         lock (PairLock)
         {
-            var pairs = Pairs;
+            if(Dead)
-            // We are trying to make things blow up if they are still happening after this method.
+                return;
-            Pairs = null;
+            Dead = true;
+            localPairs = Pairs.Keys.ToList();
+        }
+
+        foreach (var pair in localPairs)
+        {
+            pair.Kill();
+        }
+    }
+
+    public static string DeathReport()
+    {
+        lock (PairLock)
+        {
+            var builder = new StringBuilder();
+            var pairs = Pairs.Keys.OrderBy(pair => pair.PairId);
             foreach (var pair in pairs)
             {
-                pair.Client.Dispose();
+                var borrowed = Pairs[pair];
-                pair.Server.Dispose();
+                builder.AppendLine($"Pair {pair.PairId}, Tests Run: {pair.TestHistory.Count}, Borrowed: {borrowed}");
+                for (int i = 0; i < pair.TestHistory.Count; i++)
+                {
+                    builder.AppendLine($"#{i}: {pair.TestHistory[i]}");
+                }
             }
+
+            return builder.ToString();
         }
     }
 
@@ -276,7 +302,7 @@ public static class PoolManager
             if (pair != null && pair.TestHistory.Count > 1)
             {
                 await TestContext.Out.WriteLineAsync($"{nameof(GetServerClientPair)}: Pair {pair.PairId} Test History Start");
-                for (int i = 0; i < pair.TestHistory.Count - 1; i++)
+                for (int i = 0; i < pair.TestHistory.Count; i++)
                 {
                     await TestContext.Out.WriteLineAsync($"- Pair {pair.PairId} Test #{i}: {pair.TestHistory[i]}");
                 }
@@ -303,17 +329,25 @@ public static class PoolManager
     {
         lock (PairLock)
         {
-            if (Pairs.Count == 0) return null;
+            Pair fallback = null;
-            for (var i = 0; i < Pairs.Count; i++)
+            foreach (var pair in Pairs.Keys)
             {
-                var pair = Pairs[i];
+                if (Pairs[pair])
-                if (!pair.Settings.CanFastRecycle(poolSettings)) continue;
+                    continue;
-                Pairs.RemoveAt(i);
+                if (!pair.Settings.CanFastRecycle(poolSettings))
+                {
+                    fallback = pair;
+                    continue;
+                }
+                Pairs[pair] = true;
                 return pair;
             }
-            var defaultPair = Pairs[^1];
+
-            Pairs.RemoveAt(Pairs.Count - 1);
+            if (fallback != null)
-            return defaultPair;
+            {
+                Pairs[fallback!] = true;
+            }
+            return fallback;
         }
     }
 
@@ -325,7 +359,14 @@ public static class PoolManager
     {
         lock (PairLock)
         {
-            Pairs.Add(pair);
+            if (pair.Dead)
+            {
+                Pairs.Remove(pair);
+            }
+            else
+            {
+                Pairs[pair] = false;
+            }
         }
     }
 
@@ -429,11 +470,20 @@ public static class PoolManager
     {
         if (PoolFailureReason != null)
         {
+            // If the PoolFailureReason is not null, we can assume at least one test failed.
+            // So we say inconclusive so we don't add more failed tests to search through.
             Assert.Inconclusive(@"
 In a different test, the pool manager had an exception when trying to create a server/client pair.
 Instead of risking that the pool manager will fail at creating a server/client pairs for every single test,
 we are just going to end this here to save a lot of time. This is the exception that started this:\n {0}", PoolFailureReason);
         }
+
+        if (Dead)
+        {
+            // If Pairs is null, we ran out of time, we can't assume a test failed.
+            // So we are going to tell it all future tests are a failure.
+            Assert.Fail("The pool was shut down");
+        }
     }
     private static async Task<Pair> CreateServerClientPair(PoolSettings poolSettings)
     {
@@ -715,11 +765,19 @@ public sealed class TestMapData
 /// </summary>
 public sealed class Pair
 {
+    public bool Dead { get; private set; }
     public int PairId { get; init; }
     public List<string> TestHistory { get; set; } = new();
     public PoolSettings Settings { get; set; }
     public RobustIntegrationTest.ServerIntegrationInstance Server { get; init; }
     public RobustIntegrationTest.ClientIntegrationInstance Client { get; init; }
+
+    public void Kill()
+    {
+        Dead = true;
+        Server.Dispose();
+        Client.Dispose();
+    }
 }
 
 /// <summary>
@@ -735,8 +793,8 @@ public sealed class PairTracker : IAsyncDisposable
         await TestContext.Out.WriteLineAsync($"{nameof(DisposeAsync)}: Test gave back pair {Pair.PairId} in {usageTime.TotalMilliseconds} ms");
         var dirtyWatch = new Stopwatch();
         dirtyWatch.Start();
-        Pair.Client.Dispose();
+        Pair.Kill();
-        Pair.Server.Dispose();
+        PoolManager.NoCheckReturn(Pair);
         var disposeTime = dirtyWatch.Elapsed;
         await TestContext.Out.WriteLineAsync($"{nameof(DisposeAsync)}: Disposed pair {Pair.PairId} in {disposeTime.TotalMilliseconds} ms");
     }
@@ -764,8 +822,8 @@ public sealed class PairTracker : IAsyncDisposable
 
         if (Pair.Settings.MustNotBeReused)
         {
-            Pair.Client.Dispose();
+            Pair.Kill();
-            Pair.Server.Dispose();
+            PoolManager.NoCheckReturn(Pair);
             await PoolManager.ReallyBeIdle(Pair);
             var returnTime2 = cleanWatch.Elapsed;
             await TestContext.Out.WriteLineAsync($"{nameof(CleanReturnAsync)}: Clean disposed in {returnTime2.TotalMilliseconds} ms");

Content.IntegrationTests/PoolManagerTestEventHandler.cs

@@ -1,4 +1,6 @@
-using NUnit.Framework;
+using System;
+using System.Threading.Tasks;
+using NUnit.Framework;
 
 [assembly: Parallelizable(ParallelScope.Children)]
 
@@ -7,6 +9,26 @@ namespace Content.IntegrationTests;
 [SetUpFixture]
 public sealed class PoolManagerTestEventHandler
 {
+    // This value is double the usual time for Content Integration tests
+    private static TimeSpan MaximumTotalTestingTimeLimit => TimeSpan.FromMinutes(7);
+    private static TimeSpan HardStopTimeLimit => MaximumTotalTestingTimeLimit.Add(TimeSpan.FromMinutes(1));
+    [OneTimeSetUp]
+    public void Setup()
+    {
+        // If the tests seem to be stuck, we try to end it semi-nicely
+        _ = Task.Delay(MaximumTotalTestingTimeLimit).ContinueWith(_ =>
+        {
+            PoolManager.Shutdown();
+        });
+
+        // If ending it nicely doesn't work within a minute, we do something a bit meaner.
+        _ = Task.Delay(HardStopTimeLimit).ContinueWith(_ =>
+        {
+            var deathReport = PoolManager.DeathReport();
+            Environment.FailFast($"Tests took way too ;\n Death Report:\n{deathReport}");
+        });
+    }
+
     [OneTimeTearDown]
     public void TearDown()
     {

Content.IntegrationTests/Tests/Fluids/FluidSpillTest.cs

@@ -48,7 +48,6 @@ public sealed class FluidSpill
     {
         await using var pairTracker = await PoolManager.GetServerClient(new PoolSettings{NoClient = true});
         var server = pairTracker.Pair.Server;
-
         var mapManager = server.ResolveDependency<IMapManager>();
         var entityManager = server.ResolveDependency<IEntityManager>();
         var spillSystem = server.ResolveDependency<IEntitySystemManager>().GetEntitySystem<SpillableSystem>();