fix: Remove 0.4.0 features from 0.3.x branch.#171
Conversation
…figuration (a2aproject#94)" This reverts commit b03fefc.
…ject (a2aproject#89)" This reverts commit 601bf0d.
The sut_client fixture was returning a cached transport client, so all
tests in a session shared the same GRPCClient instance. A channel left
in TRANSIENT_FAILURE by one test (e.g. test_tasks_resubscribe_nonexistent)
would cause subsequent tests to see UNAVAILABLE errors.
Three-part fix:
- Convert sut_client to a yield fixture that closes the client and evicts
it from TransportManager._client_cache on teardown, so each test gets
a fresh channel
- Rewrite GRPCClient.send_streaming_message and subscribe_to_task to use
synchronous gRPC in a daemon thread instead of grpc.aio, making close()
synchronous and eliminating event-loop binding issues across per-test loops
- Add explicit aclose() in finally blocks for all streaming async generators
in test_streaming_methods.py to avoid deferred cleanup at loop teardown
Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request updates the A2A protocol definition and TCK implementation, including renaming fields like 'parts' to 'content', removing the ListTasks RPC, and simplifying transport client signatures by removing the explicit configuration parameter. The review feedback highlights several critical issues where this simplification led to protocol violations or lost functionality. Specifically, configuration options are often ignored or incorrectly placed at the top level of request payloads instead of being nested. Additionally, the review points out missing parameter passing in method signatures, an inconsistent resource path in the proto file, and factually incorrect documentation regarding the history_length field.
| // fields are not present as they don't comply with AIP rules, and the | ||
| // optional history_length on the get task method is not present as it also | ||
| // violates AIP-127 and AIP-131. |
There was a problem hiding this comment.
The comment states that history_length is not present on the get task method, but GetTaskRequest (line 629) still includes this field. This documentation is factually incorrect and should be updated or removed.
There was a problem hiding this comment.
@gemini-code-assist You're correct that the comment is factually inaccurate - GetTaskRequest does indeed include the history_length field at line 629. However, this comment is copied verbatim from the official A2A v0.3.0
specification (https://github.com/a2aproject/A2A/blob/v0.3.0/specification/grpc/a2a.proto). Since the TCK's proto file should mirror the official specification exactly, we're keeping this comment as-is to maintain
consistency with the published spec. The discrepancy appears to be a documentation error in the upstream specification itself.
There was a problem hiding this comment.
I understand the need to maintain strict consistency with the official A2A v0.3.0 specification for the TCK's proto file. Thank you for clarifying that this is an upstream documentation error. Given that context, keeping the comment as-is is the correct approach to ensure the TCK remains a faithful mirror of the published specification.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the a2a.proto definition for AIP compliance, including field renames and the removal of the ListTasks method. The TCK transport clients were updated to use keyword arguments for configuration, and the gRPC client was redesigned to use a background thread for streaming to prevent event loop conflicts. Key feedback includes correcting a singular resource path in the proto, fixing a potential hang in gRPC stream cancellation, refining the logic for handling empty message parts, and improving encapsulation in the test configuration.
| returns (TaskPushNotificationConfig) { | ||
| option (google.api.http) = { | ||
| post: "/v1/{parent=tasks/*/pushNotificationConfigs}" | ||
| post: "/v1/{parent=task/*/pushNotificationConfigs}" |
There was a problem hiding this comment.
The resource path uses the singular task (i.e., {parent=task/*/pushNotificationConfigs}), which is inconsistent with the plural tasks used in all other methods (lines 52, 60, 71, 88, 96, 110) and the standard resource naming convention for this service. This will likely cause routing issues if the server expects the plural form.
| post: "/v1/{parent=task/*/pushNotificationConfigs}" | |
| post: "/v1/{parent=tasks/*/pushNotificationConfigs}" |
There was a problem hiding this comment.
@gemini-code-assist Same comment as here #171 (comment). Will not fix
There was a problem hiding this comment.
Understood. Since you have explicitly stated that you will not fix this, I will not pursue this further in this thread. Thank you for the clarification.
Also undoes the added PushNotificationConfig tests.
Better lifecycle handling of gRPC clients