pkg/mcp: add tests for readonly enforcement, delete validation, and help resources#3666
pkg/mcp: add tests for readonly enforcement, delete validation, and help resources#3666Ankitsinghsisodya wants to merge 2 commits intoknative:mainfrom
Conversation
knative-prow
Bot
added
size/L
|
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
Adds targeted test coverage in pkg/mcp to ensure MCP tools/resources correctly enforce readonly mode, validate delete input, and correctly dispatch/shape help resource responses.
Changes:
- Added readonly-mode negative-path tests for
deployanddeletetools. - Added delete input validation tests for mutual exclusion and missing required selector (
pathvsname). - Added a table-driven test validating
func://help/*resource dispatch (executor args) and response shape.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/mcp/tools_deploy_test.go |
Adds a readonly enforcement test for the deploy tool. |
pkg/mcp/tools_delete_test.go |
Adds readonly and input-validation error-path tests for delete. |
pkg/mcp/resources_test.go |
Adds table-driven coverage for help resources’ executor dispatch and returned content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3666 +/- ##
==========================================
+ Coverage 54.42% 56.94% +2.52%
==========================================
Files 181 181
Lines 20926 20926
==========================================
+ Hits 11389 11917 +528
+ Misses 8423 7803 -620
- Partials 1114 1206 +92
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:
|
There was a problem hiding this comment.
the gap analysis in the description is accurate (the existing handler tests manually set server.readonly = false, so the guard was uncovered).
One heads-up: this PR and #3667 both add a function called TestResource_Help to pkg/mcp/resources_test.go, so whichever lands second will hit a merge conflict
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ankitsinghsisodya, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
approved
knative-prow-robot
added
the
needs-rebase
- 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.
Ankitsinghsisodya
force-pushed
the
fix/mcp-missing-test-coverage
branch
from
fd13b24 to
1fe74bb
Compare
|
New changes are detected. LGTM label has been removed. |
knative-prow-robot
removed
the
needs-rebase
knative-prow
Bot
added
size/M
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_Readonly— usesnewTestPairWithReadonly(t, true)and assertsresult.IsError.pkg/mcp/tools_delete_test.go: addTestTool_Delete_Readonly,TestTool_Delete_BothPathAndName, andTestTool_Delete_NeitherPathNorName.pkg/mcp/resources_test.go: addTestResource_Help— table-driven test over 6func://help/*URIs that validates executor subcommand, args, response URI, MIME type, and body text.Test plan
go test ./pkg/mcp/...passes (all 10 new tests green)make testpassesmake checkpasses