Conversation
Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/bb9210b4-2aa3-475c-9c4f-5b2318a436f5 Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
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
sessionIdinjection 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
| private static final ObjectMapper MAPPER = JsonRpcClient.getObjectMapper(); | ||
|
|
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
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.
| 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()); |
edburns
left a comment
There was a problem hiding this comment.
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:
-
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.
-
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. -
No separate test source root needed: Your
build-helper-maven-pluginalready 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. -
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. ✅✅
- 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
GeneratedEventTypesCoverageTestmust reside incom.github.copilot.sdk.generated. Then you don't needimport com.github.copilot.sdk.generated.*; - Your new classes
GeneratedRpcRecordsCoverageTestandGeneratedRpcRecordsCoverageTestmust reside incom.github.copilot.sdk.generated.rpc. Then you don't need all those imports of the classes in that package.
Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/0c4c84ed-1cb5-4e79-954f-633c61e99a87 Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Done in commit 863eba8. All three test files have been moved to mirror the package structure of the production code:
All 366 unit tests still pass. |
edburns
left a comment
There was a problem hiding this comment.
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.
Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/b7de171b-6625-4ab4-b222-4858b0cacfc2 Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Fixed in commit ed3307f.
|
edburns
left a comment
There was a problem hiding this comment.
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>
Root cause: a race condition in How it happened: Fix (commit 4b19e4b): Changed |
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.sendAndWaitthat caused an intermittent test failure.Before the change?
src/generated/java/(RPC API classes, param/result records, event types) had near-zero test coveragecom.github.copilot.sdkrather than mirroring the package structure of the code under testTestUtilwas package-private, preventing reuse from subpackages/tmp/paths and POSIX file-mode values made tests non-portable on WindowsCopilotSession.sendAndWaitcontained a race condition: its internalSessionIdleEventhandler calledfuture.whenComplete(fn)which ranfnsynchronously on the event-dispatch thread mid-loop, causing the calling thread to unblock before other registeredsession.on()listeners had processed the eventAfter 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 inSessionRpc/ServerRpcdispatches the correct JSON-RPC method name and injectssessionIdcorrectly, using aStubCaller. 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 existingSessionEventDeserializationTest. CoversCapabilitiesChanged,Elicitation*,ExitPlanMode*,ExternalTool*,McpOauth*,Permission*,Sampling*,SessionContext,SessionCustomAgents,SessionExtensions,SessionMcpServer*,SessionSkills/Tools/TitleChanged,UserInput*,SubagentDeselected,CommandsChanged, and more. Includes enumfromValue()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
generatedsource tree remains exclusively for auto-generated code.TestUtilis nowpublicand has a newtempPath(String filename)helper that resolves filenames underSystem.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.sendAndWaitrace condition fixed: changedfuture.whenComplete(fn)tofuture.whenCompleteAsync(fn, timeoutScheduler). The cleanup andresult.complete(r)call are now submitted to the session's dedicatedtimeoutSchedulerthread rather than running synchronously on the event-dispatch thread. This allowsdispatchEvent()to finish calling all registered handlers (including caller-providedsession.on()listeners) beforeresult.get()returns to the caller, eliminating the intermittent failure inCopilotSessionTest.testSendAndWaitBlocksUntilSessionIdleAndReturnsFinalAssistantMessage.Pull request checklist
mvn spotless:applyhas been run to format the codemvn clean verifypasses locallyDoes this introduce a breaking change?