Convert skip decorators to xfail and harden framing tests #16
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@greptileai what do you think pal? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR focuses on test infrastructure improvements and protocol hardening for the Unity MCP Bridge project. The changes span across multiple test files and the core Unity connection module, implementing three main improvements:
AST-based Code Analysis: The stdout detection logic in test_logging_stdout.py has been completely rewritten to use Python's Abstract Syntax Tree (AST) parsing instead of regex pattern matching. This change provides more accurate detection of print statements and sys.stdout.write() calls by understanding Python syntax rather than relying on text patterns that could produce false positives from comments or strings.
Test Placeholder Standardization: Multiple test files (test_script_editing.py and test_resources_api.py) have had their @pytest.mark.skip decorators converted to @pytest.mark.xfail(strict=False). This semantic change transforms completely skipped placeholder tests into expected-to-fail tests that can still be discovered and potentially pass if implemented. The reason strings have also been updated from 'TODO:' prefixes to more descriptive 'pending:' descriptions.
Transport Protocol Hardening: The framing tests in test_transport_framing.py have been enhanced with more robust error handling, timeout management, and frame reading logic. Additionally, the core Unity connection module now validates and rejects zero-length frames in the MCP protocol, adding defensive programming measures to prevent potential edge cases in the communication protocol.
These changes collectively improve the reliability of the test suite, provide better visibility into unimplemented functionality, and strengthen the underlying MCP transport protocol without affecting the core application logic.
Important Files Changed
Click to expand file changes
| Filename | Score | Overview |
|---|---|---|
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py |
4/5 | Added validation to reject zero-length frames in MCP protocol framing |
tests/test_logging_stdout.py |
4/5 | Replaced regex-based detection with AST parsing for stdout usage analysis |
tests/test_script_editing.py |
5/5 | Converted skip decorators to xfail for 7 placeholder test functions |
tests/test_resources_api.py |
5/5 | Converted skip decorators to xfail for 2 placeholder test functions |
tests/test_transport_framing.py |
4/5 | Hardened framing tests with improved timeout handling and error management |
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it focuses on test infrastructure improvements and defensive protocol validation
- Score reflects solid implementation of test hardening and protocol validation with minimal impact on core functionality
- Pay close attention to
unity_connection.pyto ensure zero-length frame rejection doesn't break legitimate protocol usage
Sequence Diagram
sequenceDiagram
participant User
participant TestFramework as "Test Framework (pytest)"
participant UnityConnection as "UnityConnection"
participant DummyServer as "Dummy Server"
participant Socket as "TCP Socket"
User->>TestFramework: "Run transport framing tests"
TestFramework->>TestFramework: "Locate Unity MCP server source"
TestFramework->>DummyServer: "start_dummy_server(greeting, respond_ping)"
DummyServer->>Socket: "bind() and listen() on random port"
DummyServer->>TestFramework: "Return port number"
TestFramework->>UnityConnection: "Create connection with test port"
UnityConnection->>Socket: "connect(host, port)"
Socket->>DummyServer: "Accept connection"
DummyServer->>UnityConnection: "Send greeting (FRAMING=1 or not)"
UnityConnection->>UnityConnection: "Check greeting for FRAMING=1"
alt Greeting contains FRAMING=1
UnityConnection->>UnityConnection: "Set use_framing = True"
UnityConnection->>TestFramework: "Return success"
else No FRAMING=1
UnityConnection->>Socket: "Send error message with framed header"
UnityConnection->>Socket: "close()"
UnityConnection->>TestFramework: "Return connection failure"
end
opt Ping-pong test
TestFramework->>UnityConnection: "Send framed ping command"
UnityConnection->>Socket: "sendall(header + payload)"
Socket->>DummyServer: "Receive framed data"
DummyServer->>DummyServer: "Parse frame and validate ping"
DummyServer->>Socket: "Send framed pong response"
Socket->>UnityConnection: "receive_full_response()"
UnityConnection->>UnityConnection: "_read_exact() for header and payload"
UnityConnection->>TestFramework: "Return parsed response"
end
TestFramework->>UnityConnection: "disconnect()"
UnityConnection->>Socket: "close()"
TestFramework->>User: "Test results"
5 files reviewed, 1 comment
| offenders.append(py_file.relative_to(SRC)) | ||
| assert not offenders, ( | ||
| "stdout writes found in: " + ", ".join(str(o) for o in offenders) | ||
| ) | ||
| continue |
There was a problem hiding this comment.
logic: Files with syntax errors are added to offenders list, but the test assertion message suggests they contain stdout writes rather than syntax issues
1 participant
Summary
Testing
pytest tests/test_logging_stdout.py tests/test_resources_api.py tests/test_script_editing.py tests/test_transport_framing.pyhttps://chatgpt.com/codex/tasks/task_e_68a2714614b08327afb864351f02a59f