-
Notifications
You must be signed in to change notification settings - Fork 104
test: add ToProto and FromProto tests #341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @Akanksha928, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Hi, I’ve added three initial tests that match the Python equivalent. Could you please confirm if this direction looks good before I add a few more? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds unit tests to improve coverage for ProtoUtils, which is a good initiative. My review focuses on improving the correctness and robustness of these new tests. I've identified a test that will fail due to an incorrect exception assertion, a test that validates potentially undesirable lenient behavior, and another test that could be strengthened with more specific assertions. Please review my comments for details.
| io.a2a.spec.InvalidParamsError exception = assertThrows( | ||
| io.a2a.spec.InvalidParamsError.class, | ||
| () -> ProtoUtils.FromProto.message(invalidMessage) | ||
| ); | ||
|
|
||
| assertEquals("Invalid parameters", exception.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test asserts that an InvalidParamsError is thrown for an unsupported part type. However, the current implementation of ProtoUtils.FromProto.part() throws an InvalidRequestError. This discrepancy will cause the test to fail.
To align with the existing implementation, the test should expect an InvalidRequestError. If the long-term goal is to throw InvalidParamsError (which could be more semantically accurate), ProtoUtils.FromProto.part() should be updated, and that change should be part of this PR to ensure tests and code are in sync.
| io.a2a.spec.InvalidParamsError exception = assertThrows( | |
| io.a2a.spec.InvalidParamsError.class, | |
| () -> ProtoUtils.FromProto.message(invalidMessage) | |
| ); | |
| assertEquals("Invalid parameters", exception.getMessage()); | |
| io.a2a.spec.InvalidRequestError exception = assertThrows( | |
| io.a2a.spec.InvalidRequestError.class, | |
| () -> ProtoUtils.FromProto.message(invalidMessage) | |
| ); | |
| assertEquals("Invalid request", exception.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can ignore this Gemini message, I think you're correct that an InvalidParamsError will occur.
| public void testTaskQueryParamsInvalidName() { | ||
| io.a2a.grpc.GetTaskRequest request = io.a2a.grpc.GetTaskRequest.newBuilder() | ||
| .setName("invalid-name-format") | ||
| .build(); | ||
|
|
||
| var result = ProtoUtils.FromProto.taskQueryParams(request); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals("invalid-name-format", result.id()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test validates that an invalid task name format is handled by using the entire string as the task ID. This behavior is too lenient and might hide bugs from malformed task names, as the name field is expected to follow the format tasks/{id}.
For better robustness, taskQueryParams() should validate the name format and throw an IllegalArgumentException if it's invalid. This would be consistent with how other methods in ProtoUtils, like getTaskPushNotificationConfigParams, handle malformed names.
Consider updating this test to assert that an IllegalArgumentException is thrown for an invalid name format, which would encourage a more robust implementation in taskQueryParams().
| assertNotNull(result); | ||
| assertEquals(1, result.getContentCount()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current assertion only checks that the number of parts is correct. To make this test more robust, you should also assert that the converted Part is empty, as it's an unsupported type. This ensures that an empty part is correctly generated and we are not accidentally creating a Part with data.
assertNotNull(result);
assertEquals(1, result.getContentCount());
assertEquals(io.a2a.grpc.Part.PartCase.PART_NOT_SET, result.getContent(0).getPartCase());| .setName("invalid-name-format") | ||
| .build(); | ||
|
|
||
| var result = ProtoUtils.FromProto.taskQueryParams(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the python test, it looks like it's expecting an error to occur in this case. If we're not currently getting an error, that likely means we should update the taskQueryParams method to make sure the name is in the right format.
| .build(); | ||
| io.a2a.grpc.Message result = ProtoUtils.ToProto.message(message); | ||
|
|
||
| assertNotNull(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the Python test, they are expecting an error to occur in this case. If we're not hitting an error here, that means that we likely need to update ToProto so that we get an error in this case.
|
@Akanksha928 It looks like you're on the right track! I've added some comments. Feel free to let us know if you have more questions. Thanks! |
|
I wrongly merged that so I reverted @Akanksha928 May you please fix the comments from @fjuma and re-send a PR. Sorry for this |
# Description This PR adds new unit tests to improve coverage for `ProtoUtils.ToProto` and `ProtoUtils.FromProto`. ### Tests Added - `testFromProtoPartUnsupportedType`: verifies invalid proto parts throw `InvalidParamsError`. - `testTaskQueryParamsInvalidName`: makes sure invalid task name formats are handled gracefully. - `testToProtoPartUnsupportedType`: checks handling of unsupported `Part` types during conversion. Fixes a2aproject#304
Description
This PR adds new unit tests to improve coverage for
ProtoUtils.ToProtoandProtoUtils.FromProto.Tests Added
testFromProtoPartUnsupportedType: verifies invalid proto parts throwInvalidParamsError.testTaskQueryParamsInvalidName: makes sure invalid task name formats are handled gracefully.testToProtoPartUnsupportedType: checks handling of unsupportedParttypes during conversion.Fixes #304