🧪 Add tests for transport conditional branches in main.py - #242
Conversation
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdds async unit tests to verify that the main.run() entry point correctly delegates to the appropriate transport-specific server functions for stdio and streamable-http, including argument propagation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The two new tests are nearly identical aside from argument values; consider parametrizing them with
pytest.mark.parametrizeto reduce duplication and make it easier to add more transports in the future. - To make the intent clearer and avoid drift if
run’s signature changes, you could build a sharedkwargsdict for the call and the assertion in each test rather than repeating the same keyword arguments twice.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The two new tests are nearly identical aside from argument values; consider parametrizing them with `pytest.mark.parametrize` to reduce duplication and make it easier to add more transports in the future.
- To make the intent clearer and avoid drift if `run`’s signature changes, you could build a shared `kwargs` dict for the call and the assertion in each test rather than repeating the same keyword arguments twice.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adds missing unit coverage for codeweaver.main.run()’s transport branching to ensure stdio vs streamable-http correctly delegates to the appropriate internal runner.
Changes:
- Added an async unit test verifying
transport="stdio"delegates to_run_stdio_serverwith the expected args. - Added an async unit test verifying
transport="streamable-http"delegates to_run_http_serverwith the expected args.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @pytest.mark.asyncio | ||
| @patch("codeweaver.main._run_http_server") | ||
| async def test_run_http_transport(mock_run_http): |
| mock_run_stdio.assert_called_once_with( | ||
| config_file=None, | ||
| project_path=None, | ||
| host="127.0.0.1", | ||
| port=8080, | ||
| verbose=True, | ||
| debug=False, | ||
| ) |
| mock_run_http.assert_called_once_with( | ||
| config_file=None, | ||
| project_path=None, | ||
| host="0.0.0.0", | ||
| port=9090, | ||
| verbose=False, | ||
| debug=True, | ||
| ) |
| @pytest.mark.asyncio | ||
| @patch("codeweaver.main._run_stdio_server") | ||
| async def test_run_stdio_transport(mock_run_stdio): | ||
| """Test that run() with transport='stdio' delegates to _run_stdio_server.""" |
PR Review - Test Coverage for Transport Conditional BranchesI've reviewed this PR and the code changes look good overall. Here's my detailed analysis: Code Quality ✅Strengths:
Implementation Details:
Regarding Comment #2944228606The Copilot comment suggests adding return type annotations ( Looking at ruff check tests/unit/test_main.py --select ANN201
# Result: All checks passed!The test file matches the CI Failures Analysis 🔍The PR has some failing CI checks, but they appear unrelated to the code changes:
The lint check passed successfully, which confirms the code meets the project's style requirements. RecommendationThe code changes are good and ready to merge once CI issues are resolved. The test implementation is solid and follows the existing test patterns in the codebase. No changes to return type annotations are needed. The CI failures appear to be environmental/infrastructure issues rather than code quality problems. I recommend:
The test logic itself is sound and improves the codebase's test coverage as intended. Review conducted by Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Detailed Code Review
I've thoroughly reviewed the test implementation and it looks excellent. Here are my findings:
Test Implementation Analysis
test_run_stdio_transport (lines 55-78)
- ✅ Correctly mocks
_run_stdio_server - ✅ Verifies all 6 parameters are passed correctly
- ✅ Uses realistic test values (127.0.0.1:8080, verbose=True, debug=False)
test_run_http_transport (lines 81-104)
- ✅ Correctly mocks
_run_http_server - ✅ Verifies all 6 parameters are passed correctly
- ✅ Uses different test values to ensure proper routing (0.0.0.0:9090, verbose=False, debug=True)
Code Alignment with Existing Patterns
The tests follow the same patterns used in the existing test functions above them (lines 12-53):
- Consistent use of
@patchdecorators - Same docstring style
- Similar assertion patterns with
assert_called_once_with
Security & Performance
No security or performance concerns. The tests are pure unit tests that mock external dependencies appropriately.
Addressing the Copilot Comment
Regarding the suggestion to add -> None return type annotations: This is not necessary. The ruff.toml configuration at lines 179-197 explicitly exempts tests/*.py from all ANN rules. I verified this with a direct lint check that passed.
Conclusion
Approve ✅ - The code is well-written, follows project conventions, and successfully adds the intended test coverage for the transport conditional logic in main.py:327-344. No changes needed.
|
Re: Comment about ANN201 return type annotations The suggestion to add
The test file |
There was a problem hiding this comment.
Code Review
Thank you for adding test coverage for the transport conditional branches! This is valuable coverage for the main entry point. However, I've identified a few issues that need to be addressed:
🔴 Critical: Async Mock Issue
The tests are using @patch which creates synchronous MagicMock objects by default, but _run_stdio_server and _run_http_server are async functions. You should use AsyncMock instead.
Current code (tests/unit/test_main.py:56-57, 82-83):
@pytest.mark.asyncio
@patch("codeweaver.main._run_stdio_server")
async def test_run_stdio_transport(mock_run_stdio):Suggested fix:
from unittest.mock import AsyncMock, patch
@pytest.mark.asyncio
@patch("codeweaver.main._run_stdio_server", new_callable=AsyncMock)
async def test_run_stdio_transport(mock_run_stdio):Apply the same fix to test_run_http_transport at line 82-83.
This pattern is used elsewhere in the codebase - see tests/unit/cli/test_start_command.py for examples.
🟡 Observation: Unused Parameter in Implementation
While reviewing the tests, I noticed that _run_http_server has a port parameter (line 109 in main.py) that is never used in the function body:
- The function uses
mcp_portfor the MCP server (line 153) - The function uses
mgmt_portfrom settings for the management server (line 182) - The
portparameter is completely ignored
In src/codeweaver/main.py:103-114:
async def _run_http_server(
*,
config_file: FilePath | None = None,
project_path: Path | None = None,
host: str = LOCALHOST,
mcp_port: int = DEFAULT_MCP_PORT,
port: int = DEFAULT_MANAGEMENT_PORT, # ← This parameter is never used\!
verbose: bool = False,
debug: bool = False,
# ...
)This suggests either:
- The parameter should be removed as dead code, OR
- There's a bug where the parameter should be used but isn't
This is a separate issue from the tests themselves, but the tests would pass even with this unused parameter, which might not be ideal.
✅ What the tests do well:
- Clear test names and comprehensive docstrings
- Good coverage of both transport modes (stdio and streamable-http)
- Follows the existing test patterns in the file
- Validates that parameters are passed through correctly
📝 Recommendation:
- Fix the AsyncMock issue (required for tests to properly validate async behavior)
- Consider filing a separate issue about the unused
portparameter in_run_http_serverto clean up the implementation
Once the AsyncMock changes are made, these tests will provide solid coverage for the transport routing logic!
There was a problem hiding this comment.
Code Review: Transport Branch Tests
Summary
This PR adds unit tests for the transport conditional branches in codeweaver.main.run(), improving test coverage for the main entry point logic. The new tests verify that the transport parameter correctly delegates to either _run_stdio_server or _run_http_server.
Test Quality: ✅ Excellent
Strengths:
- Clear test structure: Both tests follow a consistent pattern with descriptive names and docstrings
- Proper mocking: Uses
@patchdecorators to mock the internal server functions, avoiding actual server startup - Comprehensive parameter validation: Verifies all parameters are passed correctly from
run()to the internal handlers - Async handling: Correctly uses
@pytest.mark.asynciofor async test functions - Good test data: Uses different port numbers and settings between the two tests to ensure proper delegation
Code Style: ✅ Compliant
- Follows existing test patterns in
test_main.py - Proper imports and formatting
- Consistent with the codebase's testing conventions
Coverage Impact: ✅ Positive
The tests cover the previously untested conditional branch at main.py:327-344, which is the primary routing logic for transport selection. This is valuable regression protection.
CI Status: ⚠️ Note on Failures
The new tests themselves PASS in all Python versions (3.12, 3.13, 3.13t, 3.14, 3.14t). The CI failures are in unrelated integration/e2e tests:
tests/e2e/test_user_journeys.pytests/integration/real/test_full_pipeline.pytests/integration/real/test_search_behavior.py
These failures appear to be pre-existing flaky tests unrelated to this PR's changes. The PR only modifies tests/unit/test_main.py and doesn't touch any production code.
Potential Issue: Parameter Mapping (Informational)
While reviewing, I noticed a potential bug in the existing implementation (not introduced by this PR):
In main.py:341, when calling _run_http_server(), the code passes:
await _run_http_server(
...
port=port, # This maps to management port
...
)However, _run_http_server() has TWO port parameters:
mcp_port: int = DEFAULT_MCP_PORT(9328) - for the MCP serverport: int = DEFAULT_MANAGEMENT_PORT(9329) - for the management server
The run() function's port parameter defaults to DEFAULT_MCP_PORT but gets passed as the management port. This might cause unexpected behavior when users specify a custom port. Consider opening a separate issue to clarify the port parameter semantics for HTTP transport mode.
Recommendation: ✅ Approve with Note
The tests are well-written and provide valuable coverage. The CI failures are unrelated to this PR. The parameter mapping issue I identified exists in the production code but is not introduced by this PR - the tests correctly validate the current implementation.
Suggested next steps:
- Merge this PR (tests are solid)
- Investigate/fix the flaky integration tests separately
- Consider opening an issue about the port parameter semantics in HTTP mode
|
jules please address the below PR review issue
|
Thank you for the review! I have updated the Also, I acknowledge the observation regarding the unused |
Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
🎯 What: The testing gap addressed
The
runfunction insrc/codeweaver/main.pyhandles the main entry point logic to start the CodeWeaver server, but thetransportconditional branch (deciding betweenstdioproxy vsstreamable-httpweb server logic) lacked explicit unit tests. This patch adds those missing tests to ensure the configuration parameters correctly delegate to their respective inner handlers (_run_stdio_serverand_run_http_server).📊 Coverage: What scenarios are now tested
transport='stdio': Verifies that when the server is started with the stdio transport, it delegates execution to the_run_stdio_serverhandler, correctly passing down parameters likeconfig_file,project_path,host,port,verbose, anddebug.transport='streamable-http': Verifies that when the server is started with the HTTP transport, it delegates execution to the_run_http_serverhandler, again checking that all relevant configuration options are passed correctly.✨ Result: The improvement in test coverage
The codebase now benefits from increased test reliability in its primary initialization flow. By asserting that the
transportargument dictates the correct initialization branch, it serves as a safety net against regressions if new parameters are introduced to these internal handlers. It prevents silent failures that could otherwise occur if transport options are accidentally misrouted.PR created automatically by Jules for task 12817532639525235297 started by Adam Poulemanos (@bashandbone)
Summary by Sourcery
Add coverage for the main entrypoint's transport-based delegation logic.
Tests: