Skip to content

refactor: use builder pattern for CallToolResult and simplify lambda expressions in tests #652

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.

Sign up for GitHub

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 1 commit into
base: main
Choose a base branch
from
+85 −55
Open

refactor: use builder pattern for CallToolResult and simplify lambda expressions in tests #652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...src/test/java/io/modelcontextprotocol/server/AbstractMcpClientServerIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ void testThrowingToolCallIsCaughtBeforeTimeout(String clientType) {

@ParameterizedTest(name = "{0} : {displayName} ")
@MethodSource("clientsForTesting")
void testToolCallSuccessWithTranportContextExtraction(String clientType) {
void testToolCallSuccessWithTransportContextExtraction(String clientType) {

var clientBuilder = clientBuilders.get(clientType);

Expand Down
120 changes: 71 additions & 49 deletions mcp-core/src/test/java/io/modelcontextprotocol/server/AbstractMcpSyncServerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,14 @@ void testConstructorWithInvalidArguments() {
void testGracefulShutdown() {
var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
void testImmediateClose() {
var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

assertThatCode(() -> mcpSyncServer.close()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::close).doesNotThrowAnyException();
}

@Test
Expand All @@ -94,7 +94,7 @@ void testGetAsyncServer() {

assertThat(mcpSyncServer.getAsyncServer()).isNotNull();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

// ---------------------------------------
Expand All @@ -117,7 +117,7 @@ void testAddTool() {
(exchange, args) -> new CallToolResult(List.of(), false))))
.doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -134,10 +134,10 @@ void testAddToolCall() {

assertThatCode(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
.tool(newTool)
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
.callHandler((exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build())).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -158,7 +158,7 @@ void testAddDuplicateTool() {
(exchange, args) -> new CallToolResult(List.of(), false))))
.doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -171,15 +171,16 @@ void testAddDuplicateToolCall() {

var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false))
.toolCall(duplicateTool,
(exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build();

assertThatCode(() -> mcpSyncServer.addTool(McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
.callHandler((exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build())).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -192,8 +193,10 @@ void testDuplicateToolCallDuringBuilding() {

assertThatThrownBy(() -> prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false))
.toolCall(duplicateTool, (exchange, request) -> new CallToolResult(List.of(), false)) // Duplicate!
.toolCall(duplicateTool,
(exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.toolCall(duplicateTool,
(exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build()) // Duplicate!
.build()).isInstanceOf(IllegalArgumentException.class)
.hasMessage("Tool with name 'duplicate-build-toolcall' is already registered.");
}
Expand All @@ -208,11 +211,13 @@ void testDuplicateToolsInBatchListRegistration() {
List<McpServerFeatures.SyncToolSpecification> specs = List.of(
McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
.callHandler(
(exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build(),
McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
.callHandler(
(exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build() // Duplicate!
);

Expand All @@ -235,11 +240,12 @@ void testDuplicateToolsInBatchVarargsRegistration() {
.capabilities(ServerCapabilities.builder().tools(true).build())
.tools(McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
.callHandler((exchange, request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build(),
McpServerFeatures.SyncToolSpecification.builder()
.tool(duplicateTool)
.callHandler((exchange, request) -> new CallToolResult(List.of(), false))
.callHandler((exchange,
request) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build() // Duplicate!
)
.build()).isInstanceOf(IllegalArgumentException.class)
Expand All @@ -256,12 +262,12 @@ void testRemoveTool() {

var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0")
.capabilities(ServerCapabilities.builder().tools(true).build())
.toolCall(tool, (exchange, args) -> new CallToolResult(List.of(), false))
.toolCall(tool, (exchange, args) -> CallToolResult.builder().content(List.of()).isError(false).build())
.build();

assertThatCode(() -> mcpSyncServer.removeTool(TEST_TOOL_NAME)).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -272,16 +278,16 @@ void testRemoveNonexistentTool() {

assertThatCode(() -> mcpSyncServer.removeTool("nonexistent-tool")).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
void testNotifyToolsListChanged() {
var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

assertThatCode(() -> mcpSyncServer.notifyToolsListChanged()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::notifyToolsListChanged).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

// ---------------------------------------
Expand All @@ -292,9 +298,9 @@ void testNotifyToolsListChanged() {
void testNotifyResourcesListChanged() {
var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

assertThatCode(() -> mcpSyncServer.notifyResourcesListChanged()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::notifyResourcesListChanged).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -305,7 +311,7 @@ void testNotifyResourcesUpdated() {
.notifyResourcesUpdated(new McpSchema.ResourcesUpdatedNotification(TEST_RESOURCE_URI)))
.doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -314,14 +320,18 @@ void testAddResource() {
.capabilities(ServerCapabilities.builder().resources(true, false).build())
.build();

Resource resource = new Resource(TEST_RESOURCE_URI, "Test Resource", "text/plain", "Test resource description",
null);
Resource resource = Resource.builder()
.uri(TEST_RESOURCE_URI)
.name("Test Resource")
.mimeType("text/plain")
.description("Test resource description")
.build();
McpServerFeatures.SyncResourceSpecification specification = new McpServerFeatures.SyncResourceSpecification(
resource, (exchange, req) -> new ReadResourceResult(List.of()));

assertThatCode(() -> mcpSyncServer.addResource(specification)).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -334,15 +344,19 @@ void testAddResourceWithNullSpecification() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Resource must not be null");

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
void testAddResourceWithoutCapability() {
var serverWithoutResources = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

Resource resource = new Resource(TEST_RESOURCE_URI, "Test Resource", "text/plain", "Test resource description",
null);
Resource resource = Resource.builder()
.uri(TEST_RESOURCE_URI)
.name("Test Resource")
.mimeType("text/plain")
.description("Test resource description")
.build();
McpServerFeatures.SyncResourceSpecification specification = new McpServerFeatures.SyncResourceSpecification(
resource, (exchange, req) -> new ReadResourceResult(List.of()));

Expand All @@ -366,8 +380,12 @@ void testListResources() {
.capabilities(ServerCapabilities.builder().resources(true, false).build())
.build();

Resource resource = new Resource(TEST_RESOURCE_URI, "Test Resource", "text/plain", "Test resource description",
null);
Resource resource = Resource.builder()
.uri(TEST_RESOURCE_URI)
.name("Test Resource")
.mimeType("text/plain")
.description("Test resource description")
.build();
McpServerFeatures.SyncResourceSpecification specification = new McpServerFeatures.SyncResourceSpecification(
resource, (exchange, req) -> new ReadResourceResult(List.of()));

Expand All @@ -377,7 +395,7 @@ void testListResources() {
assertThat(resources).hasSize(1);
assertThat(resources.get(0).uri()).isEqualTo(TEST_RESOURCE_URI);

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -386,15 +404,19 @@ void testRemoveResource() {
.capabilities(ServerCapabilities.builder().resources(true, false).build())
.build();

Resource resource = new Resource(TEST_RESOURCE_URI, "Test Resource", "text/plain", "Test resource description",
null);
Resource resource = Resource.builder()
.uri(TEST_RESOURCE_URI)
.name("Test Resource")
.mimeType("text/plain")
.description("Test resource description")
.build();
McpServerFeatures.SyncResourceSpecification specification = new McpServerFeatures.SyncResourceSpecification(
resource, (exchange, req) -> new ReadResourceResult(List.of()));

mcpSyncServer.addResource(specification);
assertThatCode(() -> mcpSyncServer.removeResource(TEST_RESOURCE_URI)).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -407,7 +429,7 @@ void testRemoveNonexistentResource() {
// as per the new implementation that just logs a warning
assertThatCode(() -> mcpSyncServer.removeResource("nonexistent://resource")).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

// ---------------------------------------
Expand All @@ -432,7 +454,7 @@ void testAddResourceTemplate() {

assertThatCode(() -> mcpSyncServer.addResourceTemplate(specification)).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand Down Expand Up @@ -474,7 +496,7 @@ void testRemoveResourceTemplate() {

assertThatCode(() -> mcpSyncServer.removeResourceTemplate("test://template/{id}")).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -496,7 +518,7 @@ void testRemoveNonexistentResourceTemplate() {
assertThatCode(() -> mcpSyncServer.removeResourceTemplate("nonexistent://template/{id}"))
.doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -520,7 +542,7 @@ void testListResourceTemplates() {

assertThat(templates).isNotNull();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

// ---------------------------------------
Expand All @@ -531,9 +553,9 @@ void testListResourceTemplates() {
void testNotifyPromptsListChanged() {
var mcpSyncServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

assertThatCode(() -> mcpSyncServer.notifyPromptsListChanged()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::notifyPromptsListChanged).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand Down Expand Up @@ -584,7 +606,7 @@ void testRemovePrompt() {

assertThatCode(() -> mcpSyncServer.removePrompt(TEST_PROMPT_NAME)).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

@Test
Expand All @@ -595,7 +617,7 @@ void testRemoveNonexistentPrompt() {

assertThatCode(() -> mcpSyncServer.removePrompt("nonexistent://template/{id}")).doesNotThrowAnyException();

assertThatCode(() -> mcpSyncServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(mcpSyncServer::closeGracefully).doesNotThrowAnyException();
}

// ---------------------------------------
Expand All @@ -617,7 +639,7 @@ void testRootsChangeHandlers() {
}))
.build();
assertThat(singleConsumerServer).isNotNull();
assertThatCode(() -> singleConsumerServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(singleConsumerServer::closeGracefully).doesNotThrowAnyException();
onClose();

// Test with multiple consumers
Expand All @@ -633,7 +655,7 @@ void testRootsChangeHandlers() {
.build();

assertThat(multipleConsumersServer).isNotNull();
assertThatCode(() -> multipleConsumersServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(multipleConsumersServer::closeGracefully).doesNotThrowAnyException();
onClose();

// Test error handling
Expand All @@ -644,14 +666,14 @@ void testRootsChangeHandlers() {
.build();

assertThat(errorHandlingServer).isNotNull();
assertThatCode(() -> errorHandlingServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(errorHandlingServer::closeGracefully).doesNotThrowAnyException();
onClose();

// Test without consumers
var noConsumersServer = prepareSyncServerBuilder().serverInfo("test-server", "1.0.0").build();

assertThat(noConsumersServer).isNotNull();
assertThatCode(() -> noConsumersServer.closeGracefully()).doesNotThrowAnyException();
assertThatCode(noConsumersServer::closeGracefully).doesNotThrowAnyException();
}

}
Loading
Loading