Add unit tests and enhance tool handling logic

klapaudius · klapaudius · commit 66536edefb20 · 2025-05-16T23:48:22.000+02:00
Introduced `ToolsCallHandlerTest` for comprehensive testing of tool execution paths and error handling. Updated XML service configuration and schema validation to improve tool argument handling. Adjusted ping interval constraints and corrected transport output formatting in `SseTransport`.
diff --git a/src/Resources/config/routes.php b/src/Resources/config/routes.php
@@ -17,7 +17,7 @@
         ->controller([SseController::class, 'handle'])
         ->methods(['GET', 'POST']);
 
-    $routes->add('message_route', "/$defaultPath/message")
+    $routes->add('message_route', "/$defaultPath/messages")
         ->controller([MessageController::class, 'handle'])
         ->methods(['POST']);
 };
diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml
@@ -32,6 +32,8 @@
             <argument type="string">%klp_mcp_server.default_path%</argument>
             <argument type="service" id="klp_mcp_server.adapter.redis" on-invalid="null" />
             <argument type="service" id="logger" on-invalid="null" />
+            <argument type="binary">%klp_mcp_server.ping.enabled%</argument>
+            <argument type="string">%klp_mcp_server.ping.interval%</argument>
         </service>
 
         <service id="klp_mcp_server.protocol" class="KLP\KlpMcpServer\Protocol\MCPProtocol">
diff --git a/src/Services/ToolService/Examples/HelloWorldTool.php b/src/Services/ToolService/Examples/HelloWorldTool.php
@@ -20,7 +20,7 @@ public function getInputSchema(): array
     {
         return [
             'type' => 'object',
-            'properties' => [
+            'arguments' => [
                 'name' => [
                     'type' => 'string',
                     'description' => 'Developer Name',
diff --git a/src/Services/ToolService/ToolParamsValidator.php b/src/Services/ToolService/ToolParamsValidator.php
@@ -64,7 +64,7 @@ public static function validate(array $toolSchema, array $arguments): void
             }
             $valid &= $test;
         }
-        foreach ($toolSchema['required'] as $argument) {
+        foreach ($toolSchema['required'] ?? [] as $argument) {
             $test = ! empty($arguments[$argument]);
             if (! $test) {
                 self::$errors[] = "Missing required argument: $argument";
diff --git a/src/Transports/SseTransport.php b/src/Transports/SseTransport.php
@@ -45,36 +45,34 @@ final class SseTransport implements SseTransportInterface
      */
     protected array $messageHandlers = [];
 
-    /**
-     * Optional adapter for message persistence and retrieval (e.g., Redis).
-     * Enables simulation of request/response patterns over SSE.
-     */
-    protected ?SseAdapterInterface $adapter = null;
-
     /**
      * Unique identifier for the client connection, generated during initialization.
      */
     protected ?string $clientId = null;
 
-
-    protected bool $pingEnabled = false;
-
     /**
      * Stores the timestamp of the most recent ping, represented as an integer.
      */
     protected int $lastPingTimestamp = 0;
 
+
     /**
-     * Defines the interval, in secondes, at which ping messages are sent to maintain the connection.
+     * Initializes the class with the default path, adapter, logger, and ping settings.
+     *
+     * @param string $defaultPath The default path for resources.
+     * @param SseAdapterInterface|null $adapter Optional adapter for message persistence and retrieval (e.g., Redis).
+     *                                          Enables simulation of request/response patterns over SSE.
+     * @param LoggerInterface|null $logger The logger instance (optional).
+     * @param bool $pingEnabled Flag to enable or disable ping functionality.
+     * @param int $pingInterval The interval, in secondes, at which ping messages are sent to maintain the connection.
      */
-    protected int $pingInterval = 60;
-
     public function __construct(
         private readonly string $defaultPath,
-        ?SseAdapterInterface $adapter = null,
-        private readonly ?LoggerInterface $logger = null
+        private ?SseAdapterInterface $adapter = null,
+        private readonly ?LoggerInterface $logger = null,
+        private bool $pingEnabled = false,
+        private int $pingInterval = 10
     ) {
-        $this->adapter = $adapter;
     }
 
     /**
@@ -334,7 +332,7 @@ public function pushMessage(string $clientId, array $message): void
 
     private function getEndpoint(string $sessionId): string
     {
-        return sprintf('/%s/message?sessionId=%s',
+        return sprintf('/%s/messages?sessionId=%s',
             trim($this->defaultPath, '/'),
             $sessionId,
         );
@@ -362,18 +360,18 @@ private function checkPing(): bool
                 return false;
             }
         }
-        return time() - $this->getLastPongResponseTimestamp() < $this->pingInterval + 60;
+        return time() - $this->getLastPongResponseTimestamp() < $this->pingInterval * 1.8;
     }
 
     /**
      * Sets the interval for sending ping requests.
      *
      * @param  int  $pingInterval  The interval in milliseconds at which ping requests should be sent.
-     *                             The value must be between 60 and 180 secondes.
+     *                             The value must be between 5 and 30 secondes.
      */
     protected function setPingInterval(int $pingInterval): void
     {
-        $this->pingInterval = max(60, min($pingInterval, 180));
+        $this->pingInterval = max(5, min($pingInterval, 30));
     }
 
     public function getAdapter(): ?SseAdapterInterface
diff --git a/src/stubs/tool.stub b/src/stubs/tool.stub
@@ -35,7 +35,7 @@ class {{ className }} implements ToolInterface
     {
         return [
             'type' => 'object',
-            'properties' => [
+            'arguments' => [
                 'param1' => [
                     'type' => 'string',
                     'description' => 'First parameter description',
diff --git a/tests/Server/Request/ToolsCallHandlerTest.php b/tests/Server/Request/ToolsCallHandlerTest.php
@@ -0,0 +1,112 @@
+<?php
+
+namespace KLP\KlpMcpServer\Tests\Server\Request;
+
+use KLP\KlpMcpServer\Exceptions\JsonRpcErrorException;
+use KLP\KlpMcpServer\Exceptions\ToolParamsValidatorException;
+use KLP\KlpMcpServer\Server\Request\ToolsCallHandler;
+use KLP\KlpMcpServer\Services\ToolService\Examples\HelloWorldTool;
+use KLP\KlpMcpServer\Services\ToolService\ToolParamsValidator;
+use KLP\KlpMcpServer\Services\ToolService\ToolRepository;
+use PHPUnit\Framework\TestCase;
+
+class ToolsCallHandlerTest extends TestCase
+{
+    private ToolRepository $toolRepository;
+    private ToolsCallHandler $toolsCallHandler;
+
+    protected function setUp(): void
+    {
+        $this->toolRepository = $this->createMock(ToolRepository::class);
+        $this->toolsCallHandler = new ToolsCallHandler($this->toolRepository);
+    }
+
+    public function test_execute_throws_exception_when_tool_name_is_missing(): void
+    {
+        $this->expectException(JsonRpcErrorException::class);
+        $this->expectExceptionMessage('Tool name is required');
+
+        $this->toolsCallHandler->execute('tools/call', 1, []);
+    }
+
+    public function test_execute_throws_exception_when_tool_not_found(): void
+    {
+        $this->toolRepository
+            ->method('getTool')
+            ->with('nonexistent-tool')
+            ->willReturn(null);
+
+        $this->expectException(JsonRpcErrorException::class);
+        $this->expectExceptionMessage("Tool 'nonexistent-tool' not found");
+
+        $this->toolsCallHandler->execute('tools/call', 2, ['name' => 'nonexistent-tool']);
+    }
+
+    public function test_execute_throws_exception_for_invalid_arguments(): void
+    {
+        $toolMock = $this->createMock(HelloWorldTool::class);
+        $toolMock->method('getInputSchema')->willReturn(['type' => 'object']);
+
+        $this->toolRepository
+            ->method('getTool')
+            ->with('HelloWorldTool')
+            ->willReturn($toolMock);
+
+        $this->expectException(ToolParamsValidatorException::class);
+
+        ToolParamsValidator::validate(['type' => 'object'], ['invalid' => 'data']);
+
+        $this->toolsCallHandler->execute('tools/execute', 3, ['name' => 'HelloWorldTool', 'arguments' => ['invalid' => 'data']]);
+    }
+
+    public function test_execute_returns_content_for_tools_call(): void
+    {
+        $this->toolRepository
+            ->method('getTool')
+            ->with('HelloWorldTool')
+            ->willReturn(new HelloWorldTool);
+
+        $result = $this->toolsCallHandler->execute('tools/call', 4, ['name' => 'HelloWorldTool', 'arguments' => ['name' => 'Success Message']]);;
+
+        $this->assertEquals(
+            [
+                'content' => [
+                    ['type' => 'text', 'text' => 'Hello, HelloWorld `Success Message` developer.']
+                ]
+            ],
+            $result
+        );
+    }
+
+    public function test_execute_returns_result_for_tools_execute(): void
+    {
+        $this->toolRepository
+            ->method('getTool')
+            ->with('HelloWorldTool')
+            ->willReturn(new HelloWorldTool);
+
+        $result = $this->toolsCallHandler->execute('tools/execute', 5, ['name' => 'HelloWorldTool', 'arguments' => ['name' => 'Success Message']]);;;
+
+        $this->assertEquals(
+            [
+                'result' => "Hello, HelloWorld `Success Message` developer."
+            ],
+            $result
+        );
+    }
+
+    public function test_is_handle_returns_true_for_tools_call(): void
+    {
+        $this->assertTrue($this->toolsCallHandler->isHandle('tools/call'));
+    }
+
+    public function test_is_handle_returns_true_for_tools_execute(): void
+    {
+        $this->assertTrue($this->toolsCallHandler->isHandle('tools/execute'));
+    }
+
+    public function test_is_handle_returns_false_for_invalid_method(): void
+    {
+        $this->assertFalse($this->toolsCallHandler->isHandle('invalid/method'));
+    }
+}
diff --git a/tests/Transports/SseTransportTest.php b/tests/Transports/SseTransportTest.php
@@ -70,7 +70,7 @@ public function test_initialize_generates_client_id_and_sends_endpoint(): void
         // Assert
         $expectedClientId = $this->getProtectedProperty($this->instance, 'clientId');
         $expectedOutput = 'event: endpoint'.PHP_EOL
-            .'data: /default-path/message?sessionId='.$expectedClientId.PHP_EOL.PHP_EOL;
+            .'data: /default-path/messages?sessionId='.$expectedClientId.PHP_EOL.PHP_EOL;
 
         $this->assertNotNull($expectedClientId);
         $this->assertEquals($expectedOutput, $output);
@@ -97,7 +97,7 @@ public function test_initialize_does_not_overwrite_existing_client_id(): void
         // Assert
         $currentClientId = $this->getProtectedProperty($this->instance, 'clientId');
         $expectedOutput = 'event: endpoint'.PHP_EOL
-            .'data: /default-path/message?sessionId='.$existingClientId.PHP_EOL.PHP_EOL;
+            .'data: /default-path/messages?sessionId='.$existingClientId.PHP_EOL.PHP_EOL;
 
         $this->assertSame($existingClientId, $currentClientId);
         $this->assertEquals($expectedOutput, $output);
@@ -631,7 +631,7 @@ private function setProtectedProperty(SseTransport $instance, string $propertyNa
     public function test_set_ping_interval_updates_correctly(): void
     {
         // Arrange
-        $validPingInterval = 120; // A valid interval in secondes
+        $validPingInterval = 10; // A valid interval in secondes
         $reflection = new \ReflectionClass($this->instance);
         $method = $reflection->getMethod('setPingInterval');
 
@@ -649,7 +649,7 @@ public function test_set_ping_interval_updates_correctly(): void
     public function test_set_ping_interval_with_less_than_minimum_value(): void
     {
         // Arrange
-        $invalidPingInterval = 10; // Less than the minimum allowed value (60s)
+        $invalidPingInterval = 1; // Less than the minimum allowed value (5s)
         $reflection = new \ReflectionClass($this->instance);
         $method = $reflection->getMethod('setPingInterval');
 
@@ -658,7 +658,7 @@ public function test_set_ping_interval_with_less_than_minimum_value(): void
         $updatedPingInterval = $reflection->getProperty('pingInterval')->getValue($this->instance);
 
         // Assert
-        $this->assertEquals(60, $updatedPingInterval);
+        $this->assertEquals(5, $updatedPingInterval);
     }
 
     /**
@@ -667,7 +667,7 @@ public function test_set_ping_interval_with_less_than_minimum_value(): void
     public function test_set_ping_interval_with_more_than_maximum_value(): void
     {
         // Arrange
-        $invalidPingInterval = 240; // More than the maximum allowed value (180 s)
+        $invalidPingInterval = 60; // More than the maximum allowed value (30 s)
         $reflection = new \ReflectionClass($this->instance);
         $method = $reflection->getMethod('setPingInterval');
 
@@ -676,7 +676,7 @@ public function test_set_ping_interval_with_more_than_maximum_value(): void
         $updatedPingInterval = $reflection->getProperty('pingInterval')->getValue($this->instance);
 
         // Assert
-        $this->assertEquals(180, $updatedPingInterval);
+        $this->assertEquals(30, $updatedPingInterval);
     }
 
     /**
@@ -779,7 +779,7 @@ public function test_is_connected_triggers_a_ping_message_if_needed(): void
         }
 
         // Assert
-        $this->assertTrue($result, 'Expected isConnected to return false when connection is not active.');
+        $this->assertFalse($result, 'Expected isConnected to return false when connection is not active.');
         $this->assertStringContainsString('"method":"ping"', $output);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ public function getInputSchema(): array`
`20`	`20`	`{`
`21`	`21`	`return [`
`22`	`22`	`'type' => 'object',`
`23`		`- 'properties' => [`
	`23`	`+ 'arguments' => [`
`24`	`24`	`'name' => [`
`25`	`25`	`'type' => 'string',`
`26`	`26`	`'description' => 'Developer Name',`
Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,7 @@ public static function validate(array $toolSchema, array $arguments): void`
`64`	`64`	`}`
`65`	`65`	`$valid &= $test;`
`66`	`66`	`}`
`67`		`- foreach ($toolSchema['required'] as $argument) {`
	`67`	`+ foreach ($toolSchema['required'] ?? [] as $argument) {`
`68`	`68`	`$test = ! empty($arguments[$argument]);`
`69`	`69`	`if (! $test) {`
`70`	`70`	`self::$errors[] = "Missing required argument: $argument";`
Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,7 @@ class {{ className }} implements ToolInterface`
`35`	`35`	`{`
`36`	`36`	`return [`
`37`	`37`	`'type' => 'object',`
`38`		`- 'properties' => [`
	`38`	`+ 'arguments' => [`
`39`	`39`	`'param1' => [`
`40`	`40`	`'type' => 'string',`
`41`	`41`	`'description' => 'First parameter description',`