[xharness] Do not mark DeviceLoaders as loaded when loading fails (#9367)

premun · mandel-macaque · commit 758773b400e0 · 2020-10-21T15:52:26.000-04:00
diff --git a/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared.Tests/Hardware/DevicesTest.cs b/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared.Tests/Hardware/DevicesTest.cs
@@ -113,10 +113,53 @@ public async Task LoadAsyncProcessSuccess (bool extraData)
 			Assert.AreEqual (0, devices.ConnectedTV.Count ());
 		}
 
-		private void AssertArgumentValue (MlaunchArgument arg, string expected, string message = null)
+		[Test]
+		public async Task FindAndCacheDevicesWithFailingMlaunchTest ()
 		{
-			var value = arg.AsCommandLineArgument ().Split (new char [] { '=' }, 2).LastOrDefault ();
-			Assert.AreEqual (expected, value, message);
+			string processPath = null;
+			MlaunchArguments passedArguments = null;
+
+			// Moq.SetupSequence doesn't allow custom callbacks so we need to count ourselves
+			var calls = 0;
+
+			// moq It.Is is not working as nicelly as we would like it, we capture data and use asserts
+			processManager.Setup (p => p.RunAsync (It.IsAny<Process> (), It.IsAny<MlaunchArguments> (), It.IsAny<ILog> (), It.IsAny<TimeSpan?> (), It.IsAny<Dictionary<string, string>> (), It.IsAny<CancellationToken?> (), It.IsAny<bool?> ()))
+				.Returns<Process, MlaunchArguments, ILog, TimeSpan?, Dictionary<string, string>, CancellationToken?, bool?> ((p, args, log, t, env, token, d) => {
+					calls++;
+
+					if (calls == 1) {
+						// Mlaunch can sometimes time out and we are testing that a subsequent Load will trigger it again
+						return Task.FromResult (new ProcessExecutionResult { ExitCode = 137, TimedOut = true });
+					}
+
+					processPath = p.StartInfo.FileName;
+					passedArguments = args;
+
+					// we get the temp file that was passed as the args, and write our sample xml, which will be parsed to get the devices :)
+					var tempPath = args.Where (a => a is ListDevicesArgument).First ().AsCommandLineArgument ();
+					tempPath = tempPath.Substring (tempPath.IndexOf ('=') + 1).Replace ("\"", string.Empty);
+
+					var name = GetType ().Assembly.GetManifestResourceNames ().Where (a => a.EndsWith ("devices.xml", StringComparison.Ordinal)).FirstOrDefault ();
+					using (var outputStream = new StreamWriter (tempPath))
+					using (var sampleStream = new StreamReader (GetType ().Assembly.GetManifestResourceStream (name))) {
+						string line;
+						while ((line = sampleStream.ReadLine ()) != null)
+							outputStream.WriteLine (line);
+					}
+					return Task.FromResult (new ProcessExecutionResult { ExitCode = 0, TimedOut = false });
+				});
+
+			Assert.ThrowsAsync<Exception> (async () => await devices.LoadDevices (executionLog.Object));
+
+			Assert.IsEmpty (devices.ConnectedDevices);
+			Assert.AreEqual (1, calls);
+			await devices.LoadDevices (executionLog.Object);
+			Assert.AreEqual (2, calls);
+			Assert.IsNotEmpty (devices.ConnectedDevices);
+			await devices.LoadDevices (executionLog.Object);
+			Assert.AreEqual (2, calls);
+			await devices.LoadDevices (executionLog.Object);
+			Assert.AreEqual (2, calls);
 		}
 	}
 }
diff --git a/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared.Tests/Hardware/SimulatorsTest.cs b/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared.Tests/Hardware/SimulatorsTest.cs
@@ -149,10 +149,41 @@ public async Task FindAsyncDoNotCreateTest (TestTarget target, int expected)
 			Assert.AreEqual (expected, sims.Count (), $"{target} simulators count");
 		}
 
-		private void AssertArgumentValue (MlaunchArgument arg, string expected, string message = null)
+		[Test]
+		public async Task FindAndCacheSimulatorsWithFailingMlaunchTest ()
 		{
-			var value = arg.AsCommandLineArgument ().Split (new char [] { '=' }, 2).LastOrDefault ();
-			Assert.AreEqual (expected, value, message);
+			// Moq.SetupSequence doesn't allow custom callbacks so we need to count ourselves
+			var calls = 0;
+
+			processManager
+				.Setup (p => p.RunAsync (It.IsAny<Process>(), It.Is<MlaunchArguments> (args => args.Any (a => a is ListSimulatorsArgument)), It.IsAny<ILog> (), It.IsAny<TimeSpan> (), It.IsAny<Dictionary<string, string>> (), It.IsAny<CancellationToken?> (), It.IsAny<bool?> ()))
+				.Returns<Process, MlaunchArguments, ILog, TimeSpan, Dictionary<string, string>, CancellationToken?, bool?> ((process, args, log, t, env, token, diagnostics) => {
+					calls++;
+
+					if (calls == 1) {
+						// Mlaunch can sometimes time out and we are testing that a subsequent Load will trigger it again
+						return Task.FromResult (new ProcessExecutionResult { ExitCode = 137, TimedOut = true });
+					}
+
+					// We get the temp file that was passed as the args, and write our sample xml, which will be parsed to get the devices
+					var tempPath = args.Where (a => a is ListSimulatorsArgument).First ().AsCommandLineArgument ();
+					tempPath = tempPath.Substring (tempPath.IndexOf ('=') + 1).Replace ("\"", string.Empty);
+
+					CopySampleData (tempPath);
+					return Task.FromResult (new ProcessExecutionResult { ExitCode = 0, TimedOut = false });
+				});
+
+			Assert.ThrowsAsync<Exception> (async () => await simulators.LoadDevices (executionLog.Object));
+
+			Assert.IsEmpty (simulators.AvailableDevices);
+			Assert.AreEqual (1, calls);
+			await simulators.LoadDevices (executionLog.Object);
+			Assert.AreEqual (2, calls);
+			Assert.IsNotEmpty (simulators.AvailableDevices);
+			await simulators.LoadDevices (executionLog.Object);
+			Assert.AreEqual (2, calls);
+			await simulators.LoadDevices (executionLog.Object);
+			Assert.AreEqual (2, calls);
 		}
 	}
 }
diff --git a/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared/Hardware/HardwareDeviceLoader.cs b/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared/Hardware/HardwareDeviceLoader.cs
@@ -3,6 +3,7 @@
 using System.Diagnostics;
 using System.IO;
 using System.Linq;
+using System.Threading;
 using System.Threading.Tasks;
 using System.Xml;
 using Microsoft.DotNet.XHarness.iOS.Shared.Collections;
@@ -27,6 +28,7 @@ public interface IHardwareDeviceLoader : IDeviceLoader {
 	}
 
 	public class HardwareDeviceLoader : IHardwareDeviceLoader {
+		readonly SemaphoreSlim semaphore = new SemaphoreSlim (1);
 		readonly IProcessManager processManager;
 		bool loaded;
 
@@ -46,14 +48,16 @@ public HardwareDeviceLoader (IProcessManager processManager)
 
 		public async Task LoadDevices (ILog log, bool includeLocked = false, bool forceRefresh = false, bool listExtraData = false)
 		{
+			await semaphore.WaitAsync ();
+
 			if (loaded) {
-				if (!forceRefresh)
+				if (!forceRefresh) {
+					semaphore.Release ();
 					return;
+				}
 				connectedDevices.Reset ();
 			}
 
-			loaded = true;
-
 			var tmpfile = Path.GetTempFileName ();
 			try {
 				using (var process = new Process ()) {
@@ -92,11 +96,14 @@ public async Task LoadDevices (ILog log, bool includeLocked = false, bool forceR
 						}
 						connectedDevices.Add (d);
 					}
+
+					loaded = true;
 				}
 			} finally {
 				connectedDevices.SetCompleted ();
 				File.Delete (tmpfile);
 				log.Flush ();
+				semaphore.Release ();
 			}
 		}
 
