pkg/mcp: add tests for readonly enforcement, delete validation, and help resources#3831
pkg/mcp: add tests for readonly enforcement, delete validation, and help resources#3831Ankitsinghsisodya wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
Bot
added
size/M
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds negative-path test coverage for MCP deploy and delete tools to ensure correct behavior in readonly mode and for invalid argument combinations.
Changes:
- Add readonly-mode test for the
deploytool. - Add readonly-mode and argument-validation tests for the
deletetool.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/mcp/tools_deploy_test.go | Adds a readonly-mode test for the deploy tool. |
| pkg/mcp/tools_delete_test.go | Adds readonly-mode and validation tests for invalid delete arguments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lkingland
added
ok-to-test
knative-prow
Bot
added
size/L
- Implemented TestResource_Help to validate help command behavior for various subcommands. - Added tests for the delete tool to ensure it handles readonly mode and validation errors correctly when both path and name are provided, or neither is provided. - Included TestTool_Delete_Readonly, TestTool_Delete_BothPathAndName, and TestTool_Delete_NeitherPathNorName to cover these scenarios. These tests enhance the coverage and reliability of the MCP resource and tool functionalities.
- Simplified the test setup in TestTool_Delete_BothPathAndName and TestTool_Delete_NeitherPathNorName by removing the unused server variable from newTestPair. - This change enhances code clarity and maintains the focus on testing the delete tool's functionality.
- Updated TestTool_Delete_Readonly and TestTool_Deploy_Readonly to verify that the error messages returned when the server is in readonly mode contain the phrase "readonly mode". - Modified TestTool_Delete_BothPathAndName and TestTool_Delete_NeitherPathNorName to ensure that mutual-exclusion validation errors are correctly checked for the phrases "exactly one of 'path' or 'name'". - These changes improve the robustness of the tests by ensuring that the error messages are not only returned but also contain the expected content.
- Deleted TestTool_Delete_Readonly and TestTool_Deploy_Readonly as they were redundant following recent enhancements to error message validation. - This cleanup improves test maintainability and focuses on essential test cases for the delete and deploy functionalities.
Ankitsinghsisodya
force-pushed
the
fix/mcp-missing-test-coverage
branch
from
d03185b to
a07cc5e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3831 +/- ##
==========================================
+ Coverage 51.23% 53.46% +2.23%
==========================================
Files 200 200
Lines 23441 23450 +9
==========================================
+ Hits 12009 12537 +528
+ Misses 10272 9659 -613
- Partials 1160 1254 +94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
tools_deploy_test.goandtools_delete_test.goeach had a single happy-path test that manually setserver.readonly = false, leaving the readonly guard in both handlers completely untested.deleteHandler(exactly one ofpathornamemust be provided) had no test coverage for either branch.func://help/*resources registered inmcp.gohad zero tests; thenewHelpResourcehandler — URI construction, executor dispatch, args assembly, and response shape — was entirely unexercised.Changes
pkg/mcp/tools_deploy_test.go: addTestTool_Deploy_Readonlypkg/mcp/tools_delete_test.go: addTestTool_Delete_Readonly,TestTool_Delete_BothPathAndName, andTestTool_Delete_NeitherPathNorNamepkg/mcp/resources_test.go: addTestResource_Help— table-driven test over 6func://help/*URIs that validates executor subcommand, args, response URI, MIME type, and body textTest plan
go test ./pkg/mcp/...passes (all 10 new tests green)make testpassesmake checkpasses