Fixes #2923. Ensures only clear Instances if they really was disposed. (#2924)

* Fixes #2923. Ensures only clear Instances if they really was disposed and fix unit tests.

* Add Ubuntu-20.04.

* xunit nuget package update.

Terminal.Gui/Input/Responder.cs

@@ -15,7 +15,7 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
+using System.Linq;
 using System.Reflection;
 
 namespace Terminal.Gui {
@@ -256,7 +256,7 @@ namespace Terminal.Gui {
 			}
 			return m.GetBaseDefinition ().DeclaringType != m.DeclaringType;
 		}
-		
+
 		/// <summary>
 		/// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
 		/// </summary>
@@ -266,7 +266,7 @@ namespace Terminal.Gui {
 		/// can be disposed.
 		/// If disposing equals false, the method has been called by the
 		/// runtime from inside the finalizer and you should not reference
-		/// other objects. Only unmanaged resources can be disposed.		
+		/// other objects. Only unmanaged resources can be disposed.
 		/// </remarks>
 		/// <param name="disposing"></param>
 		protected virtual void Dispose (bool disposing)
@@ -276,13 +276,10 @@ namespace Terminal.Gui {
 					// TODO: dispose managed state (managed objects)
 				}
 
-#if DEBUG_IDISPOSABLE
-				Instances.Remove (this);
-#endif                               
 				disposedValue = true;
 			}
 		}
-		
+
 		/// <summary>
 		/// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resource.
 		/// </summary>
@@ -293,6 +290,10 @@ namespace Terminal.Gui {
 			GC.SuppressFinalize (this);
 #if DEBUG_IDISPOSABLE
 			WasDisposed = true;
+
+			foreach (var instance in Instances.Where (x => x.WasDisposed).ToList ()) {
+				Instances.Remove (instance);
+			}
 #endif
 		}
 	}

UnitTests/Application/ApplicationTests.cs

@@ -77,9 +77,7 @@ namespace Terminal.Gui.ApplicationTests {
 			// Validate there are no outstanding Responder-based instances 
 			// after a scenario was selected to run. This proves the main UI Catalog
 			// 'app' closed cleanly.
-			//foreach (var inst in Responder.Instances) {
+			Assert.Empty (Responder.Instances);
-				//Assert.True (inst.WasDisposed);
-			//}
 #endif
 		}
 

UnitTests/Input/ResponderTests.cs

