-
-
Notifications
You must be signed in to change notification settings - Fork 753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Providing W3C compliance #829
Conversation
There's a lot of overlap between the ProtocolHandshake and the NewSessionPayload. Time to deal with this overlap between the two. The main change is to make the NewSessionPayload capable of writing to an Appendable. It does so by streaming the Capabilities used to create the payload as the OSS capabilities and those used by old geckodrivers, and then streaming every capability through transforms to generate spec compliant capabilities. There appears to be a bug where we always generate synthetic capabilities, but we can deal with that later.
Using the magic of Buck's "maven-importer" and the following maven coordinates: ``` 'org.seleniumhq.selenium:htmlunit-driver:jar:2.28.5' \ 'junit:junit:jar:4.12' \ 'net.bytebuddy:byte-buddy:jar:1.7.9' \ 'com.google.code.gson:gson:jar:2.8.2' \ 'com.google.guava:guava:jar:23.6-jre' \ 'org.apache.commons:commons-exec:jar:1.3' \ 'org.eclipse.jetty:jetty-security:jar:9.4.8.v20171121' \ 'org.testng:testng:jar:6.13.1' \ 'org.pantsbuild:jarjar:jar:1.6.5' \ 'org.eclipse.jetty:jetty-util:jar:9.4.8.v20171121' \ 'org.eclipse.jetty:jetty-server:jar:9.4.8.v20171121' \ 'org.eclipse.jetty:jetty-servlet:jar:9.4.8.v20171121' \ 'org.hamcrest:hamcrest-library:jar:1.3' \ 'com.github.javaparser:javaparser-core:jar:3.5.7' \ 'org.eclipse.jetty:jetty-jmx:jar:9.4.8.v20171121' \ 'net.jcip:jcip-annotations:jar:1.0' \ 'org.yaml:snakeyaml:jar:1.19' \ 'org.mockito:mockito-core:jar:2.13.0' \ 'io.netty:netty-all:jar:4.1.19.Final' \ 'org.eclipse.mylyn.github:org.eclipse.egit.github.core:jar:2.1.5' \ 'org.littleshoot:littleproxy:jar:1.1.2' \ 'org.slf4j:slf4j-jdk14:jar:1.7.25' ``` The version of LittleProxy we use is a snapshot and is the old version from previously with dependencies updated.
|
|
||
/** | ||
* This is the flag which forces server to switch to the mobile WSONWP. | ||
* If {@code false} when it is switched to W3C mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when->then
private final FileBackedOutputStream backingStore; | ||
private final ImmutableSet<Dialect> dialects; | ||
|
||
private static List<String> getAppiumCapabilities(Class<?> capabilityList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List<String>
or List<Object>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mykola-mokhnach
List<String>
private static List<String> getAppiumCapabilities(Class<?> capabilityList) {
return Arrays.stream(capabilityList.getDeclaredFields()).map(field -> {
field.setAccessible(true);
try {
return field.get(capabilityList).toString(); //<-!!!!
} catch (IllegalAccessException e) {
throw new IllegalArgumentException(e);
}
}).filter(s -> !FORCE_MJSONWP.equals(s)).collect(toList());
}
// We need to convert the capabilities into a new session payload. At this point we're dealing | ||
// with references, so I'm Just Sure This Will Be Fine. | ||
boolean forceMobileJSONWP = ofNullable(caps.getCapability(FORCE_MJSONWP)) | ||
.map(o -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use ternary operator here
} | ||
|
||
public static NewSessionPayload create(Capabilities caps) throws IOException { | ||
// We need to convert the capabilities into a new session payload. At this point we're dealing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
return new NewSessionPayload(source, forceMobileJSONWP); | ||
} | ||
|
||
private NewSessionPayload(Reader source, boolean forceMobileJSONWP) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This copies a lot of stuff from the original selenium code. Is it somehow possible to avoid such duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for example using reflection
// Opera: operaOptions | ||
// SafariDriver: safari.options | ||
// | ||
// We can't use the constants defined in the classes because it would introduce circular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all the commented stuff above?
public String getKey() { | ||
String key = stringObjectEntry.getKey(); | ||
if (APPIUM_CAPABILITIES.contains(key) && !forceMobileJSONWP) { | ||
return "appium:" + key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appium:
might be a constant
.map(this::applyTransforms) | ||
.map(map -> map.entrySet().stream() | ||
.filter(entry -> { | ||
if (forceMobileJSONWP) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ternary operator might be more useful here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.filter(entry -> forceMobileJSONWP && ACCEPTED_W3C_PATTERNS.test(entry.getKey()) || !forceMobileJSONWP)
return Stream.concat(fromOss, fromW3c).distinct(); | ||
} | ||
|
||
private Map<String, Object> convertOssToW3C(Map<String, Object> capabilities) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mark as Nullable
name = input.nextName(); | ||
if ("alwaysMatch".equals(name)) { | ||
return input.read(MAP_TYPE); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this else seems redundant
name = input.nextName(); | ||
if ("firstMatch".equals(name)) { | ||
return input.read(LIST_OF_MAPS_TYPE); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
return null; | ||
} | ||
|
||
private Collection<Map<String, Object>> getFirstMatches() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullable
return toReturn; | ||
} | ||
|
||
private Map<String, Object> getAlwaysMatch() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullable
backingStore.reset(); | ||
} | ||
|
||
private Map<String, Object> getOss() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullable
String name = input.nextName(); | ||
if ("desiredCapabilities".equals(name)) { | ||
return input.read(MAP_TYPE); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else is redundant
"Illegal key values seen in w3c capabilities: " + illegalKeys); | ||
} | ||
}) | ||
.forEach(map -> {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of this operator?
} | ||
|
||
// Write the first capability we get as the desired capability. | ||
json.name("desiredCapabilities"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather put all there magic strings into constants
- removal of redundant code from the original NewSessionPayload
@SrinivasanTarget Could you try it on iOS? |
/** | ||
* Which mobile OS platform to use. | ||
*/ | ||
String PLATFORM_NAME = "platformName"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where this constant is present now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in org.openqa.selenium.remote.CapabilityType
public interface MobileCapabilityType extends CapabilityType
|
||
public static NewSessionPayload create(Capabilities caps) throws IOException { | ||
boolean forceMobileJSONWP = ofNullable(caps.getCapability(FORCE_MJSONWP)) | ||
.map(o -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map(o -> Boolean.class.isAssignableFrom(o.getClass()) && Boolean.class.cast(o))
String name = input.nextName(); | ||
if (DESIRED_CAPABILITIES.equals(name)) { | ||
return input.read(MAP_TYPE); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else is redundant
Updated. |
return null; | ||
} | ||
|
||
private @Nullable Collection<Map<String, Object>> getFirstMatches() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFirstMatch
name = input.nextName(); | ||
if (FIRST_MATCH.equals(name)) { | ||
return input.read(LIST_OF_MAPS_TYPE); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else is redundant. same comment to other similar blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks fine on iOS on both W3C and JSONWP
Change list
org.openqa.selenium.remote.NewSessionPayload
Types of changes
Details
I am not sure that this solution is 100% good but it seem the goal is achived.
https://gist.github.com/TikhomirovSergey/0bcaf79e96cd1ef6644e5e8c2b975596
https://gist.github.com/TikhomirovSergey/5987f5c74e37110427b8f5651030aa3b