-
Notifications
You must be signed in to change notification settings - Fork 106
fix(tests): Enable accurate coverage reporting with pytest-xdist #1658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…compatibility Move local class definitions from inside test functions to module level to prevent test pollution when running tests in parallel with pytest-xdist. When classes like DifferentAgent, TestAction, etc. are defined inside test functions, they get registered with Pydantic's discriminated union system but are not importable, causing failures in other tests running in the same worker. Files changed: - test_state_serialization.py: Move _DifferentAgentForVerifyTest to module level - test_reasoning_content.py: Move _TestActionForReasoningContent to module level - test_schema_immutability.py: Move _SchemaImmutabilityCustomAction and _SchemaImmutabilityCustomObservation to module level This fixes test pollution that was masked by --forked flag, enabling the switch to pytest-xdist for accurate coverage reporting. Fixes #1656 Co-authored-by: openhands <[email protected]>
Co-authored-by: openhands <[email protected]>
The --forked flag prevents coverage collection from child processes, resulting in artificially low coverage reports (near 0% in some cases). Switching to pytest-xdist (-n auto) provides: - Parallel test execution (same benefit as --forked) - Proper coverage collection from all worker processes - Better test isolation through process separation This change affects sdk-tests, tools-tests, and agent-server-tests. Closes #1656 Co-authored-by: openhands <[email protected]>
…lity Another local class in test_event_immutability.py was causing test pollution when running tests in parallel. Moved _TestEventForImmutability to module level. Co-authored-by: openhands <[email protected]>
…dist Additional local classes causing test pollution when running in parallel: - _UnknownEventForVisualizerTest in test_visualizer.py - _NestedActionForMalformedArgs in test_fix_malformed_tool_arguments.py - _ChildMCPToolActionForSerialization in test_mcp_action_serialization.py Co-authored-by: openhands <[email protected]>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
With this fix coverage reported in CI is now not the incorrect 52% but the more accurate 77%. |
Co-authored-by: openhands <[email protected]>
Terminal tests use shared paths (/tmp/test_dir) and subprocess management that conflicts when run in parallel with pytest-xdist. Co-authored-by: openhands <[email protected]>
|
We had to put --forked back in for tools-tests because they were hanging likely because the terminal tests have conflicts when run in parallel:
As a consequence coverage dropped back down to 68% but that is still closer to accurate than the 52% we were seeing before. |
Co-authored-by: openhands <[email protected]>
all-hands-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! The dependencies are properly configured with pytest-cov>=5.0.0 and pytest-xdist>=3.6.0.
Suggested verification:
- Compare coverage reports before/after this change to confirm the improvement
- Ensure no tests are being skipped due to xdist compatibility issues
- Verify that all 1679 tests pass consistently with
-n auto
2. Test Parallel Execution Safety
With pytest-xdist, tests run in parallel across multiple workers. Verify that tests don't have race conditions from:
- Shared file system access (temp files, test fixtures)
- Environment variable modifications
- Shared database connections
- Global state mutations
3. Consider Coverage Configuration
For optimal pytest-xdist coverage collection, consider adding explicit coverage configuration:
[tool.coverage.run]
parallel = true
concurrency = ["multiprocessing"]This ensures coverage data from parallel workers is properly combined.
✨ Positive Observations
- Consistent naming convention: All moved classes use underscore prefix (
_ClassName) clearly indicating they're internal test utilities - Excellent documentation: Each moved class has a clear docstring explaining why it's at module level
- Minimal changes: The refactoring is focused and doesn't introduce unnecessary modifications
- Proper dependency management: Both
pytest-xdistandpytest-forkedare properly declared in dependencies - Clear comments in workflow: The workflow changes include helpful comments explaining the rationale
📋 Summary
Issues requiring attention:
- Minor: Import inconsistency in
test_reasoning_content.py(low priority, style issue)
Recommendations:
- Verify coverage improvement claims
- Test for parallel execution race conditions
- Consider adding explicit coverage.run configuration for multiprocessing
The PR is well-structured and should achieve its stated goals of fixing test isolation and enabling accurate coverage reporting. The changes are safe to merge after addressing the minor import inconsistency (if desired) and performing the recommended verification steps.
xingyaoww
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
Fixes test isolation issues and replaces
--forkedwithpytest-xdistfor accurate coverage reporting in CI.Problem
The
--forkedpytest flag was causing two issues:Solution
1. Move local test classes to module level
Moved classes that are defined inside test functions to module level with underscore prefixes to indicate they are test-internal:
tests/sdk/conversation/local/test_state_serialization.py:_DifferentAgentForVerifyTesttests/sdk/llm/test_reasoning_content.py:_TestActionForReasoningContenttests/sdk/tool/test_schema_immutability.py:_SchemaImmutabilityCustomAction,_SchemaImmutabilityCustomObservation2. Replace
--forkedwith-n autoSwitched from
pytest-forkedtopytest-xdistfor parallel test execution:This provides:
Testing
pytest -n autoRelated Issues
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:d615f5b-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d615f5b-python) is a multi-arch manifest supporting both amd64 and arm64d615f5b-python-amd64) are also available if needed