From ae3e92a90396b1f4bca89118651aefe6f79c261c Mon Sep 17 00:00:00 2001
From: danielmeppiel <dmeppiel@microsoft.com>
Date: Sat, 14 Mar 2026 12:23:28 +0100
Subject: [PATCH] =?UTF-8?q?fix(test):=20address=20PR=20review=20=E2=80=94?=
 =?UTF-8?q?=20targeted=20teardown=20and=20stdout.close=20in=20finally?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Move process.stdout.close() into finally blocks so pipe handles are
released even when exceptions occur during output reading.

Replace blanket ignore_errors=True with targeted PermissionError
handling: retry once after 1s on Windows (WinError 32), re-raise on
other platforms so unexpected cleanup failures aren't masked.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 tests/integration/test_auto_install_e2e.py | 32 +++++++++++++++-------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/tests/integration/test_auto_install_e2e.py b/tests/integration/test_auto_install_e2e.py
index 33b9a468..b3ba24b7 100644
--- a/tests/integration/test_auto_install_e2e.py
+++ b/tests/integration/test_auto_install_e2e.py
@@ -11,10 +11,12 @@
 """
 
 import os
+import platform
 import pytest
 import subprocess
 import tempfile
 import shutil
+import time
 from pathlib import Path
 
 
@@ -69,10 +71,17 @@ def teardown_method(self):
         """Clean up test environment."""
         os.chdir(self.original_dir)
         if os.path.exists(self.test_dir):
-            # ignore_errors=True: on Windows, recently-terminated subprocesses
-            # may still hold file locks on the temp directory (WinError 32).
-            # CI temp dirs are ephemeral — safe to leave behind if needed.
-            shutil.rmtree(self.test_dir, ignore_errors=True)
+            try:
+                shutil.rmtree(self.test_dir)
+            except PermissionError:
+                if platform.system() == "Windows":
+                    # On Windows, recently-terminated subprocesses may still
+                    # hold file locks (WinError 32). Retry once after a brief
+                    # delay; CI temp dirs are ephemeral so a leftover is OK.
+                    time.sleep(1)
+                    shutil.rmtree(self.test_dir, ignore_errors=True)
+                else:
+                    raise
     
     def test_auto_install_virtual_prompt_first_run(self, temp_e2e_home):
         """Test auto-install on first run with virtual package reference.
@@ -129,7 +138,6 @@ def test_auto_install_virtual_prompt_first_run(self, temp_e2e_home):
                     break
             
             # Wait for graceful shutdown
-            process.stdout.close()
             try:
                 process.wait(timeout=10)
             except subprocess.TimeoutExpired:
@@ -137,6 +145,7 @@ def test_auto_install_virtual_prompt_first_run(self, temp_e2e_home):
                 process.wait()
         
         finally:
+            process.stdout.close()
             output = ''.join(output_lines)
         
         # Check output for auto-install messages
@@ -189,11 +198,12 @@ def test_auto_install_uses_cache_on_second_run(self, temp_e2e_home):
                 if "Package installed and ready to run" in line:
                     process.terminate()
                     break
-            process.stdout.close()
             process.wait(timeout=10)
         except:
             process.kill()
             process.wait()
+        finally:
+            process.stdout.close()
         
         # Verify package exists
         package_path = Path("apm_modules") / "github" / "awesome-copilot" / "skills" / "architecture-blueprint-generator"
@@ -223,12 +233,12 @@ def test_auto_install_uses_cache_on_second_run(self, temp_e2e_home):
                 if "Executing" in line or "Package installed and ready to run" in line:
                     process.terminate()
                     break
-            process.stdout.close()
             process.wait(timeout=10)
         except:
             process.kill()
             process.wait()
         finally:
+            process.stdout.close()
             output = ''.join(output_lines)
         
         # Check output - should NOT show install/download messages
@@ -271,11 +281,12 @@ def test_simple_name_works_after_install(self, temp_e2e_home):
                 if "Package installed and ready to run" in line:
                     process.terminate()
                     break
-            process.stdout.close()
             process.wait(timeout=10)
         except:
             process.kill()
             process.wait()
+        finally:
+            process.stdout.close()
         
         # Run with simple name - early termination
         process = subprocess.Popen(
@@ -301,12 +312,12 @@ def test_simple_name_works_after_install(self, temp_e2e_home):
                 if "Executing" in line or "Auto-discovered" in line:
                     process.terminate()
                     break
-            process.stdout.close()
             process.wait(timeout=10)
         except:
             process.kill()
             process.wait()
         finally:
+            process.stdout.close()
             output = ''.join(output_lines)
         
         # Check output - should discover the installed prompt
@@ -348,11 +359,12 @@ def test_auto_install_with_qualified_path(self, temp_e2e_home):
                 if "Package installed and ready to run" in line:
                     process.terminate()
                     break
-            process.stdout.close()
             process.wait(timeout=10)
         except:
             process.kill()
             process.wait()
+        finally:
+            process.stdout.close()
         
         # Check that package was installed
         package_path = Path("apm_modules/github/awesome-copilot/skills/architecture-blueprint-generator")