Skip to content

Add automated tests for generated RPC code to restore JaCoCo coverage ≥85%#85

Merged
edburns merged 6 commits intomainfrom
copilot/add-automated-tests-for-generated-code
Apr 20, 2026
Merged

Add automated tests for generated RPC code to restore JaCoCo coverage ≥85%#85
edburns merged 6 commits intomainfrom
copilot/add-automated-tests-for-generated-code

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 20, 2026

Coverage dropped from 84.4% → 69.2% after PR #81 added ~120 generated RPC files with no tests. This PR adds 222 unit tests targeting the generated code directly, fixes cross-platform portability issues in the new tests, and fixes a pre-existing race condition in CopilotSession.sendAndWait that caused an intermittent test failure.

Before the change?

  • Generated code in src/generated/java/ (RPC API classes, param/result records, event types) had near-zero test coverage
  • JaCoCo badge: 69.2%
  • Tests for generated code were placed in com.github.copilot.sdk rather than mirroring the package structure of the code under test
  • TestUtil was package-private, preventing reuse from subpackages
  • Hardcoded /tmp/ paths and POSIX file-mode values made tests non-portable on Windows
  • CopilotSession.sendAndWait contained a race condition: its internal SessionIdleEvent handler called future.whenComplete(fn) which ran fn synchronously on the event-dispatch thread mid-loop, causing the calling thread to unblock before other registered session.on() listeners had processed the event

After the change?

Three new test classes, all pure unit tests (no network/CLI required), placed in packages that mirror the production code they test:

  • com.github.copilot.sdk.generated.rpc.GeneratedRpcApiCoverageTest (47 tests) — verifies every API method in SessionRpc/ServerRpc dispatches the correct JSON-RPC method name and injects sessionId correctly, using a StubCaller. Covers all 15+ session namespaces: mode, plan, workspace, fleet, skills, mcp, plugins, extensions, tools, commands, ui, permissions, shell, history, usage, agent, log.

  • com.github.copilot.sdk.generated.GeneratedEventTypesCoverageTest (52 tests) — JSON deserialization tests for the 34 event types not covered by the existing SessionEventDeserializationTest. Covers CapabilitiesChanged, Elicitation*, ExitPlanMode*, ExternalTool*, McpOauth*, Permission*, Sampling*, SessionContext, SessionCustomAgents, SessionExtensions, SessionMcpServer*, SessionSkills/Tools/TitleChanged, UserInput*, SubagentDeselected, CommandsChanged, and more. Includes enum fromValue() round-trips and invalid-value exception cases.

  • com.github.copilot.sdk.generated.rpc.GeneratedRpcRecordsCoverageTest (123 tests) — constructor/accessor tests for all ~100 generated param/result records, including nested records and enum variants (e.g. SessionModeGetResult.Mode, SessionShellKillParams.Signal, SessionFsReaddirWithTypesResult.EntriesItemType).

Package alignment means tests no longer need cross-package imports for the classes they exercise, and the generated source tree remains exclusively for auto-generated code.

TestUtil is now public and has a new tempPath(String filename) helper that resolves filenames under System.getProperty("java.io.tmpdir"), making all filesystem-path test values portable across POSIX and Windows. All hardcoded /tmp/ paths and the POSIX-specific file-mode value have been replaced accordingly.

CopilotSession.sendAndWait race condition fixed: changed future.whenComplete(fn) to future.whenCompleteAsync(fn, timeoutScheduler). The cleanup and result.complete(r) call are now submitted to the session's dedicated timeoutScheduler thread rather than running synchronously on the event-dispatch thread. This allows dispatchEvent() to finish calling all registered handlers (including caller-provided session.on() listeners) before result.get() returns to the caller, eliminating the intermittent failure in CopilotSessionTest.testSendAndWaitBlocksUntilSessionIdleAndReturnsFinalAssistantMessage.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • mvn spotless:apply has been run to format the code
  • mvn clean verify passes locally

Does this introduce a breaking change?

  • Yes
  • No

