Skip to content

Commit 24cb65d

Browse files
Copilotedburns
andauthored
Address PR review feedback: fix invoke logging order, fix Javadoc, update @disabled messages
- Move success log in JsonRpcClient.invoke after treeToValue deserialization so that a schema mismatch triggers only the failure log, not both - Fix Javadoc link text from "CopilotClientOptions.Telemetry" to "CopilotClientOptions.TelemetryConfig" in SessionConfig and ResumeSessionConfig - Update @disabled annotations with accurate root cause: tests pass in isolation but time out in full suite due to test interaction (confirmed by running full suite with tests re-enabled) Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
1 parent 7bc2ff6 commit 24cb65d

5 files changed

Lines changed: 16 additions & 11 deletions

File tree

src/main/java/com/github/copilot/sdk/JsonRpcClient.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,14 +124,19 @@ public <T> CompletableFuture<T> invoke(String method, Object params, Class<T> re
124124

125125
return future.thenApply(result -> {
126126
try {
127+
if (responseType == Void.class || responseType == void.class) {
128+
LoggingHelpers.logTiming(LOG, Level.FINE,
129+
"JsonRpc.invoke JSON-RPC request finished. Elapsed={Elapsed}, Method=" + method
130+
+ ", RequestId=" + id + ", Status=Succeeded",
131+
timingNanos);
132+
return null;
133+
}
134+
T value = MAPPER.treeToValue(result, responseType);
127135
LoggingHelpers.logTiming(LOG, Level.FINE,
128136
"JsonRpc.invoke JSON-RPC request finished. Elapsed={Elapsed}, Method=" + method + ", RequestId="
129137
+ id + ", Status=Succeeded",
130138
timingNanos);
131-
if (responseType == Void.class || responseType == void.class) {
132-
return null;
133-
}
134-
return MAPPER.treeToValue(result, responseType);
139+
return value;
135140
} catch (JsonProcessingException e) {
136141
throw new CompletionException(e);
137142
}

src/main/java/com/github/copilot/sdk/json/ResumeSessionConfig.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,8 @@ public ResumeSessionConfig setProvider(ProviderConfig provider) {
237237
* custom {@link ProviderConfig} (BYOK) is configured, session telemetry is
238238
* always disabled regardless of this setting. This is independent of
239239
* {@link com.github.copilot.sdk.json.CopilotClientOptions#getTelemetry()
240-
* CopilotClientOptions.Telemetry}, which configures OpenTelemetry export for
241-
* observability.
240+
* CopilotClientOptions.TelemetryConfig}, which configures OpenTelemetry export
241+
* for observability.
242242
*
243243
* @return whether session telemetry is enabled
244244
*/

src/main/java/com/github/copilot/sdk/json/SessionConfig.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,8 @@ public SessionConfig setProvider(ProviderConfig provider) {
291291
* custom {@link ProviderConfig} (BYOK) is configured, session telemetry is
292292
* always disabled regardless of this setting. This is independent of
293293
* {@link com.github.copilot.sdk.json.CopilotClientOptions#getTelemetry()
294-
* CopilotClientOptions.Telemetry}, which configures OpenTelemetry export for
295-
* observability.
294+
* CopilotClientOptions.TelemetryConfig}, which configures OpenTelemetry export
295+
* for observability.
296296
*
297297
* @return whether session telemetry is enabled
298298
*/

src/test/java/com/github/copilot/sdk/CopilotSessionTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ void testShouldResumeSessionUsingTheSameClient() throws Exception {
307307
* @see Snapshot: session/should_resume_a_session_using_a_new_client
308308
*/
309309
@Test
310-
@Disabled("Flaky: multi-client session resume times out in snapshot-based test harness")
310+
@Disabled("Passes in isolation but times out in full suite due to test interaction (state leakage or process contention)")
311311
void testShouldResumeSessionUsingNewClient() throws Exception {
312312
ctx.configureForTest("session", "should_resume_a_session_using_a_new_client");
313313

src/test/java/com/github/copilot/sdk/StreamingFidelityTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ void testShouldNotProduceDeltasWhenStreamingIsDisabled() throws Exception {
140140
* @see Snapshot: streaming_fidelity/should_produce_deltas_after_session_resume
141141
*/
142142
@Test
143-
@Disabled("Flaky: multi-client session resume times out in snapshot-based test harness")
143+
@Disabled("Passes in isolation but times out in full suite due to test interaction (state leakage or process contention)")
144144
void testShouldProduceDeltasAfterSessionResume() throws Exception {
145145
ctx.configureForTest("streaming_fidelity", "should_produce_deltas_after_session_resume");
146146

@@ -194,7 +194,7 @@ void testShouldProduceDeltasAfterSessionResume() throws Exception {
194194
* streaming_fidelity/should_not_produce_deltas_after_session_resume_with_streaming_disabled
195195
*/
196196
@Test
197-
@Disabled("Flaky: multi-client session resume times out in snapshot-based test harness")
197+
@Disabled("Passes in isolation but times out in full suite due to test interaction (state leakage or process contention)")
198198
void testShouldNotProduceDeltasAfterSessionResumeWithStreamingDisabled() throws Exception {
199199
ctx.configureForTest("streaming_fidelity",
200200
"should_not_produce_deltas_after_session_resume_with_streaming_disabled");

0 commit comments

Comments
 (0)