Skip to content

Fix Python E2E test flakiness and missing pytest-timeout#391

Merged
SteveSandersonMS merged 1 commit intomainfrom
stevesa/python-e2e-fix
Feb 6, 2026
Merged

Fix Python E2E test flakiness and missing pytest-timeout#391
SteveSandersonMS merged 1 commit intomainfrom
stevesa/python-e2e-fix

Conversation

@SteveSandersonMS
Copy link
Contributor

Summary

Fixes the Python E2E test failure in CI: https://github.com/github/copilot-sdk/actions/runs/21746470248/job/62733447842

Changes

1. Fix race condition in test cleanup (context.py)

The test fixture cleanup was failing with OSError: Directory not empty when shutil.rmtree() tried to delete session-state directories while background processes were still writing files.

Fix: Added ignore_errors=True to shutil.rmtree() and missing_ok=True to unlink() to gracefully handle these race conditions.

2. Add missing pytest-timeout dependency (pyproject.toml)

The test_compaction.py test uses @pytest.mark.timeout(120) but the pytest-timeout plugin was not installed, causing the warning: Unknown pytest.mark.timeout

Fix: Added pytest-timeout>=2.0.0 to dev dependencies.

@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner February 6, 2026 10:10
Copilot AI review requested due to automatic review settings February 6, 2026 10:10
- Add ignore_errors=True to shutil.rmtree() in test cleanup to handle
  race conditions where files are still being written by background processes
- Add missing_ok=True to unlink() for same reason
- Add pytest-timeout to dev dependencies (fixes warning about unknown mark)
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.

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

python/e2e/testharness/context.py:112

  • The approach of silently ignoring errors with ignore_errors=True could mask legitimate issues beyond race conditions (e.g., permission problems, disk failures, or other filesystem errors). The Node.js SDK uses a more robust retry mechanism with exponential backoff (see nodejs/test/e2e/harness/sdkTestContext.ts:99-101) which would distinguish between transient race conditions and persistent failures.

Consider implementing a similar retry mechanism for Python instead of silently ignoring all errors. This would make cleanup more reliable while still providing visibility into persistent issues.

                shutil.rmtree(item, ignore_errors=True)

@github-actions
Copy link

github-actions bot commented Feb 6, 2026

Cross-SDK Consistency Review ✅

I've reviewed this PR for consistency across all SDK implementations. No consistency issues found - this change maintains good cross-language parity.

Summary

This PR fixes Python E2E test flakiness by handling race conditions during test cleanup. All three major SDKs handle the same problem with language-appropriate patterns:

SDK Cleanup Strategy Location
Python ignore_errors=True + missing_ok=True ✨ (this PR) python/e2e/testharness/context.py
Node.js Retry mechanism (5 attempts, 2s delay) nodejs/test/e2e/harness/sdkTestContext.ts:99-100
.NET Try-catch blocks silently catching exceptions dotnet/test/Harness/E2ETestContext.cs:105-106

Analysis

The Python change appropriately uses Python idioms (ignore_errors, missing_ok) rather than forcing a retry pattern from Node.js or try-catch from .NET. Each approach is equivalent in handling the race condition where background processes may still be writing files during cleanup.

The addition of pytest-timeout>=2.0.0 is Python-specific and has no cross-SDK implications (each language uses its own test timeout mechanisms).

Recommendation: ✅ Approve - maintains cross-SDK consistency while using appropriate language idioms.

AI generated by SDK Consistency Review Agent

@SteveSandersonMS SteveSandersonMS merged commit fc80c4e into main Feb 6, 2026
18 checks passed
@SteveSandersonMS SteveSandersonMS deleted the stevesa/python-e2e-fix branch February 6, 2026 10:15
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.

1 participant