Copilot AI linked an issue Apr 20, 2026 that may be closed by this pull request
1 task
Copilot AI changed the title [WIP] Add automated tests for generated code to improve JaCoCo coverage Add automated tests for generated RPC code to restore JaCoCo coverage ≥85% Apr 20, 2026
Copilot AI requested a review from edburns April 20, 2026 17:05
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@edburns edburns marked this pull request as ready for review April 20, 2026 17:29
Copilot AI review requested due to automatic review settings April 20, 2026 17:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds targeted unit tests to increase JaCoCo coverage for newly generated JSON-RPC/event code (introduced in #81) without requiring any network/CLI integration.

Changes:

  • Add RPC API wrapper coverage tests to validate method name dispatch and sessionId injection behavior.
  • Add JSON deserialization coverage tests for generated session event types not covered elsewhere.
  • Add constructor/accessor/enum-variant tests for generated RPC param/result record types to boost coverage.
Show a summary per file
File Description
src/test/java/com/github/copilot/sdk/GeneratedRpcApiCoverageTest.java Verifies generated ServerRpc/SessionRpc wrappers call the expected JSON-RPC method names and inject/merge sessionId correctly using a stub caller.
src/test/java/com/github/copilot/sdk/GeneratedEventTypesCoverageTest.java Adds Jackson deserialization tests for additional generated SessionEvent subtypes and enum round-trips/invalid values.
src/test/java/com/github/copilot/sdk/GeneratedRpcRecordsCoverageTest.java Adds record construction/accessor and enum fromValue() coverage for generated RPC param/result types (including nested records).

Copilot's findings

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

Comment on lines +134 to +135
private static final ObjectMapper MAPPER = JsonRpcClient.getObjectMapper();

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

MAPPER is declared but never used in this test class. Please remove the field (and the ObjectMapper import) or use it in a serialization/deserialization assertion so the constant isn’t dead code.

Copilot uses AI. Check for mistakes.
Comment on lines +341 to +345
var params = new SessionFsWriteFileParams("sess-21", "/tmp/out.txt", "content here", 0644.0);
assertEquals("sess-21", params.sessionId());
assertEquals("/tmp/out.txt", params.path());
assertEquals("content here", params.content());
assertEquals(0644.0, params.mode());
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

The literal 0644.0 looks like a Unix octal file mode, but in Java it reads as the decimal value 644.0 (octal notation doesn’t apply here). To avoid confusion, use an explicit decimal value (e.g., 420.0 for 0o644) or add a clarifying comment.

Suggested change
var params = new SessionFsWriteFileParams("sess-21", "/tmp/out.txt", "content here", 0644.0);
assertEquals("sess-21", params.sessionId());
assertEquals("/tmp/out.txt", params.path());
assertEquals("content here", params.content());
assertEquals(0644.0, params.mode());
var params = new SessionFsWriteFileParams("sess-21", "/tmp/out.txt", "content here", 644.0);
assertEquals("sess-21", params.sessionId());
assertEquals("/tmp/out.txt", params.path());
assertEquals("content here", params.content());
assertEquals(644.0, params.mode());

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@edburns edburns left a comment

Choose a reason for hiding this comment

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

@copilot

Consider this guidance:


The tests should go in src/test/java/com/github/copilot/sdk/generated/ and sub package rpc, as appropriate.

Here's the reasoning from Java best practices:

  1. Standard Maven convention: Tests always live under java, mirroring the package structure of the code they test. The test source root doesn't change based on where the production source root is — it's always java.

  2. Package alignment: The generated code is in package com.github.copilot.sdk.generated, so the tests go in the same package under the test tree: src/test/java/com/github/copilot/sdk/generated/. This gives tests package-private access to the generated classes if needed.

  3. No separate test source root needed: Your build-helper-maven-plugin already adds java as a compile source root. Maven's default test source root (java) can already see all compile-scope classes, including generated ones. No POM changes required.

  4. Clear ownership boundary: Placing tests in java (not generated) makes it obvious they are hand-maintained artifacts, not code-gen output. The generated tree remains exclusively for auto-generated code, while java is where all human/LLM-authored tests live — both for java code and java code.

This also aligns with how the existing tests already work — e.g., tests in sdk test code from sdk.


✅✅ Tests must reside under src/test/java. You have that already. ✅✅

  1. Standard Maven convention: Tests always live under java, mirroring the package structure of the code they test. The test source root doesn't change based on where the production source root is — it's always java. ❌❌You violate this❌❌.

Here's how to fix it.

  • Your new class GeneratedEventTypesCoverageTest must reside in com.github.copilot.sdk.generated. Then you don't need import com.github.copilot.sdk.generated.*;
  • Your new classes GeneratedRpcRecordsCoverageTest and GeneratedRpcRecordsCoverageTest must reside in com.github.copilot.sdk.generated.rpc. Then you don't need all those imports of the classes in that package.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 20, 2026

@copilot

Consider this guidance:...

Done in commit 863eba8. All three test files have been moved to mirror the package structure of the production code:

  • GeneratedEventTypesCoverageTestsrc/test/java/com/github/copilot/sdk/generated/ (package com.github.copilot.sdk.generated) — the wildcard import com.github.copilot.sdk.generated.* is gone.
  • GeneratedRpcApiCoverageTestsrc/test/java/com/github/copilot/sdk/generated/rpc/ (package com.github.copilot.sdk.generated.rpc) — all 28 individual import com.github.copilot.sdk.generated.rpc.* lines are gone.
  • GeneratedRpcRecordsCoverageTestsrc/test/java/com/github/copilot/sdk/generated/rpc/ (package com.github.copilot.sdk.generated.rpc) — all 110 generated-rpc imports are gone, along with the unused MAPPER field and the 0644.0644.0 fix.

All 366 unit tests still pass.

Copilot AI requested a review from edburns April 20, 2026 18:18
Copy link
Copy Markdown
Collaborator

@edburns edburns left a comment

Choose a reason for hiding this comment

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

@copilot

I see some POSIXisms in here. For example, assuming /tmp exists. Also permissions models.

These tests need to run on POSIX and Windows.

Please fix any POSIXisms so they take the OS into account.

There are other tests that do this. For example, CliServerManagerTest and E2ETestContext.

Don't forget, there is a TestUtil class where you can put commonly accessed functionality so as not to introduce the dreadful DRY violation.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 20, 2026

@copilot

I see some POSIXisms in here. For example, assuming /tmp exists. Also permissions models....

Fixed in commit ed3307f.

  • TestUtil is now public and has a new public static String tempPath(String filename) helper that resolves filenames under System.getProperty("java.io.tmpdir") — correct on both POSIX and Windows.
  • All 5 hardcoded /tmp/ paths in GeneratedRpcRecordsCoverageTest now call TestUtil.tempPath("filename").
  • The POSIX-specific mode value (644.0) is replaced with null — the field is declared optional (Double mode) in the generated record, so null is the right cross-platform value when there's no filesystem-mode concept.

Copilot AI requested a review from edburns April 20, 2026 18:46
Copy link
Copy Markdown
Collaborator

@edburns edburns left a comment

Choose a reason for hiding this comment

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

@copilot

You're almost there!

But fix the test failures https://github.com/github/copilot-sdk-java/actions/runs/24684164293/job/72197130020?pr=85

❌❌Don't cheat. If this is a legitimate test failure, I need to know the root cause. Don't just make the test pass for the sake of passing.

…est-thread wakeup mid-dispatch

Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/e7c09bc7-4fc9-4619-a0d9-d1bed96104b9

Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 20, 2026

@copilot

You're almost there! ...

Root cause: a race condition in CopilotSession.sendAndWait, not in my new test files.

How it happened: sendAndWait registers an internal SessionIdleEvent handler alongside any caller-registered session.on() handlers. All handlers live in a ConcurrentHashMap.newKeySet() whose iteration order is non-deterministic. When SessionIdleEvent was dispatched, if sendAndWait's internal handler happened to run first, it called future.complete()future.whenComplete(fn) ran synchronously on the event-dispatch threadresult.complete(r) woke up the test thread mid-loop, before the test's global listener had processed session.idle. Hence events.contains("session.idle") returned false.

Fix (commit 4b19e4b): Changed future.whenComplete(fn)future.whenCompleteAsync(fn, timeoutScheduler). The cleanup/completion work is now submitted to the session's existing timeoutScheduler thread instead of running inline. The event-dispatch thread finishes iterating all handlers (including the test's session.on() listener) before the scheduler thread completes result, so result.get() only returns after all listeners have seen session.idle.

Copilot AI requested a review from edburns April 20, 2026 19:58
@edburns edburns merged commit 005e275 into main Apr 20, 2026
10 checks passed
@edburns edburns deleted the copilot/add-automated-tests-for-generated-code branch April 20, 2026 20:05
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.

[MAINT]:

3 participants