test: add infra-independent unit tests for core agent protocols - #151
Open
STiFLeR7 wants to merge 4 commits into
Open
test: add infra-independent unit tests for core agent protocols#151STiFLeR7 wants to merge 4 commits into
STiFLeR7 wants to merge 4 commits into
Conversation
Author
|
This PR focuses on protocol and infrastructure correctness (A2A, scheduling, metrics). |
Paraschamoli
requested changes
Mar 23, 2026
Paraschamoli
left a comment
Collaborator
There was a problem hiding this comment.
This pr is great but it has merge conflicts because this PR has been open since mid-February, it is currently experiencing heavy merge conflicts, specifically in test_redis_scheduler.py. Could you please fetch the latest main and rebase?
Author
|
Sure will work on it and update it according to the new push history. |
STiFLeR7
force-pushed
the
test/improve-coverage-scheduler-protocol
branch
from
June 4, 2026 05:34
db4d3b9 to
83ddae2
Compare
Author
|
@Paraschamoli I have resolved the merge conflicts, updated all tests to match the new directory layout, improved Windows compatibility for the test suite, and rebased onto the latest upstream main. All 1114+ tests now pass cleanly. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request expands unit-test coverage for A2A endpoints, Redis scheduling, metrics error handling, platform-specific permissions, and explicit UTF-8 README reads. ChangesTest Coverage Expansion
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/unit/auth/test_hydra_registration.py (1)
79-82: 💤 Low valueConsider moving the
sysimport to module level.The platform check is correct and necessary for Windows compatibility. However, importing
syslocally within the test is unconventional. For consistency with the rest of the file and Python conventions, consider moving the import to the top of the file alongside the other imports (after line 7).♻️ Proposed refactor
At module level (after line 7):
from unittest.mock import AsyncMock, MagicMock, patch +import sys import pytestIn the test (lines 79-82):
- # Check permissions (owner read/write only) on non-Windows platforms - import sys + # Check permissions (owner read/write only) on non-Windows platforms if sys.platform != "win32": assert oct(creds_file.stat().st_mode)[-3:] == "600"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/auth/test_hydra_registration.py` around lines 79 - 82, Move the local import of sys to the module level: add "import sys" with the other imports near the top of tests/unit/auth/test_hydra_registration.py (after the existing imports around line 7), then remove the inline "import sys" inside the test so the platform check uses the top-level sys reference (the assertion using creds_file.stat().st_mode and sys.platform should remain unchanged).tests/unit/server/scheduler/test_redis_scheduler.py (1)
120-132: ⚡ Quick winAvoid
RuntimeError("StopLoop")as the generator stop signal.Using an unrelated exception to escape
receive_task_operations()makes these tests depend on the current exception-handling policy inside the loop. If that code is later hardened to catch generic exceptions, these tests will hang instead of failing cleanly. Prefer consuming a single item withanext(...)and then closing the async generator explicitly.♻️ Safer pattern
+import asyncio import json import uuid from unittest.mock import AsyncMock, MagicMock, patch @@ - mock_redis_client.blpop.side_effect = [ - ("bindu:tasks", json.dumps(task_data)), - RuntimeError("StopLoop") # Break the infinite loop for testing - ] - - # Consume generator - try: - async for operation in scheduler.receive_task_operations(): - assert operation["operation"] == "run" - assert operation["params"]["to"] == "agent-id" - break # Only verify first item - except RuntimeError: - pass # Expected from side_effect + mock_redis_client.blpop.side_effect = [ + ("bindu:tasks", json.dumps(task_data)), + ] + + operations = scheduler.receive_task_operations() + operation = await asyncio.wait_for(anext(operations), timeout=1) + assert operation["operation"] == "run" + assert operation["params"]["to"] == "agent-id" + await operations.aclose()Also applies to: 149-162, 183-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/server/scheduler/test_redis_scheduler.py` around lines 120 - 132, Replace the RuntimeError-based stop in the test that sets mock_redis_client.blpop.side_effect and instead consume a single item from the async generator returned by scheduler.receive_task_operations() using anext(...) (await anext(generator)) to get and assert the first operation, then explicitly close the generator with await generator.aclose(); remove the RuntimeError side_effect and the try/except that swallows it, and update the other similar test blocks (the ones using mock_redis_client.blpop.side_effect with RuntimeError) to follow the same anext + aclose pattern so the tests don't rely on throwing unrelated exceptions to stop the generator.tests/unit/server/middleware/test_metrics.py (1)
100-128: ⚡ Quick winStrengthen test assertions for completeness.
The test correctly verifies error resilience, but could be more complete by asserting:
increment_requests_in_flightwas called (matching the pattern intest_dispatch_decrements_on_errorat lines 77-98)record_http_requestwas called (proving the error path was actually exercised, not accidentally skipped)🧪 Suggested additions
# Should not raise exception response = await middleware.dispatch(mock_request, mock_call_next) assert response == mock_response + mock_metrics.increment_requests_in_flight.assert_called_once() + mock_metrics.record_http_request.assert_called_once() mock_metrics.decrement_requests_in_flight.assert_called_once() # cleanup always runs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/server/middleware/test_metrics.py` around lines 100 - 128, Add assertions to test_dispatch_ignores_recording_error to ensure the error path was exercised: after awaiting middleware.dispatch, assert mock_metrics.increment_requests_in_flight.assert_called_once() and mock_metrics.record_http_request.assert_called_once() (keeping the existing mock_metrics.decrement_requests_in_flight.assert_called_once()). This updates the test for MetricsMiddleware to verify both the increment and the attempted record call in addition to the cleanup decrement.tests/unit/server/endpoints/test_a2a_protocol.py (2)
54-62: ⚡ Quick winConsider adding explicit mock for
get_client_ip.The endpoint calls
get_client_ip(request)at its entry point. WhileMagicMockreturns a mock by default, explicitly mocking this dependency makes the test more readable and resilient to changes.🔧 Suggested enhancement
+ with patch("bindu.server.endpoints.a2a_protocol.get_client_ip", return_value="127.0.0.1"): mock_request = MagicMock(spec=Request) mock_request.body = AsyncMock(return_value=json.dumps(body).encode()) class MockState: pass mock_request.state = MockState() # Call endpoint response = await agent_run_endpoint(mock_app, mock_request)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/server/endpoints/test_a2a_protocol.py` around lines 54 - 62, The test should explicitly patch get_client_ip used by agent_run_endpoint to make the dependency clear and stable; update the test to mock get_client_ip (e.g., via monkeypatch or unittest.mock.patch) so it returns a deterministic IP and attach that to the request context before calling agent_run_endpoint, referencing the existing mock_request and the agent_run_endpoint function to locate where to inject the mock.
74-82: ⚡ Quick winEnhance test to validate JSON-RPC error structure.
The test currently only checks the HTTP status code. According to the PR objectives to verify JSON-RPC contract correctness, the test should also validate the error response structure includes the expected
codeandmessagefields fromJSONParseError.📋 Suggested enhancement
response = await agent_run_endpoint(mock_app, mock_request) assert response.status_code == 400 + content = json.loads(response.body) + assert "error" in content + assert content["error"]["code"] == -32700 # JSON Parse Error + assert "message" in content["error"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/server/endpoints/test_a2a_protocol.py` around lines 74 - 82, Update the test_invalid_json test to assert the JSON-RPC error payload as well as the HTTP status: after calling agent_run_endpoint(mock_app, mock_request) parse the response JSON and assert it contains an "error" object with "code" and "message" fields that match the JSONParseError contract (use the JSONParseError.code and JSONParseError.message symbols or their constants from the JSON-RPC errors module), and keep the existing assert that response.status_code == 400; locate this logic in the test_invalid_json function and add the JSON body assertions right after the agent_run_endpoint call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/server/endpoints/test_a2a_protocol.py`:
- Line 26: The mock handler return value is not a complete JSON-RPC 2.0
response—update the AsyncMock assigned to app.task_manager.mock_handler so its
return_value includes the full JSON-RPC structure (include "jsonrpc": "2.0",
"result": "success", and an "id" field) and then adjust the test assertions that
inspect the response (the assertions referencing content/result) to verify
content["jsonrpc"] == "2.0", content["result"] == "success", and that an "id"
key is present; use the existing AsyncMock on app.task_manager.mock_handler and
the existing response assertions to locate where to change the mock and add the
new checks.
- Around line 85-101: The test test_unsupported_method is asserting a 400
because agent_run_endpoint first calls a2a_request_ta.validate_json (A2ARequest
is a discriminated union on "method") so an unknown "method" literal triggers
JSONParseError → 400; update the test name/comment to reflect it validates
schema/400 rather than handler-not-found/404, and add a new test that sends a
valid method literal (one of A2ARequest's method tags) but ensures that method
is not present in app_settings.agent.method_handlers to exercise the
MethodNotFoundError branch and assert a 404 from agent_run_endpoint.
In `@tests/unit/server/scheduler/test_redis_scheduler.py`:
- Around line 67-75: The test only asserts "span_id" exists in the serialized
payload but misses "trace_id", so update the assertions in the push-path test
(the block that loads payload = json.loads(args[1])) to also assert that
"trace_id" is present (e.g., assert "trace_id" in payload) and do the same
addition for the second push-block around lines 87-102; keep checks for
payload["operation"], payload["params"], and span_id as-is and simply add the
trace_id presence assertion to lock down the serialized tracing contract.
---
Nitpick comments:
In `@tests/unit/auth/test_hydra_registration.py`:
- Around line 79-82: Move the local import of sys to the module level: add
"import sys" with the other imports near the top of
tests/unit/auth/test_hydra_registration.py (after the existing imports around
line 7), then remove the inline "import sys" inside the test so the platform
check uses the top-level sys reference (the assertion using
creds_file.stat().st_mode and sys.platform should remain unchanged).
In `@tests/unit/server/endpoints/test_a2a_protocol.py`:
- Around line 54-62: The test should explicitly patch get_client_ip used by
agent_run_endpoint to make the dependency clear and stable; update the test to
mock get_client_ip (e.g., via monkeypatch or unittest.mock.patch) so it returns
a deterministic IP and attach that to the request context before calling
agent_run_endpoint, referencing the existing mock_request and the
agent_run_endpoint function to locate where to inject the mock.
- Around line 74-82: Update the test_invalid_json test to assert the JSON-RPC
error payload as well as the HTTP status: after calling
agent_run_endpoint(mock_app, mock_request) parse the response JSON and assert it
contains an "error" object with "code" and "message" fields that match the
JSONParseError contract (use the JSONParseError.code and JSONParseError.message
symbols or their constants from the JSON-RPC errors module), and keep the
existing assert that response.status_code == 400; locate this logic in the
test_invalid_json function and add the JSON body assertions right after the
agent_run_endpoint call.
In `@tests/unit/server/middleware/test_metrics.py`:
- Around line 100-128: Add assertions to test_dispatch_ignores_recording_error
to ensure the error path was exercised: after awaiting middleware.dispatch,
assert mock_metrics.increment_requests_in_flight.assert_called_once() and
mock_metrics.record_http_request.assert_called_once() (keeping the existing
mock_metrics.decrement_requests_in_flight.assert_called_once()). This updates
the test for MetricsMiddleware to verify both the increment and the attempted
record call in addition to the cleanup decrement.
In `@tests/unit/server/scheduler/test_redis_scheduler.py`:
- Around line 120-132: Replace the RuntimeError-based stop in the test that sets
mock_redis_client.blpop.side_effect and instead consume a single item from the
async generator returned by scheduler.receive_task_operations() using anext(...)
(await anext(generator)) to get and assert the first operation, then explicitly
close the generator with await generator.aclose(); remove the RuntimeError
side_effect and the try/except that swallows it, and update the other similar
test blocks (the ones using mock_redis_client.blpop.side_effect with
RuntimeError) to follow the same anext + aclose pattern so the tests don't rely
on throwing unrelated exceptions to stop the generator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6edc6d70-70c3-4654-81a8-0e925cc072ec
📒 Files selected for processing (5)
tests/unit/auth/test_hydra_registration.pytests/unit/server/endpoints/test_a2a_protocol.pytests/unit/server/middleware/test_metrics.pytests/unit/server/scheduler/test_redis_scheduler.pytests/unit/test_minimax_example.py
STiFLeR7
force-pushed
the
test/improve-coverage-scheduler-protocol
branch
from
June 26, 2026 06:10
83ddae2 to
e7c866a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/server/endpoints/test_a2a_protocol.py`:
- Around line 75-83: The A2A endpoint tests only assert HTTP 400 and do not
verify the JSON-RPC payload, so tighten the contract checks in test_invalid_json
and the invalid method-tag validation test by parsing response.body and
asserting the expected error object shape, including the correct protocol-level
error.code and message. Update the assertions around agent_run_endpoint to
validate both the transport status and the JSON-RPC error response for invalid
JSON and invalid method-tag failures.
In `@tests/unit/server/scheduler/test_redis_scheduler.py`:
- Around line 126-132: The loop tests are catching any RuntimeError from
receive_task_operations(), which can hide real failures. Update the test helpers
around receive_task_operations() to catch only the injected sentinel stop signal
(for example the synthetic "StopLoop" exception from the side_effect) and let
unexpected RuntimeError instances fail the test. Apply the same change in the
affected test blocks so the assertions still verify the first yielded operation
without masking regressions.
- Around line 27-33: The scheduler fixture only patches redis.asyncio.from_url
during RedisScheduler construction, but RedisScheduler.__aenter__ creates the
Redis client later, so tests can still use a real Redis connection. Update the
scheduler fixture in test_redis_scheduler.py to keep the patch active for the
async context manager execution, or assign the mocked client after entering the
context, so the mocked from_url applies when RedisScheduler.__aenter__ runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7405573b-8f21-454a-9c5f-4ec399e3ac3e
📒 Files selected for processing (5)
tests/unit/auth/test_hydra_registration.pytests/unit/server/endpoints/test_a2a_protocol.pytests/unit/server/middleware/test_metrics.pytests/unit/server/scheduler/test_redis_scheduler.pytests/unit/test_minimax_example.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/test_minimax_example.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/auth/test_hydra_registration.py
- tests/unit/server/middleware/test_metrics.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/server/endpoints/test_a2a_protocol.py`:
- Around line 25-30: The success-path A2A protocol test is only checking that an
`id` exists, so it can miss a wrong JSON-RPC correlation id. Update the mock on
`app.task_manager.mock_handler` to return the incoming request `id` instead of a
hardcoded value, and change the assertion in the test to compare `content["id"]`
directly against `body["id"]`. Apply the same fix to the duplicate success-path
case referenced by the review.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75fdab12-5967-49f4-bf61-bed039725db9
📒 Files selected for processing (4)
tests/unit/auth/test_hydra_registration.pytests/unit/server/endpoints/test_a2a_protocol.pytests/unit/server/middleware/test_metrics.pytests/unit/server/scheduler/test_redis_scheduler.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/server/middleware/test_metrics.py
- tests/unit/auth/test_hydra_registration.py
- tests/unit/server/scheduler/test_redis_scheduler.py
The success-path contract test hardcoded the mock handler's response id to "123" and only asserted an id key was present, so it would pass even if the endpoint returned a wrong JSON-RPC correlation id. Echo the request id from the mock and assert content["id"] == body["id"] so the test actually exercises id correlation. Addresses CodeRabbit review on GetBindu#151.
Author
|
Addressed the CodeRabbit finding on the success-path test in c1816f3: the mock handler now echoes the request's JSON-RPC |
|
Hey! I'm building an x402 MCP server that wraps agent tools as pay-per-call (USDC on Base). Was checking getbindu/bindu — interesting approach to agent infrastructure. Would love to connect if you're thinking about agent monetization models.
|
Author
|
@Paraschamoli just checking in on this one. The rebase you asked for back in March is done (merged current main, updated tests for the new directory layout, fixed Windows compatibility), and the CodeRabbit finding from the last review round was addressed in c1816f3. mergeable=MERGEABLE now. The CI workflow run is sitting at action_required, which looks like it needs a maintainer to approve the run for a fork PR. Let me know if there's anything else you'd like changed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds fast, infrastructure-independent unit tests covering Bindu’s core agent communication and scheduling primitives. The goal is to make protocol-level correctness easy to validate locally and in CI, without requiring live Redis, Web3, or native crypto builds.
What’s Covered
A2A Protocol
Redis Scheduler
Metrics Middleware
Test Infrastructure Improvements
Why This Matters
This PR intentionally focuses on infrastructure correctness rather than agent behavior, laying groundwork for higher-level agent workflows.
Summary by CodeRabbit