diff --git a/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared/Hardware/SimulatorLoader.cs b/tests/xharness/Microsoft.DotNet.XHarness.iOS.Shared/Hardware/SimulatorLoader.cs
@@ -49,7 +49,6 @@ public async Task LoadDevices (ILog log, bool includeLocked = false, bool forceR
 				available_devices.Reset ();
 				available_device_pairs.Reset ();
 			}
-			loaded = true;
 
 			await Task.Run (async () => {
 				var tmpfile = Path.GetTempFileName ();
@@ -115,6 +114,8 @@ await Task.Run (async () => {
 							});
 						}
 					}
+
+					loaded = true;
 				} finally {
 					supported_runtimes.SetCompleted ();
 					supported_device_types.SetCompleted ();

Original file line number	Diff line number	Diff line change
`@@ -3,6 +3,7 @@`
`3`	`3`	`using System.Diagnostics;`
`4`	`4`	`using System.IO;`
`5`	`5`	`using System.Linq;`
	`6`	`+using System.Threading;`
`6`	`7`	`using System.Threading.Tasks;`
`7`	`8`	`using System.Xml;`
`8`	`9`	`using Microsoft.DotNet.XHarness.iOS.Shared.Collections;`
`@@ -27,6 +28,7 @@ public interface IHardwareDeviceLoader : IDeviceLoader {`
`27`	`28`	`}`
`28`	`29`
`29`	`30`	`public class HardwareDeviceLoader : IHardwareDeviceLoader {`
	`31`	`+ readonly SemaphoreSlim semaphore = new SemaphoreSlim (1);`
`30`	`32`	`readonly IProcessManager processManager;`
`31`	`33`	`bool loaded;`
`32`	`34`
`@@ -46,14 +48,16 @@ public HardwareDeviceLoader (IProcessManager processManager)`
`46`	`48`
`47`	`49`	`public async Task LoadDevices (ILog log, bool includeLocked = false, bool forceRefresh = false, bool listExtraData = false)`
`48`	`50`	`{`
	`51`	`+ await semaphore.WaitAsync ();`
	`52`	`+`
`49`	`53`	`if (loaded) {`
`50`		`- if (!forceRefresh)`
	`54`	`+ if (!forceRefresh) {`
	`55`	`+ semaphore.Release ();`
`51`	`56`	`return;`
	`57`	`+ }`
`52`	`58`	`connectedDevices.Reset ();`
`53`	`59`	`}`
`54`	`60`
`55`		`- loaded = true;`
`56`		`-`
`57`	`61`	`var tmpfile = Path.GetTempFileName ();`
`58`	`62`	`try {`
`59`	`63`	`using (var process = new Process ()) {`
`@@ -92,11 +96,14 @@ public async Task LoadDevices (ILog log, bool includeLocked = false, bool forceR`
`92`	`96`	`}`
`93`	`97`	`connectedDevices.Add (d);`
`94`	`98`	`}`
	`99`	`+`
	`100`	`+ loaded = true;`
`95`	`101`	`}`
`96`	`102`	`} finally {`
`97`	`103`	`connectedDevices.SetCompleted ();`
`98`	`104`	`File.Delete (tmpfile);`
`99`	`105`	`log.Flush ();`
	`106`	`+ semaphore.Release ();`
`100`	`107`	`}`
`101`	`108`	`}`
`102`	`109`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,6 @@ public async Task LoadDevices (ILog log, bool includeLocked = false, bool forceR`
`49`	`49`	`available_devices.Reset ();`
`50`	`50`	`available_device_pairs.Reset ();`
`51`	`51`	`}`
`52`		`- loaded = true;`
`53`	`52`
`54`	`53`	`await Task.Run (async () => {`
`55`	`54`	`var tmpfile = Path.GetTempFileName ();`
`@@ -115,6 +114,8 @@ await Task.Run (async () => {`
`115`	`114`	`});`
`116`	`115`	`}`
`117`	`116`	`}`
	`117`	`+`
	`118`	`+ loaded = true;`
`118`	`119`	`} finally {`
`119`	`120`	`supported_runtimes.SetCompleted ();`
`120`	`121`	`supported_device_types.SetCompleted ();`