Fix StartInfo test (#44392)

danmoseley · jkotas · web-flow · commit a207c7f5df92 · 2020-11-12T01:58:23.000Z
* Stabilize StartInfo_NotepadWithContent_withArgumentList

* Add state in failed SetApartmentState exception

* Same for other tests

* Extract common code

* Modify

* Extract common code

* Pass down throwOnError

* Apply suggestions from code review

Co-authored-by: Jan Kotas &lt;jkotas@microsoft.com&gt;

* feedback

* Add process for disposal

* Make slow test outer loop

Co-authored-by: Jan Kotas &lt;jkotas@microsoft.com&gt;
diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs
@@ -418,7 +418,7 @@ public ApartmentState GetApartmentState() =>
         /// single-threaded or multi-threaded apartment.
         /// </summary>
 #if FEATURE_COMINTEROP_APARTMENT_SUPPORT
-        private bool TrySetApartmentStateUnchecked(ApartmentState state)
+        private bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError)
         {
             ApartmentState retState = (ApartmentState)SetApartmentStateNative((int)state);
 
@@ -433,6 +433,12 @@ private bool TrySetApartmentStateUnchecked(ApartmentState state)
 
             if (retState != state)
             {
+                if (throwOnError)
+                {
+                    string msg = SR.Format(SR.Thread_ApartmentState_ChangeFailed, retState);
+                    throw new InvalidOperationException(msg);
+                }
+
                 return false;
             }
 
@@ -445,9 +451,19 @@ private bool TrySetApartmentStateUnchecked(ApartmentState state)
         [MethodImpl(MethodImplOptions.InternalCall)]
         internal extern int SetApartmentStateNative(int state);
 #else // FEATURE_COMINTEROP_APARTMENT_SUPPORT
-        private static bool TrySetApartmentStateUnchecked(ApartmentState state)
+        private static bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError)
         {
-            return state == ApartmentState.Unknown;
+             if (state != ApartmentState.Unknown)
+             {
+                if (throwOnError)
+                {
+                    throw new PlatformNotSupportedException(SR.PlatformNotSupported_ComInterop);
+                }
+
+                return false;
+             }
+
+             return true;
         }
 #endif // FEATURE_COMINTEROP_APARTMENT_SUPPORT
 
diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs
@@ -947,11 +947,7 @@ public void StartInfo_NotepadWithContent(bool useShellExecute)
 
                 try
                 {
-                    process.WaitForInputIdle(); // Give the file a chance to load
-                    Assert.Equal("notepad", process.ProcessName);
-
-                    // On some Windows versions, the file extension is not included in the title
-                    Assert.StartsWith(Path.GetFileNameWithoutExtension(tempFile), process.MainWindowTitle);
+                    VerifyNotepadMainWindowTitle(process, tempFile);
                 }
                 finally
                 {
@@ -985,18 +981,7 @@ public void StartInfo_TextFile_ShellExecute()
 
                 try
                 {
-                    process.WaitForInputIdle(); // Give the file a chance to load
-                    Assert.Equal("notepad", process.ProcessName);
-
-                    if (PlatformDetection.IsInAppContainer)
-                    {
-                        Assert.Throws<PlatformNotSupportedException>(() => process.MainWindowTitle);
-                    }
-                    else
-                    {
-                        // On some Windows versions, the file extension is not included in the title
-                        Assert.StartsWith(Path.GetFileNameWithoutExtension(tempFile), process.MainWindowTitle);
-                    }
+                    VerifyNotepadMainWindowTitle(process, tempFile);
                 }
                 finally
                 {
@@ -1172,17 +1157,38 @@ public void StartInfo_NotepadWithContent_withArgumentList(bool useShellExecute)
 
                 try
                 {
-                    process.WaitForInputIdle(); // Give the file a chance to load
-                    Assert.Equal("notepad", process.ProcessName);
-
-                    // On some Windows versions, the file extension is not included in the title
-                    Assert.StartsWith(Path.GetFileNameWithoutExtension(tempFile), process.MainWindowTitle);
+                    VerifyNotepadMainWindowTitle(process, tempFile);
                 }
                 finally
                 {
                     process?.Kill();
                 }
             }
         }