@@ -54,7 +54,9 @@ namespace Terminal.Gui.InputTests {
 #endif
 
 			r.Dispose ();
-
+#if DEBUG_IDISPOSABLE
+			Assert.Empty (Responder.Instances);
+#endif
 		}
 
 		public class DerivedView : View {

UnitTests/TestHelpers.cs

@@ -62,10 +62,13 @@ public class AutoInitShutdownAttribute : Xunit.Sdk.BeforeAfterTestAttribute {
 	{
 		Debug.WriteLine ($"Before: {methodUnderTest.Name}");
 		if (AutoInit) {
-#if DEBUG_IDISPOSABLE && FORCE_RESPONDER_DISPOSE
+#if DEBUG_IDISPOSABLE
 			// Clear out any lingering Responder instances from previous tests
-			Responder.Instances.Clear ();
+			if (Responder.Instances.Count == 0) {
-			Assert.Equal (0, Responder.Instances.Count);
+				Assert.Empty (Responder.Instances);
+			} else {
+				Responder.Instances.Clear ();
+			}
 #endif
 			Application.Init ((ConsoleDriver)Activator.CreateInstance (_driverType));
 		}
@@ -76,8 +79,12 @@ public class AutoInitShutdownAttribute : Xunit.Sdk.BeforeAfterTestAttribute {
 		Debug.WriteLine ($"After: {methodUnderTest.Name}");
 		if (AutoInit) {
 			Application.Shutdown ();
-#if DEBUG_IDISPOSABLE && FORCE_RESPONDER_DISPOSE
+#if DEBUG_IDISPOSABLE
-			Assert.Equal (0, Responder.Instances.Count);
+			if (Responder.Instances.Count == 0) {
+				Assert.Empty (Responder.Instances);
+			} else {
+				Responder.Instances.Clear ();
+			}
 #endif
 		}
 	}
@@ -104,9 +111,7 @@ public class TestRespondersDisposed : Xunit.Sdk.BeforeAfterTestAttribute {
 	{
 		Debug.WriteLine ($"After: {methodUnderTest.Name}");
 #if DEBUG_IDISPOSABLE
-#pragma warning disable xUnit2013 // Do not use equality check to check for collection size.
+		Assert.Empty (Responder.Instances);
-		Assert.Equal (0, Responder.Instances.Count);
-#pragma warning restore xUnit2013 // Do not use equality check to check for collection size.
 #endif
 	}
 }

UnitTests/UICatalog/ScenarioTests.cs

@@ -105,17 +105,11 @@ namespace UICatalog.Tests {
 
 				Application.Shutdown ();
 #if DEBUG_IDISPOSABLE
-				foreach (var inst in Responder.Instances) {
+				Assert.Empty (Responder.Instances);
-					Assert.True (inst.WasDisposed);
-				}
-				Responder.Instances.Clear ();
 #endif
 			}
 #if DEBUG_IDISPOSABLE
-			foreach (var inst in Responder.Instances) {
+				Assert.Empty (Responder.Instances);
-				Assert.True (inst.WasDisposed);
-			}
-			Responder.Instances.Clear ();
 #endif
 		}
 
@@ -132,20 +126,8 @@ namespace UICatalog.Tests {
 			// BUGBUG: (#2474) For some reason ReadKey is not returning the QuitKey for some Scenarios
 			// by adding this Space it seems to work.
 
-			FakeConsole.PushMockKeyPress (Key.Space);
 			FakeConsole.PushMockKeyPress (Application.QuitKey);
 
-			int iterations = 0;
-			Application.Iteration = () => {
-				iterations++;
-				output.WriteLine ($"'Generic' iteration {iterations}");
-				// Stop if we run out of control...
-				if (iterations == 10) {
-					output.WriteLine ($"'Generic' had to be force quit!");
-					Application.RequestStop ();
-				}
-			};
-
 			var ms = 100;
 			var abortCount = 0;
 			Func<MainLoop, bool> abortCallback = (MainLoop loop) => {
@@ -154,7 +136,22 @@ namespace UICatalog.Tests {
 				Application.RequestStop ();
 				return false;
 			};
-			var token = Application.MainLoop.AddTimeout (TimeSpan.FromMilliseconds (ms), abortCallback);
+
+			int iterations = 0;
+			object token = null;
+			Application.Iteration = () => {
+				if (token == null) {
+					// Timeout only must start at first iteration
+					token = Application.MainLoop.AddTimeout (TimeSpan.FromMilliseconds (ms), abortCallback);
+				}
+				iterations++;
+				output.WriteLine ($"'Generic' iteration {iterations}");
+				// Stop if we run out of control...
+				if (iterations == 10) {
+					output.WriteLine ($"'Generic' had to be force quit!");
+					Application.RequestStop ();
+				}
+			};
 
 			Application.Top.KeyPress += (object sender, KeyEventEventArgs args) => {
 				// See #2474 for why this is commented out
@@ -175,10 +172,7 @@ namespace UICatalog.Tests {
 			Application.Shutdown ();
 
 #if DEBUG_IDISPOSABLE
-			foreach (var inst in Responder.Instances) {
+			Assert.Empty (Responder.Instances);
-				Assert.True (inst.WasDisposed);
-			}
-			Responder.Instances.Clear ();
 #endif
 		}
 

UnitTests/UnitTests.csproj

@@ -25,7 +25,7 @@
     <PackageReference Include="ReportGenerator" Version="5.1.26" />
     <PackageReference Include="System.Collections" Version="4.3.0" />
     <PackageReference Include="TestableIO.System.IO.Abstractions.TestingHelpers" Version="19.2.69" />
-    <PackageReference Include="xunit" Version="2.5.2" />
+    <PackageReference Include="xunit" Version="2.5.3" />
     <PackageReference Include="xunit.runner.visualstudio" Version="2.5.3">
       <PrivateAssets>all</PrivateAssets>
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

UnitTests/Views/OverlappedTests.cs

@@ -32,6 +32,10 @@ namespace Terminal.Gui.ViewsTests {
 
 			Application.End (rs);
 			Application.Shutdown ();
+
+#if DEBUG_IDISPOSABLE
+			Assert.Empty (Responder.Instances);
+#endif
 		}
 
 		[Fact, TestRespondersDisposed]

testenvironments.json

@@ -10,6 +10,11 @@
 			"type": "wsl",
 			"wslDistribution": "Ubuntu"
 		},
+		{
+			"name": "WSL-Ubuntu-20.04",
+			"type": "wsl",
+			"wslDistribution": "Ubuntu-20.04"
+		},
 		{
 			"name": "WSL-Debian",
 			"type": "wsl",