Fix StartInfo test (#44392)

* 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 <jkotas@microsoft.com>
* feedback

* Add process for disposal

* Make slow test outer loop

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

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
index dc8a82c..e096e96 100644 (file)
--- 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 @@ namespace System.Threading
         /// 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 @@ namespace System.Threading
 
             if (retState != state)
             {
+                if (throwOnError)
+                {
+                    string msg = SR.Format(SR.Thread_ApartmentState_ChangeFailed, retState);
+                    throw new InvalidOperationException(msg);
+                }
+
                 return false;
             }
 
@@ -445,9 +451,19 @@ namespace System.Threading
         [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
index 3d946ad..e1c3c55 100644 (file)
--- a/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs
+++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs
@@ -947,11 +947,7 @@ namespace System.Diagnostics.Tests
 
                 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 @@ namespace System.Diagnostics.Tests
 
                 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,11 +1157,7 @@ namespace System.Diagnostics.Tests
 
                 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
                 {
@@ -1184,5 +1165,30 @@ namespace System.Diagnostics.Tests
                 }
             }
         }
+
+        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
index 6b1d00e..efd8422 100644 (file)
--- a/src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs
+++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs
@@ -19,8 +19,12 @@ namespace System.Diagnostics.Tests
 
         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
index bb0edda..7348b70 100644 (file)
--- a/src/libraries/System.Diagnostics.Process/tests/ProcessWaitingTests.cs
+++ b/src/libraries/System.Diagnostics.Process/tests/ProcessWaitingTests.cs
@@ -637,6 +637,7 @@ namespace System.Diagnostics.Tests
         }
 
         [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
index cd4d6a2..575a5fd 100644 (file)
--- 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
index 1597214..5ceff52 100644 (file)
--- 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
deleted file mode 100644 (file)
index c68be85..0000000
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.Unix.cs
+++ /dev/null
@@ -1,10 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-
-namespace System.Threading
-{
-    public sealed partial class Thread
-    {
-        private static Exception GetApartmentStateChangeFailedException() => new PlatformNotSupportedException(SR.PlatformNotSupported_ComInterop);
-    }
-}

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
deleted file mode 100644 (file)
index 6bc647f..0000000
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.Windows.cs
+++ /dev/null
@@ -1,11 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-
-namespace System.Threading
-{
-    public sealed partial class Thread
-    {
-        private static Exception GetApartmentStateChangeFailedException() =>
-            new InvalidOperationException(SR.Thread_ApartmentState_ChangeFailed);
-    }
-}

diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
index 0bfcc94..e0b4978 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
@@ -295,14 +295,16 @@ namespace System.Threading
         [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)
             {
                 case ApartmentState.STA:
@@ -314,7 +316,7 @@ namespace System.Threading
                     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
index 17c08a5..877c2cb 100644 (file)
--- a/src/libraries/System.Threading.Thread/tests/ThreadTests.cs
+++ b/src/libraries/System.Threading.Thread/tests/ThreadTests.cs
@@ -190,7 +190,7 @@ namespace System.Threading.Threads.Tests
             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
index 0cc8310..2ca14d0 100644 (file)
--- 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 @@ namespace System.Threading
             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()
         {