+
+        private void VerifyNotepadMainWindowTitle(Process process, string filename)
+        {
+            // On some Windows versions, the file extension is not included in the title
+            string expected = Path.GetFileNameWithoutExtension(filename);
+
+            process.WaitForInputIdle(); // Give the file a chance to load
+            Assert.Equal("notepad", process.ProcessName);
+
+            // Notepad calls CreateWindowEx with pWindowName of empty string, then calls SetWindowTextW
+            // with "Untitled - Notepad" then finally if you're opening a file, calls SetWindowTextW
+            // with something similar to "myfilename - Notepad". So there's a race between input idle
+            // and the expected MainWindowTitle because of how Notepad is implemented.
+            string title = process.MainWindowTitle;
+            int count = 0;
+            while (!title.StartsWith(expected) && count < 500)
+            {
+                Thread.Sleep(10);
+                process.Refresh();
+                title = process.MainWindowTitle;
+                count++;
+            }
+
+            Assert.StartsWith(expected, title);
+        }
     }
 }
diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs
@@ -19,8 +19,12 @@ public partial class ProcessTestBase : FileCleanupTestBase
 
         protected Process CreateDefaultProcess()
         {
+            if (_process != null)
+                throw new InvalidOperationException();
+
             _process = CreateProcessLong();
             _process.Start();
+            AddProcessForDispose(_process);
             return _process;
         }
 
diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessWaitingTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessWaitingTests.cs
@@ -637,6 +637,7 @@ public async Task WaitAsyncForSelfTerminatingChild()
         }
 
         [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        [OuterLoop("As written, takes 30 seconds")]
         public async Task WaitAsyncForProcess()
         {
             Process p = CreateSleepProcess(WaitInMS);
diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
@@ -3440,7 +3440,7 @@
     <value>An attempt was made to transition a task to a final state when it had already completed.</value>
   </data>
   <data name="Thread_ApartmentState_ChangeFailed" xml:space="preserve">
-    <value>Failed to set the specified COM apartment state.</value>
+    <value>Failed to set the specified COM apartment state. Current apartment state '{0}'.</value>
   </data>
   <data name="Thread_GetSetCompressedStack_NotSupported" xml:space="preserve">
     <value>Use CompressedStack.(Capture/Run) instead.</value>
diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
@@ -1623,7 +1623,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\Marshal.Windows.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\StandardOleMarshalObject.Windows.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Security\SecureString.Windows.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Threading\Thread.Windows.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\LowLevelMonitor.Windows.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\TimerQueue.Windows.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\TimeZoneInfo.Win32.cs" />
@@ -1829,7 +1828,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\StandardOleMarshalObject.Unix.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Security\SecureString.Unix.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\LowLevelMonitor.Unix.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Threading\Thread.Unix.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Threading\TimerQueue.Unix.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\TimeZoneInfo.Unix.cs" />
   </ItemGroup>
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.Unix.cs
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.Windows.cs
diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
@@ -295,13 +295,15 @@ public ApartmentState ApartmentState
         [SupportedOSPlatform("windows")]
         public void SetApartmentState(ApartmentState state)
         {
-            if (!TrySetApartmentState(state))
-            {
-                throw GetApartmentStateChangeFailedException();
-            }
+            SetApartmentState(state, throwOnError:true);
         }
 
         public bool TrySetApartmentState(ApartmentState state)
+        {
+            return SetApartmentState(state, throwOnError:false);
+        }
+
+        private bool SetApartmentState(ApartmentState state, bool throwOnError)
         {
             switch (state)
             {
@@ -314,7 +316,7 @@ public bool TrySetApartmentState(ApartmentState state)
                     throw new ArgumentOutOfRangeException(nameof(state), SR.ArgumentOutOfRange_Enum);
             }
 
-            return TrySetApartmentStateUnchecked(state);
+            return SetApartmentStateUnchecked(state, throwOnError);
         }
 
         [Obsolete("Thread.GetCompressedStack is no longer supported. Please use the System.Threading.CompressedStack class")]
diff --git a/src/libraries/System.Threading.Thread/tests/ThreadTests.cs b/src/libraries/System.Threading.Thread/tests/ThreadTests.cs
@@ -190,7 +190,7 @@ public static void ApartmentState_NoAttributePresent_DefaultState_Windows()
             RemoteExecutor.Invoke(() =>
             {
                 Assert.Equal(ApartmentState.MTA, Thread.CurrentThread.GetApartmentState());
-                Assert.Throws<InvalidOperationException>(() => Thread.CurrentThread.SetApartmentState(ApartmentState.STA));
+                AssertExtensions.ThrowsContains<InvalidOperationException>(() => Thread.CurrentThread.SetApartmentState(ApartmentState.STA), "MTA");
                 Thread.CurrentThread.SetApartmentState(ApartmentState.MTA);
             }).Dispose();
         }
diff --git a/src/mono/netcore/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs b/src/mono/netcore/System.Private.CoreLib/src/System/Threading/Thread.Mono.cs
@@ -309,7 +309,20 @@ public static bool Yield()
             return YieldInternal();
         }
 
