Skip to content

Fix Java tool-processor test generation and stabilize session-id test#1799

Open
edburns wants to merge 4 commits into
edburns/1682-java-tool-ergonomics-review-draft-01from
edburns/1682-java-tool-ergonomics-review-draft-02
Open

Fix Java tool-processor test generation and stabilize session-id test#1799
edburns wants to merge 4 commits into
edburns/1682-java-tool-ergonomics-review-draft-01from
edburns/1682-java-tool-ergonomics-review-draft-02

Conversation

@edburns

@edburns edburns commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Address the Java test failures observed in the Java 17 surefire/failsafe run by fixing how annotation-processing output is discovered in CopilotToolProcessor tests and by hardening one timing-sensitive session test.

Changes included:

  • CopilotToolProcessor: resolve @copilotTool elements via TypeElement lookup and reuse that element list through validation and generation passes, making annotation discovery robust across compiler/module contexts.

  • CopilotToolProcessorTest: force annotation processing in the in-memory compile harness (-proc:full, explicit processor), close the file manager with try-with-resources, and add a collecting forwarding file manager that captures generated source content from getJavaFileForOutput to avoid missing generated CopilotToolMeta classes in tests.

  • CopilotSessionTest#testShouldGetLastSessionId: add bounded retry for session creation (including timeout and execution-timeout-cause handling) to absorb transient startup delays while preserving failure behavior on persistent errors.

Result:

  • CopilotToolProcessorTest now consistently observes generated CopilotToolMeta output and passes.

  • The full requested Maven workflow (jacoco prepare/report + surefire + failsafe under Java 17, with prior Java 25 compile) completes successfully.

Address the Java test failures observed in the Java 17 surefire/failsafe run by fixing how annotation-processing output is discovered in CopilotToolProcessor tests and by hardening one timing-sensitive session test.

Changes included:

- CopilotToolProcessor: resolve @copilotTool elements via TypeElement lookup and reuse that element list through validation and generation passes, making annotation discovery robust across compiler/module contexts.

- CopilotToolProcessorTest: force annotation processing in the in-memory compile harness (-proc:full, explicit processor), close the file manager with try-with-resources, and add a collecting forwarding file manager that captures generated source content from getJavaFileForOutput to avoid missing generated CopilotToolMeta classes in tests.

- CopilotSessionTest#testShouldGetLastSessionId: add bounded retry for session creation (including timeout and execution-timeout-cause handling) to absorb transient startup delays while preserving failure behavior on persistent errors.

Result:

- CopilotToolProcessorTest now consistently observes generated CopilotToolMeta output and passes.

- The full requested Maven workflow (jacoco prepare/report + surefire + failsafe under Java 17, with prior Java 25 compile) completes successfully.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edburns edburns requested a review from a team as a code owner June 25, 2026 16:38
Copilot AI review requested due to automatic review settings June 25, 2026 16:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses Java test instability by making the CopilotToolProcessor’s annotation discovery more robust, improving the in-memory compilation harness so generated $$CopilotToolMeta sources are reliably observed in tests, and adding timeout/retry logic to a timing-sensitive session-id test.

Changes:

  • CopilotToolProcessor: resolve @CopilotTool-annotated elements via TypeElement lookup and reuse the discovered element list across validation + generation.
  • CopilotToolProcessorTest: force annotation processing (-proc:full + explicit -processor) and capture generated source content via a forwarding JavaFileManager wrapper, with a disk-scan fallback.
  • CopilotSessionTest: add bounded retry + timeout handling around session creation in testShouldGetLastSessionId.
Show a summary per file
File Description
java/src/test/java/com/github/copilot/tool/CopilotToolProcessorTest.java Makes the compile harness reliably run the processor and capture generated $$CopilotToolMeta sources.
java/src/test/java/com/github/copilot/CopilotSessionTest.java Adds timeout/retry around session creation to reduce test flakiness.
java/src/main/java/com/github/copilot/tool/CopilotToolProcessor.java Stabilizes annotation discovery by resolving @CopilotTool via TypeElement and reusing the element set.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment thread java/src/test/java/com/github/copilot/CopilotSessionTest.java
@github-actions

This comment has been minimized.

The retry on session creation uses `future.get(timeout)` but does not cancel the in-flight `createSession` future when a timeout occurs. If attempt 1 eventually completes after attempt 2 starts, it can leave an orphaned session registered in the client (and potentially race `getLastSessionId` persistence), reintroducing flakiness and leaking resources. Capture the future and cancel it on timeout before retrying.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR is focused on Java-internal bug fixes with no cross-SDK consistency concerns.

Changes analyzed:

File Scope Assessment
CopilotToolProcessor.java Java-only Annotation processing is Java-specific — no equivalent needed in other SDKs
CopilotSessionTest.java Java test only Timing-sensitive retry is Java test infrastructure — other SDKs have their own test stability mechanisms
CopilotToolProcessorTest.java Java test only ForwardingJavaFileManager / in-memory compile harness is Java-specific
test/snapshots/session/should_abort_a_session.yaml Shared (all SDKs) Additive: adds a 3rd conversation scenario where both tool calls return "The execution of this tool, or a previous tool was interrupted." — differs from conversation 2 where toolcall_0 returns "Tool 'report_intent' does not exist.". Other SDKs that match the existing conversations are unaffected.

No action required in other SDK implementations (Node.js, Python, Go, .NET, Rust). The abort-session test already exists in all six SDKs and the snapshot addition only covers an additional valid response pattern.

Generated by SDK Consistency Review Agent for issue #1799 · sonnet46 876.5K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants