✨ Adapt MCP protocal to different program language #416

[Specification Details] 1. Add test cases.

Author: YehongPan

test/backend/app/test_remote_mcp_app.py

@@ -621,6 +621,34 @@ def test_add_mcp_from_config_missing_command(self, mock_container_manager_class,
         data = response.json()
         assert "command" in str(data["detail"]).lower()
 
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    @patch('apps.remote_mcp_app.MCPContainerManager')
+    def test_add_mcp_from_config_empty_command(self, mock_container_manager_class, mock_get_user_id):
+        """Test adding MCP server with empty command string (covers line 189-191)"""
+        mock_get_user_id.return_value = ("user123", "tenant456")
+        
+        mock_container_manager = MagicMock()
+        mock_container_manager_class.return_value = mock_container_manager
+
+        response = client.post(
+            "/mcp/add-from-config",
+            json={
+                "mcpServers": {
+                    "test-service": {
+                        "command": "",
+                        "args": ["-y", "test-mcp"],
+                        "port": 5020
+                    }
+                }
+            },
+            headers={"Authorization": "Bearer test_token"}
+        )
+
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+        data = response.json()
+        assert "All MCP servers failed" in data["detail"]
+        assert "command is required" in data["detail"]
+
     @patch('apps.remote_mcp_app.get_current_user_id')
     @patch('apps.remote_mcp_app.MCPContainerManager')
     def test_add_mcp_from_config_missing_port(self, mock_container_manager_class, mock_get_user_id):
@@ -687,6 +715,48 @@ def test_add_mcp_from_config_name_exists(self, mock_add_server, mock_container_m
         assert "MCP name already exists" in data["detail"]
         mock_container_manager.stop_mcp_container.assert_called_once_with("container-123")
 
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    @patch('apps.remote_mcp_app.MCPContainerManager')
+    @patch('apps.remote_mcp_app.add_remote_mcp_server_list')
+    def test_add_mcp_from_config_name_exists_stop_fails(self, mock_add_server, mock_container_manager_class, mock_get_user_id):
+        """Test adding MCP server when name exists and stopping container fails (covers line 236)"""
+        mock_get_user_id.return_value = ("user123", "tenant456")
+        
+        mock_container_manager = MagicMock()
+        mock_container_manager_class.return_value = mock_container_manager
+        mock_container_manager.start_mcp_container = AsyncMock(return_value={
+            "container_id": "container-123",
+            "mcp_url": "http://localhost:5020/mcp",
+            "host_port": "5020",
+            "status": "started",
+            "container_name": "test-service-user1234"
+        })
+        # stop_mcp_container raises exception, should be silently caught
+        mock_container_manager.stop_mcp_container = AsyncMock(side_effect=Exception("Stop failed"))
+        
+        mock_add_server.side_effect = MCPNameIllegal("MCP name already exists")
+
+        response = client.post(
+            "/mcp/add-from-config",
+            json={
+                "mcpServers": {
+                    "test-service": {
+                        "command": "npx",
+                        "args": ["-y", "test-mcp"],
+                        "port": 5020
+                    }
+                }
+            },
+            headers={"Authorization": "Bearer test_token"}
+        )
+
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+        data = response.json()
+        assert "All MCP servers failed" in data["detail"]
+        assert "MCP name already exists" in data["detail"]
+        # Verify stop was attempted even though it failed
+        mock_container_manager.stop_mcp_container.assert_called_once_with("container-123")
+
     @patch('apps.remote_mcp_app.get_current_user_id')
     @patch('apps.remote_mcp_app.MCPContainerManager')
     def test_add_mcp_from_config_container_error(self, mock_container_manager_class, mock_get_user_id):
@@ -718,6 +788,36 @@ def test_add_mcp_from_config_container_error(self, mock_container_manager_class,
         assert "All MCP servers failed" in data["detail"]
         assert "Container failed" in data["detail"]
 
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    @patch('apps.remote_mcp_app.MCPContainerManager')
+    def test_add_mcp_from_config_unexpected_error_in_loop(self, mock_container_manager_class, mock_get_user_id):
+        """Test adding MCP server when unexpected exception occurs in loop (covers line 253-255)"""
+        mock_get_user_id.return_value = ("user123", "tenant456")
+        
+        mock_container_manager = MagicMock()
+        mock_container_manager_class.return_value = mock_container_manager
+        # Raise a non-MCPContainerError exception to trigger the general Exception handler
+        mock_container_manager.start_mcp_container = AsyncMock(side_effect=ValueError("Unexpected error"))
+
+        response = client.post(
+            "/mcp/add-from-config",
+            json={
+                "mcpServers": {
+                    "test-service": {
+                        "command": "npx",
+                        "args": ["-y", "test-mcp"],
+                        "port": 5020
+                    }
+                }
+            },
+            headers={"Authorization": "Bearer test_token"}
+        )
+
+        assert response.status_code == HTTPStatus.BAD_REQUEST
+        data = response.json()
+        assert "All MCP servers failed" in data["detail"]
+        assert "Unexpected error" in data["detail"]
+
     @patch('apps.remote_mcp_app.get_current_user_id')
     @patch('apps.remote_mcp_app.MCPContainerManager')
     def test_add_mcp_from_config_all_fail(self, mock_container_manager_class, mock_get_user_id):
@@ -815,6 +915,30 @@ def test_add_mcp_from_config_with_custom_image(self, mock_add_server, mock_conta
         call_kwargs = mock_container_manager.start_mcp_container.call_args[1]
         assert call_kwargs["image"] == "custom-image:latest"
 
+    @patch('apps.remote_mcp_app.get_current_user_id')
+    def test_add_mcp_from_config_outer_exception(self, mock_get_user_id):
+        """Test adding MCP server when exception occurs outside loop (covers line 275-277)"""
+        # Make get_current_user_id raise an exception to trigger outer exception handler
+        mock_get_user_id.side_effect = RuntimeError("Failed to get user ID")
+
+        response = client.post(
+            "/mcp/add-from-config",
+            json={
+                "mcpServers": {
+                    "test-service": {
+                        "command": "npx",
+                        "args": ["-y", "test-mcp"],
+                        "port": 5020
+                    }
+                }
+            },
+            headers={"Authorization": "Bearer test_token"}
+        )
+
+        assert response.status_code == HTTPStatus.INTERNAL_SERVER_ERROR
+        data = response.json()
+        assert "Failed to add MCP servers" in data["detail"]
+
 
 # ---------------------------------------------------------------------------
 # Test stop_mcp_container

test/sdk/container/test_docker_client.py

@@ -887,6 +887,100 @@ async def test_start_container_existing_multiple_ports(self, docker_container_cl
             assert result["status"] == "existing"
             assert result["host_port"] == "5020"
 
+    @pytest.mark.asyncio
+    async def test_start_container_existing_no_port_mapping_with_host_port(self, docker_container_client, mock_container):
+        """Test starting container when existing container has no port mapping but host_port is provided (line 228-229)"""
+        docker_container_client.client.containers.get.return_value = mock_container
+        mock_container.status = "running"
+        mock_container.attrs = {
+            "NetworkSettings": {
+                "Ports": {}  # No port mappings
+            }
+        }
+
+        with patch.object(DockerContainerClient, "_is_running_in_docker", return_value=False), \
+             patch.object(DockerContainerClient, "_get_service_host", return_value="localhost"):
+            result = await docker_container_client.start_container(
+                service_name="test-service",
+                tenant_id="tenant123",
+                user_id="user12345",
+                full_command=["npx", "-y", "test-mcp"],
+                host_port=5025,  # Provide host_port parameter
+            )
+
+            # Should use existing container with provided host_port
+            assert result["status"] == "existing"
+            assert result["host_port"] == "5025"
+
+    @pytest.mark.asyncio
+    async def test_start_container_empty_full_command(self, docker_container_client):
+        """Test starting container with empty full_command raises error (line 272-273)"""
+        docker_container_client.client.containers.get.side_effect = NotFound("Container not found")
+
+        with pytest.raises(ContainerError, match="full_command is required to start container"):
+            await docker_container_client.start_container(
+                service_name="test-service",
+                tenant_id="tenant123",
+                user_id="user12345",
+                full_command=[],  # Empty command
+            )
+
+    @pytest.mark.asyncio
+    async def test_start_container_with_custom_image(self, docker_container_client):
+        """Test starting container with custom image parameter (line 276-277)"""
+        docker_container_client.client.containers.get.side_effect = NotFound("Container not found")
+
+        new_container = MagicMock()
+        new_container.id = "new-container-id"
+        new_container.status = "running"
+        new_container.reload.return_value = None
+        docker_container_client.client.containers.run.return_value = new_container
+
+        with patch.object(DockerContainerClient, "find_free_port", return_value=5020), \
+             patch.object(DockerContainerClient, "_get_service_host", return_value="localhost"), \
+             patch.object(DockerContainerClient, "_wait_for_service_ready", new_callable=AsyncMock), \
+             patch("asyncio.sleep", new_callable=AsyncMock):
+            await docker_container_client.start_container(
+                service_name="test-service",
+                tenant_id="tenant123",
+                user_id="user12345",
+                full_command=["python", "script.py"],
+                image="python:3.11-alpine",  # Custom image
+            )
+
+            call_args = docker_container_client.client.containers.run.call_args
+            assert call_args is not None
+            assert call_args.kwargs["image"] == "python:3.11-alpine"
+
+    @pytest.mark.asyncio
+    async def test_start_container_with_host_port_provided(self, docker_container_client):
+        """Test starting container when host_port is provided (line 252 - skip find_free_port)"""
+        docker_container_client.client.containers.get.side_effect = NotFound("Container not found")
+
+        new_container = MagicMock()
+        new_container.id = "new-container-id"
+        new_container.status = "running"
+        new_container.reload.return_value = None
+        docker_container_client.client.containers.run.return_value = new_container
+
+        with patch.object(DockerContainerClient, "find_free_port") as mock_find_port, \
+             patch.object(DockerContainerClient, "_get_service_host", return_value="localhost"), \
+             patch.object(DockerContainerClient, "_wait_for_service_ready", new_callable=AsyncMock), \
+             patch("asyncio.sleep", new_callable=AsyncMock), \
+             patch.object(DockerContainerClient, "_is_running_in_docker", return_value=False):
+            await docker_container_client.start_container(
+                service_name="test-service",
+                tenant_id="tenant123",
+                user_id="user12345",
+                full_command=["npx", "-y", "test-mcp"],
+                host_port=8080,  # Provide host_port, should skip find_free_port
+            )
+
+            # find_free_port should not be called when host_port is provided
+            mock_find_port.assert_not_called()
+            call_args = docker_container_client.client.containers.run.call_args
+            assert call_args.kwargs["environment"]["PORT"] == "8080"
+
 
 # ---------------------------------------------------------------------------
 # Test _wait_for_service_ready
@@ -950,6 +1044,27 @@ async def test_wait_for_service_ready_exception(self, docker_container_client):
             with pytest.raises(ContainerConnectionError):
                 await docker_container_client._wait_for_service_ready("http://localhost:5020/mcp", max_retries=3, retry_delay=0.1)
 
+    @pytest.mark.asyncio
+    async def test_wait_for_service_ready_loop_iterations(self, docker_container_client):
+        """Test waiting for service ready with multiple loop iterations (line 356)"""
+        mock_client = MagicMock()
+        # Simulate multiple failures before success to test loop
+        call_count = 0
+        def is_connected():
+            nonlocal call_count
+            call_count += 1
+            return call_count >= 5  # Success on 5th attempt
+        mock_client.is_connected.side_effect = is_connected
+        mock_client.__aenter__ = AsyncMock(return_value=mock_client)
+        mock_client.__aexit__ = AsyncMock(return_value=False)
+
+        with patch("nexent.container.docker_client.Client", return_value=mock_client), \
+             patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep:
+            await docker_container_client._wait_for_service_ready("http://localhost:5020/mcp", max_retries=10, retry_delay=0.01)
+            
+            # Should have slept 4 times (before 5th attempt succeeds)
+            assert mock_sleep.call_count == 4
+
 
 # ---------------------------------------------------------------------------
 # Test stop_container
@@ -1111,6 +1226,32 @@ def test_list_containers_empty_port_mapping(self, docker_container_client):
             assert len(result) == 1
             assert result[0]["host_port"] is None
 
+    def test_list_containers_host_port_none_or_empty(self, docker_container_client):
+        """Test listing containers when host_port is None or empty string (line 448)"""
+        container = MagicMock()
+        container.id = "test-container-id"
+        container.name = "mcp-test-service-user12345"
+        container.status = "running"
+        container.attrs = {
+            "NetworkSettings": {
+                "Ports": {
+                    "5020/tcp": [{"HostPort": None}],  # None value
+                    "5021/tcp": [{"HostPort": ""}],   # Empty string
+                }
+            }
+        }
+        docker_container_client.client.containers.list.return_value = [container]
+
+        with patch.object(DockerContainerClient, "_get_service_host", return_value="localhost"), \
+             patch.object(DockerContainerClient, "_is_running_in_docker", return_value=False):
+            result = docker_container_client.list_containers()
+
+            assert len(result) == 1
+            # Should not break on None or empty HostPort
+            # When HostPort is empty string, it will be returned as empty string (not None)
+            # Since the last HostPort value is empty string, host_port will be empty string
+            assert result[0]["host_port"] == ""
+
     def test_list_containers_exception(self, docker_container_client):
         """Test listing containers when exception occurs"""
         docker_container_client.client.containers.list.side_effect = Exception("Connection error")
@@ -1269,6 +1410,34 @@ def test_get_container_status_empty_port_mapping(self, docker_container_client):
             assert result is not None
             assert result["host_port"] is None
 
+    def test_get_container_status_host_port_none_or_empty(self, docker_container_client):
+        """Test getting container status when host_port is None or empty string (line 513)"""
+        container = MagicMock()
+        container.id = "test-container-id"
+        container.name = "mcp-test-service-user12345"
+        container.status = "running"
+        container.attrs = {
+            "NetworkSettings": {
+                "Ports": {
+                    "5020/tcp": [{"HostPort": None}],  # None value
+                    "5021/tcp": [{"HostPort": ""}],   # Empty string
+                }
+            },
+            "Created": "2024-01-01T00:00:00Z",
+            "Config": {"Image": "node:22-alpine"},
+        }
+        docker_container_client.client.containers.get.return_value = container
+
+        with patch.object(DockerContainerClient, "_get_service_host", return_value="localhost"), \
+             patch.object(DockerContainerClient, "_is_running_in_docker", return_value=False):
+            result = docker_container_client.get_container_status("test-container-id")
+
+            assert result is not None
+            # Should not break on None or empty HostPort
+            # When HostPort is empty string, it will be returned as empty string (not None)
+            # Since the last HostPort value is empty string, host_port will be empty string
+            assert result["host_port"] == ""
+
 
 # ---------------------------------------------------------------------------
 # Test _ensure_network
@@ -1503,6 +1672,22 @@ def test_get_container_service_port_no_hostport(self):
         port = DockerContainerClient._get_container_service_port(container)
         assert port is None
 
+    def test_get_container_service_port_non_string_env_item(self):
+        """Test getting port when env list contains non-string items (line 131)"""
+        container = MagicMock()
+        container.attrs = {
+            "Config": {
+                "Env": [123, "PORT=5020", None, {"key": "value"}]  # Mixed types
+            },
+            "NetworkSettings": {
+                "Ports": {}
+            }
+        }
+
+        port = DockerContainerClient._get_container_service_port(container)
+        # Should still find PORT=5020 despite non-string items
+        assert port == "5020"
+
 
 # ---------------------------------------------------------------------------
 # Test start_container in Docker mode