From 100472d61325ded44e24fdb32685a61cf70c692f Mon Sep 17 00:00:00 2001 From: Kabir Khan Date: Tue, 11 Aug 2026 17:02:14 +0100 Subject: [PATCH] chore: address PR review cleanup from #1039 and #1040 - Fix import ordering of TaskQueryParams in DefaultRequestHandlerTest - Remove fragile reflection-based test for negative historyLength guard (the two public-API tests already exercise the limitTaskHistory path) - Trim verbose 4-line comment on negative historyLength guard to one line - Fix Javadoc on bufferFlushDelayMs(): negative values are clamped to 0, not returned as the default - Replace fully-qualified class names with imports in EventQueueTest Co-Authored-By: Claude Opus 4.6 (1M context) --- .../sdk/server/events/EventConsumer.java | 4 ++-- .../DefaultRequestHandler.java | 5 +--- .../sdk/server/events/EventQueueTest.java | 23 +++++++++++-------- .../DefaultRequestHandlerTest.java | 19 +-------------- 4 files changed, 17 insertions(+), 34 deletions(-) diff --git a/server-common/src/main/java/org/a2aproject/sdk/server/events/EventConsumer.java b/server-common/src/main/java/org/a2aproject/sdk/server/events/EventConsumer.java index 8719ff611..531421aca 100644 --- a/server-common/src/main/java/org/a2aproject/sdk/server/events/EventConsumer.java +++ b/server-common/src/main/java/org/a2aproject/sdk/server/events/EventConsumer.java @@ -58,8 +58,8 @@ public class EventConsumer { /** * Returns the configured buffer-flush delay in milliseconds. * - *

Reads the {@value #BUFFER_FLUSH_DELAY_MS_PROPERTY} system property; values that - * are absent, non-numeric, or negative fall back to the default.

+ *

Reads the {@value #BUFFER_FLUSH_DELAY_MS_PROPERTY} system property; absent or + * non-numeric values fall back to the default, negative values are clamped to 0.

* * @return the delay in milliseconds (never negative) */ diff --git a/server-common/src/main/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandler.java b/server-common/src/main/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandler.java index 0eea16db4..d41b1e9c0 100644 --- a/server-common/src/main/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandler.java +++ b/server-common/src/main/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandler.java @@ -420,10 +420,7 @@ public Task onGetTask(TaskQueryParams params, ServerCallContext context) throws * @return the task with limited history, or the original task if no limiting needed */ private static Task limitTaskHistory(Task task, @Nullable Integer historyLength) { - // A negative historyLength is invalid (TaskQueryParams rejects it at construction, but - // guard defensively here to avoid IndexOutOfBoundsException in subList below). Consistent - // with the Python/JS SDKs, a negative value leaves the history untouched; 0 means an - // empty history, and N >= history size means no limiting is needed. + // Negative values leave history untouched (defensive guard against IndexOutOfBoundsException) if (task.history() == null || historyLength == null || historyLength < 0 || historyLength >= task.history().size()) { return task; diff --git a/server-common/src/test/java/org/a2aproject/sdk/server/events/EventQueueTest.java b/server-common/src/test/java/org/a2aproject/sdk/server/events/EventQueueTest.java index 6df08a7e1..98953de05 100644 --- a/server-common/src/test/java/org/a2aproject/sdk/server/events/EventQueueTest.java +++ b/server-common/src/test/java/org/a2aproject/sdk/server/events/EventQueueTest.java @@ -9,9 +9,15 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import java.lang.reflect.Field; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.BlockingQueue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -110,9 +116,9 @@ private EventQueue createQueueWithEventBus(String taskId, @Nullable TaskStreamLi */ private void waitForEventProcessing(Runnable action) throws InterruptedException { CountDownLatch processingLatch = new CountDownLatch(1); - mainEventBusProcessor.setCallback(new org.a2aproject.sdk.server.events.MainEventBusProcessorCallback() { + mainEventBusProcessor.setCallback(new MainEventBusProcessorCallback() { @Override - public void onEventProcessed(String taskId, org.a2aproject.sdk.spec.Event event) { + public void onEventProcessed(String taskId, Event event) { processingLatch.countDown(); } @@ -711,10 +717,9 @@ public void testChildQueueIsBoundedByParentQueueSize() throws Exception { .build(); EventQueue childQueue = mainQueue.tap(); - java.lang.reflect.Field queueField = EventQueue.ChildQueue.class.getDeclaredField("queue"); + Field queueField = EventQueue.ChildQueue.class.getDeclaredField("queue"); queueField.setAccessible(true); - java.util.concurrent.BlockingQueue childDeque = - (java.util.concurrent.BlockingQueue) queueField.get(childQueue); + BlockingQueue childDeque = (BlockingQueue) queueField.get(childQueue); // The child queue must be bounded by the parent's configured capacity: // an unbounded deque would let a slow subscriber grow memory without limit. @@ -723,11 +728,9 @@ public void testChildQueueIsBoundedByParentQueueSize() throws Exception { @Test public void testSemaphorePermitReleasedWhenSubmitFails() { - MainEventBus failingBus = org.mockito.Mockito.mock(MainEventBus.class); - org.mockito.Mockito.doThrow(new RuntimeException("submit failed")) - .when(failingBus).submit(org.mockito.ArgumentMatchers.anyString(), - org.mockito.ArgumentMatchers.any(), - org.mockito.ArgumentMatchers.any()); + MainEventBus failingBus = mock(MainEventBus.class); + doThrow(new RuntimeException("submit failed")) + .when(failingBus).submit(anyString(), any(), any()); EventQueue mainQueue = EventQueueUtil.getEventQueueBuilder(failingBus) .taskId(TASK_ID) diff --git a/server-common/src/test/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandlerTest.java b/server-common/src/test/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandlerTest.java index 40a6b36a4..cf768231e 100644 --- a/server-common/src/test/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandlerTest.java +++ b/server-common/src/test/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandlerTest.java @@ -4,13 +4,11 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import java.lang.reflect.Method; import java.util.List; import java.util.Map; import java.util.Set; @@ -51,8 +49,8 @@ import org.a2aproject.sdk.spec.TaskPushNotificationConfig; import org.a2aproject.sdk.spec.TaskState; import org.a2aproject.sdk.spec.TaskStatus; -import org.a2aproject.sdk.spec.TaskStatusUpdateEvent; import org.a2aproject.sdk.spec.TaskQueryParams; +import org.a2aproject.sdk.spec.TaskStatusUpdateEvent; import org.a2aproject.sdk.spec.TextPart; import org.a2aproject.sdk.spec.UnsupportedOperationError; @@ -1173,21 +1171,6 @@ void testOnGetTaskHistoryLengthZeroReturnsEmptyHistory() throws Exception { assertTrue(result.history().isEmpty()); } - @Test - void testLimitTaskHistoryNegativeHistoryLengthReturnsTaskUnchanged() throws Exception { - Task task = taskWithHistory("task-hl-negative"); - - Method method = DefaultRequestHandler.class.getDeclaredMethod( - "limitTaskHistory", Task.class, Integer.class); - method.setAccessible(true); - Task result = (Task) method.invoke(null, task, -1); - - // A negative historyLength must not throw IndexOutOfBoundsException and must leave - // the history untouched (aligned with the Python/JS SDK semantics). - assertSame(task, result); - assertEquals(task.history(), result.history()); - } - private Task taskWithHistory(String id) { return Task.builder() .id(id)