test(job): cover job tool validation and state transitions#681
test(job): cover job tool validation and state transitions#681Protocol-zero-0 wants to merge 1 commit intonearai:mainfrom
Conversation
Add focused coverage for create/list/status/cancel job tools so validation errors, summary formatting, and cancellation behavior stay stable. This locks in the current user-facing responses for running and completed jobs without changing production code. Made-with: Cursor
github-actions
bot
added
scope: tool/builtin
Summary of ChangesHello, 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 robustness of the job tool by introducing comprehensive test coverage for previously implicit behaviors. The added tests explicitly validate job creation parameters, ensure correct formatting and user-specific scoping of job lists, and solidify the lifecycle management of jobs, particularly around state transitions and cancellation logic. This effort aims to reduce regression risks during future refactoring by making these critical user-facing interactions explicit and thoroughly tested. Highlights
Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds valuable test coverage for the job management tools, specifically focusing on parameter validation, job listing formats, status transitions, and cancellation behavior. The new tests help solidify the existing behavior and prevent future regressions.
I've found one critical issue in the test_create_job_params test where the use of unwrap_err() after borrowing the Result with is_err() will cause a compilation failure. I've provided a code suggestion to fix this by using the more idiomatic expect_err() method, which aligns with our guidelines for clear test failures. The other tests appear to be well-structured and correctly verify the intended tool behaviors.
| let missing_title = tool | ||
| .execute(serde_json::json!({ "description": "A test job" }), &ctx) | ||
| .await; | ||
| assert!(missing_title.is_err()); | ||
| assert!( | ||
| missing_title | ||
| .unwrap_err() | ||
| .to_string() | ||
| .contains("missing 'title' parameter") | ||
| ); | ||
|
|
||
| let missing_description = tool | ||
| .execute(serde_json::json!({ "title": "Test Job" }), &ctx) | ||
| .await; | ||
| assert!(missing_description.is_err()); | ||
| assert!( | ||
| missing_description | ||
| .unwrap_err() | ||
| .to_string() | ||
| .contains("missing 'description' parameter") | ||
| ); |
There was a problem hiding this comment.
This test contains a pattern that will fail to compile. The unwrap_err() method consumes the Result, so it cannot be used after is_err() has already borrowed it. A more idiomatic and robust way to write this test is to use expect_err(), which asserts that the result is an Err and returns the error value, making the test cleaner and more direct. This approach aligns with the principle of using expect() in tests to explicitly fail with clear messages, as outlined in our repository's code review guidelines.
| let missing_title = tool | |
| .execute(serde_json::json!({ "description": "A test job" }), &ctx) | |
| .await; | |
| assert!(missing_title.is_err()); | |
| assert!( | |
| missing_title | |
| .unwrap_err() | |
| .to_string() | |
| .contains("missing 'title' parameter") | |
| ); | |
| let missing_description = tool | |
| .execute(serde_json::json!({ "title": "Test Job" }), &ctx) | |
| .await; | |
| assert!(missing_description.is_err()); | |
| assert!( | |
| missing_description | |
| .unwrap_err() | |
| .to_string() | |
| .contains("missing 'description' parameter") | |
| ); | |
| let missing_title_err = tool | |
| .execute(serde_json::json!({ "description": "A test job" }), &ctx) | |
| .await | |
| .expect_err("should fail without title"); | |
| assert!( | |
| missing_title_err | |
| .to_string() | |
| .contains("missing 'title' parameter") | |
| ); | |
| let missing_description_err = tool | |
| .execute(serde_json::json!({ "title": "Test Job" }), &ctx) | |
| .await | |
| .expect_err("should fail without description"); | |
| assert!( | |
| missing_description_err | |
| .to_string() | |
| .contains("missing 'description' parameter") | |
| ); |
References
- In tests, when setting up a state that depends on environmental factors (e.g., system uptime for time calculations), prefer
expect()to explicitly fail the test with a clear message if the setup is not possible. Avoid fallbacks likeunwrap_or()that could cause the test to silently check the wrong logic.
There was a problem hiding this comment.
Pull request overview
Adds additional unit tests in the built-in job tool module to explicitly cover parameter validation failures, list output formatting/summarization, job state transitions, and cancellation behavior.
Changes:
- Add tests for
create_jobparameter validation error paths. - Add tests for
list_jobsformatting + per-user scoping and summary counts. - Add tests for
job_statustransition timestamps andcancel_jobbehavior across running vs. completed jobs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert!(jobs.iter().any(|job| { | ||
| job.get("job_id").and_then(|v| v.as_str()) == Some(&pending_id.to_string()) | ||
| && job.get("status").and_then(|v| v.as_str()) == Some("Pending") | ||
| })); | ||
| assert!(jobs.iter().any(|job| { | ||
| job.get("job_id").and_then(|v| v.as_str()) == Some(&completed_id.to_string()) | ||
| && job.get("status").and_then(|v| v.as_str()) == Some("Completed") | ||
| })); | ||
| assert!(jobs.iter().any(|job| { | ||
| job.get("job_id").and_then(|v| v.as_str()) == Some(&failed_id.to_string()) | ||
| && job.get("status").and_then(|v| v.as_str()) == Some("Failed") | ||
| })); |
There was a problem hiding this comment.
These assertions expect status values like "Pending"/"Completed"/"Failed", but the codebase generally serializes JobState as snake_case via JobState::to_string() (e.g., the web jobs API returns state: ctx.state.to_string()). Locking TitleCase here will cement an inconsistent API across job surfaces (create_job/cancel_job already return lowercase). Consider updating ListJobsTool to emit status: ctx.state.to_string() and adjust this test to expect "pending"/"completed"/"failed" accordingly.
|
|
||
| assert_eq!( | ||
| result.result.get("status").and_then(|v| v.as_str()), | ||
| Some("Completed") |
There was a problem hiding this comment.
This test expects job_status.status to be "Completed" (Debug formatting), but other parts of the codebase (and JobState's Display) use snake_case ("completed"). To keep the tool API consistent with create_job/cancel_job and the web jobs endpoints, consider changing JobStatusTool to use job_ctx.state.to_string() and update this assertion to expect "completed".
| Some("Completed") | |
| Some("completed") |
Summary
Why
The job tool surface has useful coverage already, but several user-facing paths still relied on implicit behavior. These tests make the small but important responses around validation and job lifecycle changes explicit, which lowers regression risk for future refactors.
Tests
cargo test --lib test_create_job_paramstarget/debug/deps/ironclaw-6fd0143b37beb5d3 --exact tools::builtin::job::tests::test_list_jobs_formattingtarget/debug/deps/ironclaw-6fd0143b37beb5d3 --exact tools::builtin::job::tests::test_job_status_transitionstarget/debug/deps/ironclaw-6fd0143b37beb5d3 --exact tools::builtin::job::tests::test_cancel_job_runningtarget/debug/deps/ironclaw-6fd0143b37beb5d3 --exact tools::builtin::job::tests::test_cancel_job_completedcargo fmt --checkNotes
Cargo.lockupdates caused by local dependency resolution so this PR stays test-only.cargo clippy --all --benches --tests --examples --all-features -- -D warningson the rebased branch because local disk pressure from a fresh full rebuild on currentmaincaused repeated out-of-space failures. The same test-only diff passed full clippy before the rebase, and the targeted tests above were rerun after rebasing this branch onto currentmain.Made with Cursor