Skip to content

Commit 0dd362c

Browse files
shs96cshs96c
committed
Make *Options mirror Capabilities far better
Prior to this change, although all the `*Options` classes implemented `Capabilities`, they would not actually adhere to the contract. Notably, this meant that browser-specific capabilities, managed by the options class, would not be returned from `getCapabilityNames`, and calling `getCapability` on those browser-specific capabilities wouldn't return anything. We had originally worked around this by modifying the hashCode of options as part of the `hashCode` method. Now, we properly return all the capability names, and therefore all the methods that depend upon that (including `toString`, `hashCode`, `equals`, and `asMap`) now operate equivalently.
1 parent 7bf0e70 commit 0dd362c

File tree

11 files changed

+151
-58
lines changed

11 files changed

+151
-58
lines changed

‎java/client/src/org/openqa/selenium/ImmutableCapabilities.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ public boolean equals(Object o) {
194194

195195
@Override
196196
public String toString() {
197-
return SharedCapabilitiesMethods.toString(delegate);
197+
return SharedCapabilitiesMethods.toString(this);
198198
}
199199

200200
public static ImmutableCapabilities copyOf(Capabilities capabilities) {

‎java/client/src/org/openqa/selenium/MutableCapabilities.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,6 @@ public boolean equals(Object o) {
138138

139139
@Override
140140
public String toString() {
141-
return SharedCapabilitiesMethods.toString(caps);
141+
return SharedCapabilitiesMethods.toString(this);
142142
}
143143
}

‎java/client/src/org/openqa/selenium/PersistentCapabilities.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ public Set<String> getCapabilityNames() {
9797

9898
@Override
9999
public String toString() {
100-
return SharedCapabilitiesMethods.toString(asMap());
100+
return SharedCapabilitiesMethods.toString(this);
101101
}
102102

103103
@Override

‎java/client/src/org/openqa/selenium/SharedCapabilitiesMethods.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,9 @@ static void setCapability(Map<String, Object> caps, String key, Object value) {
7474
caps.put(key, value);
7575
}
7676

77-
static String toString(Map<String, Object> caps) {
77+
static String toString(Capabilities caps) {
7878
Map<Object, String> seen = new IdentityHashMap<>();
79-
return "Capabilities " + abbreviate(seen, caps);
79+
return "Capabilities " + abbreviate(seen, caps.asMap());
8080
}
8181

8282
private static String abbreviate(Map<Object, String> seen, Object stringify) {

‎java/client/src/org/openqa/selenium/chromium/ChromiumOptions.java

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@
1717

1818
package org.openqa.selenium.chromium;
1919

20-
import static java.util.Collections.unmodifiableList;
21-
import static java.util.Collections.unmodifiableMap;
22-
import static java.util.stream.Collectors.toList;
23-
2420
import org.openqa.selenium.Capabilities;
2521
import org.openqa.selenium.SessionNotCreatedException;
2622
import org.openqa.selenium.internal.Require;
@@ -32,13 +28,18 @@
3228
import java.util.ArrayList;
3329
import java.util.Arrays;
3430
import java.util.Base64;
31+
import java.util.Collections;
3532
import java.util.HashMap;
3633
import java.util.List;
3734
import java.util.Map;
38-
import java.util.Objects;
35+
import java.util.Set;
3936
import java.util.TreeMap;
4037
import java.util.stream.Stream;
4138

39+
import static java.util.Collections.unmodifiableList;
40+
import static java.util.Collections.unmodifiableMap;
41+
import static java.util.stream.Collectors.toList;
42+
4243
/**
4344
* Class to manage options specific to {@link ChromiumDriver}.
4445
*
@@ -67,10 +68,10 @@ public class ChromiumOptions<T extends ChromiumOptions<?>> extends AbstractDrive
6768
private final List<String> extensions = new ArrayList<>();
6869
private final Map<String, Object> experimentalOptions = new HashMap<>();
6970

70-
private final String CAPABILITY;
71+
private final String capabilityName;
7172

7273
public ChromiumOptions(String capabilityType, String browserType, String capability) {
73-
this.CAPABILITY = capability;
74+
this.capabilityName = capability;
7475
setCapability(capabilityType, browserType);
7576
}
7677

@@ -193,8 +194,16 @@ public T setHeadless(boolean headless) {
193194
}
194195

195196
@Override
196-
public Map<String, Object> asMap() {
197-
Map<String, Object> toReturn = new TreeMap<>(super.asMap());
197+
protected Set<String> getExtraCapabilityNames() {
198+
return Collections.singleton(capabilityName);
199+
}
200+
201+
@Override
202+
protected Object getExtraCapability(String capabilityName) {
203+
Require.nonNull("Capability name", capabilityName);
204+
if (!this.capabilityName.equals(capabilityName)) {
205+
return null;
206+
}
198207

199208
Map<String, Object> options = new TreeMap<>();
200209
experimentalOptions.forEach(options::put);
@@ -206,22 +215,20 @@ public Map<String, Object> asMap() {
206215
options.put("args", unmodifiableList(new ArrayList<>(args)));
207216

208217
options.put(
209-
"extensions",
210-
unmodifiableList(Stream.concat(
211-
extensionFiles.stream()
212-
.map(file -> {
213-
try {
214-
return Base64.getEncoder().encodeToString(Files.readAllBytes(file.toPath()));
215-
} catch (IOException e) {
216-
throw new SessionNotCreatedException(e.getMessage(), e);
217-
}
218-
}),
219-
extensions.stream()
220-
).collect(toList())));
221-
222-
toReturn.put(CAPABILITY, unmodifiableMap(options));
223-
224-
return unmodifiableMap(toReturn);
218+
"extensions",
219+
unmodifiableList(Stream.concat(
220+
extensionFiles.stream()
221+
.map(file -> {
222+
try {
223+
return Base64.getEncoder().encodeToString(Files.readAllBytes(file.toPath()));
224+
} catch (IOException e) {
225+
throw new SessionNotCreatedException(e.getMessage(), e);
226+
}
227+
}),
228+
extensions.stream()
229+
).collect(toList())));
230+
231+
return unmodifiableMap(options);
225232
}
226233

227234
protected void mergeInPlace(Capabilities capabilities) {

‎java/client/src/org/openqa/selenium/firefox/FirefoxOptions.java

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,10 @@
3636
import java.util.Map;
3737
import java.util.Objects;
3838
import java.util.Optional;
39+
import java.util.Set;
3940
import java.util.TreeMap;
41+
import java.util.TreeSet;
4042

41-
import static java.util.Collections.unmodifiableMap;
4243
import static java.util.stream.Collectors.toMap;
4344
import static org.openqa.selenium.firefox.FirefoxDriver.Capability.BINARY;
4445
import static org.openqa.selenium.firefox.FirefoxDriver.Capability.MARIONETTE;
@@ -342,13 +343,31 @@ private FirefoxOptions setFirefoxOption(Keys key, Object value) {
342343
}
343344

344345
@Override
345-
public Map<String, Object> asMap() {
346-
Map<String, Object> toReturn = new TreeMap<>(super.asMap());
347-
toReturn.put(FIREFOX_OPTIONS, firefoxOptions);
346+
protected Set<String> getExtraCapabilityNames() {
347+
Set<String> names = new TreeSet<>();
348+
349+
names.add(FIREFOX_OPTIONS);
348350
if (legacy) {
349-
toReturn.put(MARIONETTE, false);
351+
names.add(MARIONETTE);
352+
}
353+
354+
return Collections.unmodifiableSet(names);
355+
}
356+
357+
@Override
358+
protected Object getExtraCapability(String capabilityName) {
359+
Require.nonNull("Capability name", capabilityName);
360+
361+
switch (capabilityName) {
362+
case FIREFOX_OPTIONS:
363+
return Collections.unmodifiableMap(firefoxOptions);
364+
365+
case MARIONETTE:
366+
return !legacy;
367+
368+
default:
369+
return null;
350370
}
351-
return unmodifiableMap(toReturn);
352371
}
353372

354373
@Override

‎java/client/src/org/openqa/selenium/ie/InternetExplorerOptions.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,12 @@
3939

4040
import java.time.Duration;
4141
import java.util.Arrays;
42+
import java.util.Collections;
4243
import java.util.HashMap;
4344
import java.util.LinkedList;
4445
import java.util.List;
4546
import java.util.Map;
47+
import java.util.Set;
4648
import java.util.concurrent.TimeUnit;
4749
import java.util.stream.Collectors;
4850
import java.util.stream.Stream;
@@ -253,4 +255,15 @@ public void setCapability(String key, Object value) {
253255
});
254256
}
255257
}
258+
259+
@Override
260+
protected Set<String> getExtraCapabilityNames() {
261+
return Collections.emptySet();
262+
}
263+
264+
@Override
265+
protected Object getExtraCapability(String capabilityName) {
266+
Require.nonNull("Capability name", capabilityName);
267+
return null;
268+
}
256269
}

‎java/client/src/org/openqa/selenium/opera/OperaOptions.java

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,27 +17,28 @@
1717

1818
package org.openqa.selenium.opera;
1919

20-
import static java.util.Collections.unmodifiableList;
21-
import static java.util.Collections.unmodifiableMap;
22-
import static org.openqa.selenium.remote.BrowserType.OPERA_BLINK;
23-
import static org.openqa.selenium.remote.CapabilityType.BROWSER_NAME;
24-
25-
import org.openqa.selenium.internal.Require;
26-
import org.openqa.selenium.remote.AbstractDriverOptions;
2720
import org.openqa.selenium.Capabilities;
2821
import org.openqa.selenium.WebDriverException;
22+
import org.openqa.selenium.internal.Require;
23+
import org.openqa.selenium.remote.AbstractDriverOptions;
2924

3025
import java.io.File;
3126
import java.io.IOException;
3227
import java.nio.file.Files;
3328
import java.util.ArrayList;
3429
import java.util.Arrays;
3530
import java.util.Base64;
31+
import java.util.Collections;
3632
import java.util.HashMap;
3733
import java.util.List;
3834
import java.util.Map;
35+
import java.util.Set;
3936
import java.util.TreeMap;
4037

38+
import static java.util.Collections.unmodifiableList;
39+
import static org.openqa.selenium.remote.BrowserType.OPERA_BLINK;
40+
import static org.openqa.selenium.remote.CapabilityType.BROWSER_NAME;
41+
4142
/**
4243
* Class to manage options specific to {@link OperaDriver}.
4344
*
@@ -203,8 +204,17 @@ public Object getExperimentalOption(String name) {
203204
}
204205

205206
@Override
206-
public Map<String, Object> asMap() {
207-
Map<String, Object> toReturn = new TreeMap<>(super.asMap());
207+
protected Set<String> getExtraCapabilityNames() {
208+
return Collections.singleton(CAPABILITY);
209+
}
210+
211+
@Override
212+
protected Object getExtraCapability(String capabilityName) {
213+
Require.nonNull("Capability name", capabilityName);
214+
215+
if (!CAPABILITY.equals(capabilityName)) {
216+
return null;
217+
}
208218

209219
Map<String, Object> options = new TreeMap<>(experimentalOptions);
210220

@@ -227,8 +237,6 @@ public Map<String, Object> asMap() {
227237
encodedExtensions.addAll(extensions);
228238
options.put("extensions", unmodifiableList(encodedExtensions));
229239

230-
toReturn.put(CAPABILITY, options);
231-
232-
return unmodifiableMap(toReturn);
240+
return Collections.unmodifiableMap(options);
233241
}
234242
}

‎java/client/src/org/openqa/selenium/remote/AbstractDriverOptions.java

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,13 @@
3030
import org.openqa.selenium.UnexpectedAlertBehaviour;
3131
import org.openqa.selenium.internal.Require;
3232

33-
public class AbstractDriverOptions<DO extends AbstractDriverOptions> extends MutableCapabilities {
33+
import java.util.Collections;
34+
import java.util.Map;
35+
import java.util.Set;
36+
import java.util.TreeMap;
37+
import java.util.TreeSet;
38+
39+
public abstract class AbstractDriverOptions<DO extends AbstractDriverOptions> extends MutableCapabilities {
3440

3541
public DO setPageLoadStrategy(PageLoadStrategy strategy) {
3642
setCapability(
@@ -62,4 +68,31 @@ public DO setProxy(Proxy proxy) {
6268
return (DO) this;
6369
}
6470

71+
@Override
72+
public Set<String> getCapabilityNames() {
73+
TreeSet<String> names = new TreeSet<>(super.getCapabilityNames());
74+
names.addAll(getExtraCapabilityNames());
75+
return Collections.unmodifiableSet(names);
76+
}
77+
78+
protected abstract Set<String> getExtraCapabilityNames();
79+
80+
@Override
81+
public Object getCapability(String capabilityName) {
82+
Require.nonNull("Capability name", capabilityName);
83+
84+
if (getExtraCapabilityNames().contains(capabilityName)) {
85+
return getExtraCapability(capabilityName);
86+
}
87+
return super.getCapability(capabilityName);
88+
}
89+
90+
protected abstract Object getExtraCapability(String capabilityName);
91+
92+
@Override
93+
public Map<String, Object> asMap() {
94+
Map<String, Object> toReturn = new TreeMap<>(super.asMap());
95+
getExtraCapabilityNames().forEach(name -> toReturn.put(name, getCapability(name)));
96+
return Collections.unmodifiableMap(toReturn);
97+
}
6598
}

‎java/client/src/org/openqa/selenium/safari/SafariOptions.java

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,17 @@
1717

1818
package org.openqa.selenium.safari;
1919

20-
import static java.util.Collections.unmodifiableMap;
21-
import static org.openqa.selenium.remote.CapabilityType.BROWSER_NAME;
22-
20+
import org.openqa.selenium.Capabilities;
2321
import org.openqa.selenium.MutableCapabilities;
22+
import org.openqa.selenium.WebDriverException;
2423
import org.openqa.selenium.internal.Require;
2524
import org.openqa.selenium.remote.AbstractDriverOptions;
26-
import org.openqa.selenium.Capabilities;
27-
import org.openqa.selenium.WebDriverException;
2825

26+
import java.util.Collections;
2927
import java.util.Map;
30-
import java.util.TreeMap;
28+
import java.util.Set;
29+
30+
import static org.openqa.selenium.remote.CapabilityType.BROWSER_NAME;
3131

3232
/**
3333
* Class to manage options specific to {@link SafariDriver}.
@@ -164,8 +164,12 @@ public boolean getUseTechnologyPreview() {
164164
}
165165

166166
@Override
167-
public Map<String, Object> asMap() {
168-
Map<String, Object> result = new TreeMap<>(super.asMap());
169-
return unmodifiableMap(result);
167+
protected Set<String> getExtraCapabilityNames() {
168+
return Collections.emptySet();
169+
}
170+
171+
@Override
172+
protected Object getExtraCapability(String capabilityName) {
173+
return null;
170174
}
171175
}

0 commit comments

Comments
 (0)