Handle optional RL dependencies and async pytest support#9
Handle optional RL dependencies and async pytest support#9robotlearning123 merged 1 commit intomainfrom
Conversation
WalkthroughIntroduces a pytest hook in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Pytest as Pytest Runner
participant Hook as conftest.py::pytest_pyfunc_call
participant Test as Test Function
Pytest->>Hook: Call pytest_pyfunc_call(pyfuncitem)
alt Test is coroutine
Hook->>Test: asyncio.run(test_func(**funcargs))
Hook-->>Pytest: return True (handled)
else Test is normal function
Hook-->>Pytest: return None (not handled)
Pytest->>Test: Invoke normally
end
sequenceDiagram
autonumber
participant Pytest as Pytest Collector
participant Module as test_* module
participant Importlib as importlib.util
Pytest->>Module: Import module
Module->>Importlib: find_spec("gymnasium"/"scipy")
alt Dependency missing
Module-->>Pytest: pytest.skip(reason) at module level
else Dependency present
Module-->>Pytest: proceed with imports and test definitions
end
sequenceDiagram
autonumber
participant Runner as main()
participant Suite as RLTestSuite
participant Cats as Test Categories
Runner->>Suite: instantiate
Runner->>Suite: run_all_tests()
loop For each category
Suite->>Cats: execute tests
Cats-->>Suite: results
end
Suite-->>Runner: aggregated results / exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test_rl_integration.py (1)
7-30: Fix Black formatting failure.CI flagged this file for reformatting—please run
black tests/test_rl_integration.py(orblack .) so we keep the formatter satisfied.test_rl_functionality.py (1)
413-467: Pytest now skips this entire suiteEverything here lives inside
RLTestSuiteand is only driven bymain(). Because there are no module-leveltest_*callables anymore, pytest won’t discover or execute any of these checks—our RL coverage silently disappeared. Please expose real pytest tests again (e.g., keep the suite helper but adddef test_rl_functionality_suite(): assert RLTestSuite().run_all_tests()or restore the individualtest_*functions) so the runner actually exercises this code in CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
conftest.py(1 hunks)test_advanced_features.py(2 hunks)test_debug.py(1 hunks)test_rl_advanced.py(1 hunks)test_rl_functionality.py(1 hunks)test_rl_integration.py(2 hunks)test_rl_simple.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Follow PEP 8 style guide
Add docstrings to all functions
Use type hints where possible
Files:
test_rl_simple.pytest_rl_advanced.pytest_rl_functionality.pytest_debug.pyconftest.pytest_advanced_features.pytest_rl_integration.py
🧬 Code graph analysis (4)
test_rl_simple.py (1)
src/mujoco_mcp/rl_integration.py (7)
RLConfig(23-35)MuJoCoRLEnvironment(189-596)RLTrainer(599-689)create_reaching_env(693-701)create_balancing_env(704-712)create_walking_env(715-723)example_training(727-756)
test_rl_advanced.py (1)
src/mujoco_mcp/rl_integration.py (5)
RLConfig(23-35)MuJoCoRLEnvironment(189-596)create_reaching_env(693-701)create_balancing_env(704-712)create_walking_env(715-723)
test_rl_functionality.py (2)
src/mujoco_mcp/rl_integration.py (9)
RLConfig(23-35)MuJoCoRLEnvironment(189-596)RLTrainer(599-689)ReachingTaskReward(56-101)BalancingTaskReward(104-142)WalkingTaskReward(145-186)create_reaching_env(693-701)create_balancing_env(704-712)create_walking_env(715-723)src/mujoco_mcp/viewer_client.py (1)
MuJoCoViewerClient(19-270)
test_rl_integration.py (2)
src/mujoco_mcp/rl_integration.py (6)
RLConfig(23-35)MuJoCoRLEnvironment(189-596)RLTrainer(599-689)create_reaching_env(693-701)create_balancing_env(704-712)create_walking_env(715-723)src/mujoco_mcp/viewer_client.py (1)
MuJoCoViewerClient(19-270)
🪛 GitHub Actions: Code Quality
test_rl_simple.py
[error] 1-1: Black formatting check failed. File would be reformatted by 'black'. Run 'black .' to format.
test_rl_advanced.py
[error] 1-1: Black formatting check failed. File would be reformatted by 'black'. Run 'black .' to format.
test_rl_functionality.py
[error] 1-1: Black formatting check failed. File would be reformatted by 'black'. Run 'black .' to format.
conftest.py
[error] 16-16: TypeError in pytest runner: test_* function received an unexpected keyword argument 'event_loop_policy'. This originates from conftest.py and indicates a Pytest/pytest-asyncio compatibility issue. The CI step 'pytest --cov=src/mujoco_mcp --cov-report=xml --cov-report=html --cov-report=term-missing' failed with exit code 1.
test_rl_integration.py
[error] 1-1: Black formatting check failed. File would be reformatted by 'black'. Run 'black .' to format.
🔇 Additional comments (4)
test_advanced_features.py (1)
21-29: Clean skip for missing optional deps.I like this early guard—collecting every absent optional module and bailing out before the heavy imports keeps the suite tidy for minimal installs.
test_debug.py (1)
14-16: Gymnasium gate looks good.The module-level skip keeps these diagnostics quiet when gymnasium isn’t around—nice consistency with the broader test suite.
test_rl_advanced.py (1)
20-27: Solid optional dependency handling.This aligns the advanced RL suite with the new gating strategy and avoids pointless collection failures—thanks for the uniform approach.
test_rl_simple.py (1)
17-24: LGTM on the gymnasium gate.Consistent skip behavior across the RL tests will make life nicer for environments without gymnasium—looks great.
| def pytest_pyfunc_call(pyfuncitem: pytest.Item) -> bool | None: | ||
| """Allow ``async def`` tests to run without pytest-asyncio.""" | ||
| test_func = getattr(pyfuncitem, "obj", None) | ||
| if inspect.iscoroutinefunction(test_func): | ||
| asyncio.run(test_func(**pyfuncitem.funcargs)) | ||
| return True |
There was a problem hiding this comment.
Fix async hook to ignore non-parameter fixtures
CI is currently failing with TypeError: ... got an unexpected keyword argument 'event_loop_policy' coming from this hook. We call the coroutine with pyfuncitem.funcargs, which also includes fixtures injected via usefixtures/autouse (like event_loop_policy) that are not declared in the test signature; once we expand that dict we end up passing unexpected kwargs. Please filter the kwargs down to only the parameters the test actually declares before invoking the coroutine so the hook mirrors pytest’s normal call semantics.
test_func = getattr(pyfuncitem, "obj", None)
if inspect.iscoroutinefunction(test_func):
- asyncio.run(test_func(**pyfuncitem.funcargs))
+ signature = inspect.signature(test_func)
+ accepts_kwargs = any(
+ param.kind == inspect.Parameter.VAR_KEYWORD
+ for param in signature.parameters.values()
+ )
+ call_kwargs = (
+ pyfuncitem.funcargs
+ if accepts_kwargs
+ else {
+ name: pyfuncitem.funcargs[name]
+ for name in signature.parameters
+ if name in pyfuncitem.funcargs
+ }
+ )
+ asyncio.run(test_func(**call_kwargs))
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def pytest_pyfunc_call(pyfuncitem: pytest.Item) -> bool | None: | |
| """Allow ``async def`` tests to run without pytest-asyncio.""" | |
| test_func = getattr(pyfuncitem, "obj", None) | |
| if inspect.iscoroutinefunction(test_func): | |
| asyncio.run(test_func(**pyfuncitem.funcargs)) | |
| return True | |
| def pytest_pyfunc_call(pyfuncitem: pytest.Item) -> bool | None: | |
| """Allow ``async def`` tests to run without pytest-asyncio.""" | |
| test_func = getattr(pyfuncitem, "obj", None) | |
| if inspect.iscoroutinefunction(test_func): | |
| signature = inspect.signature(test_func) | |
| accepts_kwargs = any( | |
| param.kind == inspect.Parameter.VAR_KEYWORD | |
| for param in signature.parameters.values() | |
| ) | |
| call_kwargs = ( | |
| pyfuncitem.funcargs | |
| if accepts_kwargs | |
| else { | |
| name: pyfuncitem.funcargs[name] | |
| for name in signature.parameters | |
| if name in pyfuncitem.funcargs | |
| } | |
| ) | |
| asyncio.run(test_func(**call_kwargs)) | |
| return True |
🧰 Tools
🪛 GitHub Actions: Code Quality
[error] 16-16: TypeError in pytest runner: test_* function received an unexpected keyword argument 'event_loop_policy'. This originates from conftest.py and indicates a Pytest/pytest-asyncio compatibility issue. The CI step 'pytest --cov=src/mujoco_mcp --cov-report=xml --cov-report=html --cov-report=term-missing' failed with exit code 1.
🤖 Prompt for AI Agents
In conftest.py around lines 12 to 17, the async pytest hook calls the test
coroutine with pyfuncitem.funcargs which includes extra fixtures (like
autouse/usefixtures) not declared in the test signature and causes unexpected
keyword errors; before invoking asyncio.run, inspect the test function signature
to collect only parameter names actually declared by the test and filter
pyfuncitem.funcargs down to that set (e.g. build a kwargs dict containing only
keys present in the function's parameters), then call
asyncio.run(test_func(**filtered_kwargs)) and return True as before.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d93781e2b0832fb677b06108eaf810
Summary by CodeRabbit