-        private static bool TrySetApartmentStateUnchecked(ApartmentState state) => state == ApartmentState.Unknown;
+        private static bool SetApartmentStateUnchecked(ApartmentState state, bool throwOnError)
+        {
+             if (state != ApartmentState.Unknown)
+             {
+                if (throwOnError)
+                {
+                    throw new PlatformNotSupportedException(SR.PlatformNotSupported_ComInterop);
+                }
+
+                return false;
+             }
+
+             return true;
+        }
 
         private ThreadState ValidateThreadState()
         {

Original file line number	Diff line number	Diff line change
`@@ -947,11 +947,7 @@ public void StartInfo_NotepadWithContent(bool useShellExecute)`
`947`	`947`
`948`	`948`	`try`
`949`	`949`	`{`
`950`		`- process.WaitForInputIdle(); // Give the file a chance to load`
`951`		`- Assert.Equal("notepad", process.ProcessName);`
`952`		`-`
`953`		`- // On some Windows versions, the file extension is not included in the title`
`954`		`- Assert.StartsWith(Path.GetFileNameWithoutExtension(tempFile), process.MainWindowTitle);`
	`950`	`+ VerifyNotepadMainWindowTitle(process, tempFile);`
`955`	`951`	`}`
`956`	`952`	`finally`
`957`	`953`	`{`
`@@ -985,18 +981,7 @@ public void StartInfo_TextFile_ShellExecute()`
`985`	`981`
`986`	`982`	`try`
`987`	`983`	`{`
`988`		`- process.WaitForInputIdle(); // Give the file a chance to load`
`989`		`- Assert.Equal("notepad", process.ProcessName);`
`990`		`-`
`991`		`- if (PlatformDetection.IsInAppContainer)`
`992`		`- {`
`993`		`- Assert.Throws<PlatformNotSupportedException>(() => process.MainWindowTitle);`
`994`		`- }`
`995`		`- else`
`996`		`- {`
`997`		`- // On some Windows versions, the file extension is not included in the title`
`998`		`- Assert.StartsWith(Path.GetFileNameWithoutExtension(tempFile), process.MainWindowTitle);`
`999`		`- }`
	`984`	`+ VerifyNotepadMainWindowTitle(process, tempFile);`
`1000`	`985`	`}`
`1001`	`986`	`finally`
`1002`	`987`	`{`
`@@ -1172,17 +1157,38 @@ public void StartInfo_NotepadWithContent_withArgumentList(bool useShellExecute)`
`1172`	`1157`
`1173`	`1158`	`try`
`1174`	`1159`	`{`
`1175`		`- process.WaitForInputIdle(); // Give the file a chance to load`
`1176`		`- Assert.Equal("notepad", process.ProcessName);`
`1177`		`-`
`1178`		`- // On some Windows versions, the file extension is not included in the title`
`1179`		`- Assert.StartsWith(Path.GetFileNameWithoutExtension(tempFile), process.MainWindowTitle);`
	`1160`	`+ VerifyNotepadMainWindowTitle(process, tempFile);`
`1180`	`1161`	`}`
`1181`	`1162`	`finally`
`1182`	`1163`	`{`
`1183`	`1164`	`process?.Kill();`
`1184`	`1165`	`}`
`1185`	`1166`	`}`
`1186`	`1167`	`}`
	`1168`	`+`
	`1169`	`+ private void VerifyNotepadMainWindowTitle(Process process, string filename)`
	`1170`	`+ {`
	`1171`	`+ // On some Windows versions, the file extension is not included in the title`
	`1172`	`+ string expected = Path.GetFileNameWithoutExtension(filename);`
	`1173`	`+`
	`1174`	`+ process.WaitForInputIdle(); // Give the file a chance to load`
	`1175`	`+ Assert.Equal("notepad", process.ProcessName);`
	`1176`	`+`
	`1177`	`+ // Notepad calls CreateWindowEx with pWindowName of empty string, then calls SetWindowTextW`
	`1178`	`+ // with "Untitled - Notepad" then finally if you're opening a file, calls SetWindowTextW`
	`1179`	`+ // with something similar to "myfilename - Notepad". So there's a race between input idle`
	`1180`	`+ // and the expected MainWindowTitle because of how Notepad is implemented.`
	`1181`	`+ string title = process.MainWindowTitle;`
	`1182`	`+ int count = 0;`
	`1183`	`+ while (!title.StartsWith(expected) && count < 500)`
	`1184`	`+ {`
	`1185`	`+ Thread.Sleep(10);`
	`1186`	`+ process.Refresh();`
	`1187`	`+ title = process.MainWindowTitle;`
	`1188`	`+ count++;`
	`1189`	`+ }`
	`1190`	`+`
	`1191`	`+ Assert.StartsWith(expected, title);`
	`1192`	`+ }`
`1187`	`1193`	`}`
`1188`	`1194`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,8 +19,12 @@ public partial class ProcessTestBase : FileCleanupTestBase`
`19`	`19`
`20`	`20`	`protected Process CreateDefaultProcess()`
`21`	`21`	`{`
	`22`	`+ if (_process != null)`
	`23`	`+ throw new InvalidOperationException();`
	`24`	`+`
`22`	`25`	`_process = CreateProcessLong();`
`23`	`26`	`_process.Start();`
	`27`	`+ AddProcessForDispose(_process);`
`24`	`28`	`return _process;`
`25`	`29`	`}`
`26`	`30`
Original file line number	Diff line number	Diff line change
`@@ -637,6 +637,7 @@ public async Task WaitAsyncForSelfTerminatingChild()`
`637`	`637`	`}`
`638`	`638`
`639`	`639`	`[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]`
	`640`	`+ [OuterLoop("As written, takes 30 seconds")]`
`640`	`641`	`public async Task WaitAsyncForProcess()`
`641`	`642`	`{`
`642`	`643`	`Process p = CreateSleepProcess(WaitInMS);`
Original file line number	Diff line number	Diff line change
`@@ -295,13 +295,15 @@ public ApartmentState ApartmentState`
`295`	`295`	`[SupportedOSPlatform("windows")]`
`296`	`296`	`public void SetApartmentState(ApartmentState state)`
`297`	`297`	`{`
`298`		`- if (!TrySetApartmentState(state))`
`299`		`- {`
`300`		`- throw GetApartmentStateChangeFailedException();`
`301`		`- }`
	`298`	`+ SetApartmentState(state, throwOnError:true);`
`302`	`299`	`}`
`303`	`300`
`304`	`301`	`public bool TrySetApartmentState(ApartmentState state)`
	`302`	`+ {`
	`303`	`+ return SetApartmentState(state, throwOnError:false);`
	`304`	`+ }`
	`305`	`+`
	`306`	`+ private bool SetApartmentState(ApartmentState state, bool throwOnError)`
`305`	`307`	`{`
`306`	`308`	`switch (state)`
`307`	`309`	`{`
`@@ -314,7 +316,7 @@ public bool TrySetApartmentState(ApartmentState state)`
`314`	`316`	`throw new ArgumentOutOfRangeException(nameof(state), SR.ArgumentOutOfRange_Enum);`
`315`	`317`	`}`
`316`	`318`
`317`		`- return TrySetApartmentStateUnchecked(state);`
	`319`	`+ return SetApartmentStateUnchecked(state, throwOnError);`
`318`	`320`	`}`
`319`	`321`
`320`	`322`	`[Obsolete("Thread.GetCompressedStack is no longer supported. Please use the System.Threading.CompressedStack class")]`
Original file line number	Diff line number	Diff line change
`@@ -190,7 +190,7 @@ public static void ApartmentState_NoAttributePresent_DefaultState_Windows()`
`190`	`190`	`RemoteExecutor.Invoke(() =>`
`191`	`191`	`{`
`192`	`192`	`Assert.Equal(ApartmentState.MTA, Thread.CurrentThread.GetApartmentState());`
`193`		`- Assert.Throws<InvalidOperationException>(() => Thread.CurrentThread.SetApartmentState(ApartmentState.STA));`
	`193`	`+ AssertExtensions.ThrowsContains<InvalidOperationException>(() => Thread.CurrentThread.SetApartmentState(ApartmentState.STA), "MTA");`
`194`	`194`	`Thread.CurrentThread.SetApartmentState(ApartmentState.MTA);`
`195`	`195`	`}).Dispose();`
`196`	`196`	`}`