Skip to content

fix(test): address PR #294 review — targeted teardown and stdout.close in finally#295

Merged
danielmeppiel merged 1 commit intomainfrom
fix/windows-teardown-review-feedback
Mar 14, 2026
Merged

fix(test): address PR #294 review — targeted teardown and stdout.close in finally#295
danielmeppiel merged 1 commit intomainfrom
fix/windows-teardown-review-feedback

Conversation

@danielmeppiel
Copy link
Collaborator

Summary

Addresses review feedback from #294 (two comments from Copilot reviewer).

Changes

  1. process.stdout.close() moved to finally blocks — Previously only ran on the happy path. If an exception occurred while reading subprocess output, pipe handles would stay open, potentially causing the exact WinError 32 we're fixing. Now runs unconditionally in finally.

  2. Targeted PermissionError handling in teardown — Replaced blanket ignore_errors=True with:

    • Catch PermissionError specifically
    • On Windows: retry once after 1s delay, then ignore_errors=True as last resort
    • On other platforms: re-raise so unexpected cleanup failures aren't masked

Review comments addressed

Ref #185

…finally

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>
Copilot AI review requested due to automatic review settings March 14, 2026 11:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Windows-flaky auto-install E2E integration tests to be more robust during subprocess shutdown and test directory teardown, addressing feedback from PR #294.

Changes:

  • Moves process.stdout.close() into finally blocks so pipe handles are closed even when exceptions occur while reading output.
  • Replaces shutil.rmtree(..., ignore_errors=True) with targeted PermissionError handling in teardown, including a Windows-only retry with a short delay.
Comments suppressed due to low confidence (4)

tests/integration/test_auto_install_e2e.py:239

  • Avoid using a bare except: here; it will also catch KeyboardInterrupt/SystemExit and can make test runs hard to abort. Prefer except Exception: (and optionally handle subprocess.TimeoutExpired explicitly if you want a distinct path).
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:287

  • Avoid using a bare except: here; it will also catch KeyboardInterrupt/SystemExit and can make test runs hard to abort. Prefer except Exception: (and optionally handle subprocess.TimeoutExpired explicitly if you want a distinct path).
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:318

  • Avoid using a bare except: here; it will also catch KeyboardInterrupt/SystemExit and can make test runs hard to abort. Prefer except Exception: (and optionally handle subprocess.TimeoutExpired explicitly if you want a distinct path).
        except:
            process.kill()
            process.wait()

tests/integration/test_auto_install_e2e.py:365

  • Avoid using a bare except: here; it will also catch KeyboardInterrupt/SystemExit and can make test runs hard to abort. Prefer except Exception: (and optionally handle subprocess.TimeoutExpired explicitly if you want a distinct path).
        except:
            process.kill()
            process.wait()

break
process.stdout.close()
process.wait(timeout=10)
except:
@danielmeppiel danielmeppiel merged commit e4a4baa into main Mar 14, 2026
17 checks passed
@danielmeppiel danielmeppiel deleted the fix/windows-teardown-review-feedback branch March 14, 2026 11:30
danielmeppiel added a commit that referenced this pull request Mar 14, 2026
- Replace Unicode symbols (✓, ✨) with ASCII equivalents in test output
  to avoid UnicodeEncodeError on Windows cp1252 stdout
- Add encoding='utf-8' and errors='replace' to subprocess.run/Popen calls
  so APM's Rich-formatted output can be captured on Windows
- Applied to guardrailing, MCP registry, and ADO test files

Fixes the second Windows CI failure (after teardown fix in #295):
  UnicodeEncodeError: 'charmap' codec can't encode character '\u2713'

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Mar 14, 2026
* fix(test): Windows Unicode encoding in integration tests

- Replace Unicode symbols (✓, ✨) with ASCII equivalents in test output
  to avoid UnicodeEncodeError on Windows cp1252 stdout
- Add encoding='utf-8' and errors='replace' to subprocess.run/Popen calls
  so APM's Rich-formatted output can be captured on Windows
- Applied to guardrailing, MCP registry, and ADO test files

Fixes the second Windows CI failure (after teardown fix in #295):
  UnicodeEncodeError: 'charmap' codec can't encode character '\u2713'

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* fix: replace remaining Unicode warning symbol in MCP test

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Mar 14, 2026
TemporaryDirectory context manager fails on Windows when subprocesses
still hold file locks. Use ignore_cleanup_errors=True (Python 3.10+)
and close Popen stdout in finally block to release handles.

Same WinError 32 pattern as the auto-install teardown fix (#295).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Mar 14, 2026
…#298)

TemporaryDirectory context manager fails on Windows when subprocesses
still hold file locks. Use ignore_cleanup_errors=True (Python 3.10+)
and close Popen stdout in finally block to release handles.

Same WinError 32 pattern as the auto-install teardown fix (#295).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants