Skip to content

Commit 6e2e991

Browse files
Copilotbrunoborges
andcommitted
Fix test and clone implementation per PR feedback
- Fix sessionConfigListIndependence test to properly validate list independence by mutating original list after cloning - Clone environment map in CopilotClientOptions for collection independence - Add copilotClientOptionsEnvironmentIndependence test to validate environment map cloning - Update CopilotClientOptions Javadoc to reflect environment map is now copied - Apply spotless formatting Co-authored-by: brunoborges <129743+brunoborges@users.noreply.github.com>
1 parent 063be94 commit 6e2e991

File tree

4 files changed

+47
-28
lines changed

4 files changed

+47
-28
lines changed

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,9 @@ public CopilotClientOptions setUseLoggedInUser(Boolean useLoggedInUser) {
328328
* Creates a shallow clone of this {@code CopilotClientOptions} instance.
329329
* <p>
330330
* Array properties (like {@code cliArgs}) are copied into new arrays so that
331-
* modifications to the clone do not affect the original. Other reference-type
332-
* properties (like {@code environment}) are shared between the original and
333-
* clone.
331+
* modifications to the clone do not affect the original. The
332+
* {@code environment} map is also copied to a new map instance. Other
333+
* reference-type properties are shared between the original and clone.
334334
*
335335
* @return a clone of this options instance
336336
*/
@@ -346,7 +346,7 @@ public CopilotClientOptions clone() {
346346
copy.logLevel = this.logLevel;
347347
copy.autoStart = this.autoStart;
348348
copy.autoRestart = this.autoRestart;
349-
copy.environment = this.environment;
349+
copy.environment = this.environment != null ? new java.util.HashMap<>(this.environment) : null;
350350
copy.githubToken = this.githubToken;
351351
copy.useLoggedInUser = this.useLoggedInUser;
352352
return copy;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -483,8 +483,8 @@ public ResumeSessionConfig setInfiniteSessions(InfiniteSessionConfig infiniteSes
483483
* Mutable collection properties are copied into new collection instances so
484484
* that modifications to those collections on the clone do not affect the
485485
* original. Other reference-type properties (like provider configuration,
486-
* system messages, hooks, infinite session configuration, and handlers) are
487-
* not deep-cloned; the original and the clone will share those objects.
486+
* system messages, hooks, infinite session configuration, and handlers) are not
487+
* deep-cloned; the original and the clone will share those objects.
488488
*
489489
* @return a clone of this config instance
490490
*/

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -520,8 +520,8 @@ public SessionConfig setConfigDir(String configDir) {
520520
* Mutable collection properties are copied into new collection instances so
521521
* that modifications to those collections on the clone do not affect the
522522
* original. Other reference-type properties (like provider configuration,
523-
* system messages, hooks, infinite session configuration, and handlers) are
524-
* not deep-cloned; the original and the clone will share those objects.
523+
* system messages, hooks, infinite session configuration, and handlers) are not
524+
* deep-cloned; the original and the clone will share those objects.
525525
*
526526
* @return a clone of this config instance
527527
*/

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

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@
77
import static org.junit.jupiter.api.Assertions.*;
88

99
import java.util.ArrayList;
10+
import java.util.HashMap;
1011
import java.util.List;
12+
import java.util.Map;
1113

1214
import org.junit.jupiter.api.Test;
1315

@@ -24,9 +26,9 @@ void copilotClientOptionsCloneBasic() {
2426
original.setCliPath("/usr/local/bin/copilot");
2527
original.setLogLevel("debug");
2628
original.setPort(9000);
27-
29+
2830
CopilotClientOptions cloned = original.clone();
29-
31+
3032
assertEquals(original.getCliPath(), cloned.getCliPath());
3133
assertEquals(original.getLogLevel(), cloned.getLogLevel());
3234
assertEquals(original.getPort(), cloned.getPort());
@@ -37,23 +39,40 @@ void copilotClientOptionsArrayIndependence() {
3739
CopilotClientOptions original = new CopilotClientOptions();
3840
String[] args = {"--flag1", "--flag2"};
3941
original.setCliArgs(args);
40-
42+
4143
CopilotClientOptions cloned = original.clone();
4244
cloned.getCliArgs()[0] = "--changed";
43-
45+
4446
assertEquals("--flag1", original.getCliArgs()[0]);
4547
assertEquals("--changed", cloned.getCliArgs()[0]);
4648
}
4749

50+
@Test
51+
void copilotClientOptionsEnvironmentIndependence() {
52+
CopilotClientOptions original = new CopilotClientOptions();
53+
Map<String, String> env = new HashMap<>();
54+
env.put("KEY1", "value1");
55+
original.setEnvironment(env);
56+
57+
CopilotClientOptions cloned = original.clone();
58+
59+
// Mutate the original environment map to test independence
60+
env.put("KEY2", "value2");
61+
62+
// The cloned config should be unaffected by mutations to the original map
63+
assertEquals(1, cloned.getEnvironment().size());
64+
assertEquals(2, original.getEnvironment().size());
65+
}
66+
4867
@Test
4968
void sessionConfigCloneBasic() {
5069
SessionConfig original = new SessionConfig();
5170
original.setSessionId("my-session");
5271
original.setModel("gpt-4o");
5372
original.setStreaming(true);
54-
73+
5574
SessionConfig cloned = original.clone();
56-
75+
5776
assertEquals(original.getSessionId(), cloned.getSessionId());
5877
assertEquals(original.getModel(), cloned.getModel());
5978
assertEquals(original.isStreaming(), cloned.isStreaming());
@@ -66,25 +85,25 @@ void sessionConfigListIndependence() {
6685
toolList.add("grep");
6786
toolList.add("bash");
6887
original.setAvailableTools(toolList);
69-
88+
7089
SessionConfig cloned = original.clone();
71-
72-
List<String> clonedTools = new ArrayList<>(cloned.getAvailableTools());
73-
clonedTools.add("web");
74-
cloned.setAvailableTools(clonedTools);
75-
76-
assertEquals(2, original.getAvailableTools().size());
77-
assertEquals(3, cloned.getAvailableTools().size());
90+
91+
// Mutate the original list directly to test independence
92+
toolList.add("web");
93+
94+
// The cloned config should be unaffected by mutations to the original list
95+
assertEquals(2, cloned.getAvailableTools().size());
96+
assertEquals(3, original.getAvailableTools().size());
7897
}
7998

8099
@Test
81100
void resumeSessionConfigCloneBasic() {
82101
ResumeSessionConfig original = new ResumeSessionConfig();
83102
original.setModel("o1");
84103
original.setStreaming(false);
85-
104+
86105
ResumeSessionConfig cloned = original.clone();
87-
106+
88107
assertEquals(original.getModel(), cloned.getModel());
89108
assertEquals(original.isStreaming(), cloned.isStreaming());
90109
}
@@ -94,9 +113,9 @@ void messageOptionsCloneBasic() {
94113
MessageOptions original = new MessageOptions();
95114
original.setPrompt("What is 2+2?");
96115
original.setMode("immediate");
97-
116+
98117
MessageOptions cloned = original.clone();
99-
118+
100119
assertEquals(original.getPrompt(), cloned.getPrompt());
101120
assertEquals(original.getMode(), cloned.getMode());
102121
}
@@ -106,11 +125,11 @@ void clonePreservesNullFields() {
106125
CopilotClientOptions opts = new CopilotClientOptions();
107126
CopilotClientOptions optsClone = opts.clone();
108127
assertNull(optsClone.getCliPath());
109-
128+
110129
SessionConfig cfg = new SessionConfig();
111130
SessionConfig cfgClone = cfg.clone();
112131
assertNull(cfgClone.getModel());
113-
132+
114133
MessageOptions msg = new MessageOptions();
115134
MessageOptions msgClone = msg.clone();
116135
assertNull(msgClone.getMode());

0 commit comments

Comments
 (0)