-
Notifications
You must be signed in to change notification settings - Fork 104
test: improve TaskManager test coverage #181
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
test: improve TaskManager test coverage #181
Conversation
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.
Summary of Changes
Hello @misselvexu, 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 focuses on substantially improving the unit test coverage for the TaskManager class. The changes introduce new test cases that validate critical functionalities, including the nuanced behavior of artifact updates, strict task ID validation during event processing, and the correct handling of initial messages and multiple artifacts. These additions aim to enhance the reliability and stability of the TaskManager by covering previously untested scenarios.
Highlights
- Test Coverage Expansion: Added a significant number of new test cases to
TaskManagerTest.javato improve the overall test coverage and validate various behaviors of theTaskManagerclass. - Artifact Update Event Scenarios: Introduced comprehensive tests for
TaskArtifactUpdateEvent, covering scenarios such as appending content to existing artifacts, handling attempts to append to non-existent artifacts, and verifying replacement behavior when theappendflag isfalseornull. - Task ID Mismatch Validation: Added tests to ensure that
TaskManagercorrectly throws anA2AServerExceptionwhen an attempt is made to save a task event (e.g., a newTask,TaskStatusUpdateEvent, orTaskArtifactUpdateEvent) with ataskIdthat does not match theTaskManager's initializedtaskId. - Initial Message Integration: Included tests to verify the logic for integrating an
initialMessageinto a new task's history. These tests confirm that theinitialMessageis used when no message is provided in the task event, and conversely, that it is not used when a message is explicitly present in the task. - Multiple Artifact Management: Developed tests to validate how
TaskManagerhandles multiple artifacts. This includes confirming that artifacts with the sameartifactIdare correctly replaced, while artifacts with distinctartifactIdsare properly added and maintained within the task.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 enhances the test coverage for the TaskManager class with a comprehensive suite of new tests, covering various scenarios for artifact and status updates. The tests are well-structured and effectively validate the core logic.
sdk-server-common/src/test/java/io/a2a/server/tasks/TaskManagerTest.java
Outdated
Show resolved
Hide resolved
sdk-server-common/src/test/java/io/a2a/server/tasks/TaskManagerTest.java
Outdated
Show resolved
Hide resolved
sdk-server-common/src/test/java/io/a2a/server/tasks/TaskManagerTest.java
Outdated
Show resolved
Hide resolved
kabir
left a comment
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.
Thank you @misselvexu !
I have a few nitpicks and a question for something I am not sure about.
I see that TaskManager is missing some log statements which appear in the Python information. I will add those in a separate PR
sdk-server-common/src/test/java/io/a2a/server/tasks/TaskManagerTest.java
Outdated
Show resolved
Hide resolved
| Task retrieved = taskManagerWithInitialMessage.getTask(); | ||
|
|
||
| // Check that the task does not have the initial message in its history | ||
| assertNull(retrieved.getHistory()); |
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.
Here I am a bit unsure. Should the taskMessage that was part of taskWithMessage show up in the Task's history?
If I am getting something wrong, feel free to educate me.
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.
Here I am a bit unsure. Should the
taskMessagethat was part oftaskWithMessageshow up in the Task's history? If I am getting something wrong, feel free to educate me.
Based on my detailed analysis of the code, I can clearly answer your question:
Answer: No, the taskMessage should NOT appear in the Task's history
Reasoning:
-
When messages move to history:
- According to lines 67-71 in TaskManager, a message only moves to history when there's an existing task with a message in its status
- The message movement happens during
TaskStatusUpdateEventprocessing, moving the old status message to history
-
Current test execution flow:
1. Create new TaskManager (no existing task) 2. Send TaskStatusUpdateEvent with taskMessage 3. ensureTask() finds no existing task, calls createTask() 4. createTask() creates new task with history containing only initialMessage 5. Set new status (containing taskMessage) 6. Since there's no "old status message", taskMessage doesn't move to history -
Actual logic flow:
- First status update:
taskMessagebecomes current status, history containsinitialMessage - Second status update:
taskMessagemoves to history, new message becomes current status
- First status update:
I readjusted the code for the test:
// History should only contain initialMessage (1 message)
assertEquals(1, retrieved.getHistory().size());
assertEquals("initial message", ((TextPart) retrieved.getHistory().get(0).getParts().get(0)).getText());
// Current status should contain taskMessage
assertEquals("task message", ((TextPart) retrieved.getStatus().message().getParts().get(0)).getText());Summary: The taskMessage should not appear in history during the first status update.
sdk-server-common/src/test/java/io/a2a/server/tasks/TaskManagerTest.java
Outdated
Show resolved
Hide resolved
# Description I noticed while reviewing #181 and looking at the Python implementation that we don't have logging in the TaskManager. We decided on the logging strategy after the initial code was written. This PR addresses this omission.
…rTest.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…rTest.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
78d174e to
5ca10cc
Compare
|
Hi @misselvexu I have rebased on upstream/main and pushed the branch to your remote again (I have admin rights). If CI passes, I'll merge it |
# Description I noticed while reviewing a2aproject#181 and looking at the Python implementation that we don't have logging in the TaskManager. We decided on the logging strategy after the initial code was written. This PR addresses this omission.
# Description Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [x] Follow the [`CONTRIBUTING` Guide](../CONTRIBUTING.md). - [x] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [x] Ensure the tests pass - [x] Appropriate READMEs were updated (if necessary) Fixes a2aproject#46 🦕 --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.Fixes #46 🦕