Skip to content

feat: add uts for mcpgo and toolsets pkgs #13

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ChiragChiranjib
Copy link
Collaborator

Description

Add uts for mcpgo and toolsets packages.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Refactoring (no functional changes, code improvements only)

Testing

  • Manual testing
  • Added unit tests
  • All tests pass locally

Checklist

  • I have followed the code style of this project
  • I have added comments to code where necessary, particularly in hard-to-understand areas
  • I have updated the documentation where necessary
  • I have verified that my changes do not introduce new warnings or errors
  • I have checked for and resolved any merge conflicts
  • I have considered the performance implications of my changes

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive unit tests for the mcpgo and toolsets packages to improve test coverage and validate expected behaviors.

  • Added new unit tests for toolset management and enabling logic in pkg/toolsets.
  • Introduced tests for JSON, text, and error outputs in tool results under pkg/mcpgo.
  • Expanded server tests, including stdio server creation and tool conversion validations, in pkg/mcpgo.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/toolsets/toolsets_test.go Tests validating toolset group functionality and behavior.
pkg/mcpgo/tool_test.go Unit tests for various tool result outputs including JSON.
pkg/mcpgo/stdio_test.go Tests for stdio server creation and invalid server error cases.
pkg/mcpgo/stdio.go Refactored NewStdioServer to return a TransportServer interface.
pkg/mcpgo/server_test.go Comprehensive tests covering server creation, tool conversion, and option setter behavior.

stuckinforloop
stuckinforloop previously approved these changes May 7, 2025
Copy link
Collaborator

@stuckinforloop stuckinforloop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. I think we can remove tests for mcpgo pkg. In case we are keeping them, seems good to me.

@@ -0,0 +1,323 @@
package mcpgo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these tests for mcpgo pkg required given those are already tested in the mark3labs/mcp-go pkg

Copy link
Collaborator Author

@ChiragChiranjib ChiragChiranjib May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're implementing a wrapper around our functional handler, we should ensure it's properly tested—this was something that got missed when we wrote unit tests for our tools earlier.

Additionally, the definition of tool parameters within the schema should also be covered by tests, as it directly impacts the structure consumed by the LLM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants