Skip to content
This repository was archived by the owner on Sep 4, 2025. It is now read-only.

Conversation

anannya03
Copy link
Member

@anannya03 anannya03 commented Aug 6, 2025

What does this PR do?

Added Unit testcases for the ToolListCommand.cs file.

Command- azmcp tool list
Function: List all available tools in the Azure MCP server.

GitHub issue number?

[Link to the GitHub issue this PR addresses]

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Updated CHANGELOG.md for product changes (features, bug fixes, UI/UX, updated dependencies)
    • Spelling check passes: .\eng\common\spelling\Invoke-Cspell.ps1
  • For MCP tool changes, updated:
    • Updated README.md documentation
    • Updated command list in /docs/azmcp-commands.md
    • Updated test prompts in /e2eTests/e2eTestPrompts.md
  • 👉 For Community (non-Azure team member) PRs:
    • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
    • Manual tests run: added comment /azp run azure - mcp to run Live Test Pipeline

@Copilot Copilot AI review requested due to automatic review settings August 6, 2025 06:39
@anannya03 anannya03 requested a review from a team as a code owner August 6, 2025 06:39
Copy link
Contributor

@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 ToolsListCommand.cs functionality, which implements the azmcp tool list command that lists all available tools in the Azure MCP server. The tests validate various scenarios including successful execution, error handling, JSON serialization, and command structure validation.

Key changes:

  • Added 10 comprehensive test methods covering normal operation, error scenarios, and edge cases
  • Implemented proper mocking and service provider setup for isolated testing
  • Added validation for command structure, filtering, and metadata properties
Comments suppressed due to low confidence (2)

core/tests/AzureMcp.Core.UnitTests/Areas/Tools/UnitTests/ToolsListCommandTests.cs:147

  • This test assertion appears to be checking that the 'list' command is filtered out, but the logic is unclear and may not be testing the intended behavior. The test name suggests it should verify hidden commands are filtered, but this assertion doesn't clearly demonstrate what constitutes a 'hidden' command.
        Assert.DoesNotContain(result, cmd => cmd.Name == "list" && cmd.Command.Contains("tool"));

core/tests/AzureMcp.Core.UnitTests/Areas/Tools/UnitTests/ToolsListCommandTests.cs:149

  • This assertion is redundant as it only verifies that at least one command has a non-empty name, which is already covered by the earlier loop validation in ExecuteAsync_WithValidContext_ReturnsCommandInfoList test. This doesn't add meaningful test coverage for the hidden command filtering behavior.
        Assert.Contains(result, cmd => !string.IsNullOrEmpty(cmd.Name));

- Add 10 test methods covering various scenarios
- Test JSON serialization, error handling, and command validation
- Verify hidden command filtering and options validation
- Add helper methods for test data setup
@anannya03
Copy link
Member Author

While attempting to rebase my branch with main, I accidentally ran git push --force, which caused my original PR #796 to close.

I've since addressed all the review comments. For reference and better visibility, I’m including screenshots of each comment below:

image
image
image
image
  1. I have run- dotnet format "AzureMcp.sln" for formatting errors.
image

@anannya03 anannya03 requested a review from vcolin7 August 6, 2025 07:24
Copy link
Member

@vcolin7 vcolin7 left a comment

Choose a reason for hiding this comment

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

The changes look good. You'll have to update the .vscode/cspell.json file to include the words flagged by the pipeline. Alternatively, you can remove those words from the PR. Up to you :)

@github-project-automation github-project-automation bot moved this from Untriaged to In Progress in Azure MCP Server (OLD) Aug 7, 2025
@anannya03 anannya03 requested a review from vcolin7 August 10, 2025 21:17
@anannya03 anannya03 merged commit da4903f into main Aug 11, 2025
26 checks passed
@anannya03 anannya03 deleted the tool_list_testcase_restore branch August 11, 2025 17:19
@github-project-automation github-project-automation bot moved this from In Progress to Done in Azure MCP Server (OLD) Aug 11, 2025
chidozieononiwu pushed a commit to chidozieononiwu/azure-mcp that referenced this pull request Aug 18, 2025
* Unit testcase Tool List

* Cleaned the code

* Mocked a testcase properly

* Add comprehensive unit tests for ToolsListCommand

- Add 10 test methods covering various scenarios
- Test JSON serialization, error handling, and command validation
- Verify hidden command filtering and options validation
- Add helper methods for test data setup

* Added AppConfig to testcase and reviewed other comments

* Fixed a testcase check

* Fixed spelling error
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants