As map #1356
As map #1356
Conversation
|
|
||
| Map<String, Object> capsMap = super.asMap(); | ||
| capsMap.forEach((key, value) -> { | ||
| if (key.equals("platformName")) { |
There was a problem hiding this comment.
I think it is not only the platform name. W3C defines a bunch of keys, which must be supplied without prefixes and Selenium has this list somewhere in constants
There was a problem hiding this comment.
Hmm; platformName is the only one we specify in MobileOptions, but you are probably right here. Looks like NewAppiumSessionPayload uses AcceptedW3CCapabilityKeys which is probably the right fix?
| if (key.equals("platformName")) { | ||
| toReturn.put(key, value); | ||
| } else { | ||
| toReturn.put(APPIUM_PREFIX + key, value); |
There was a problem hiding this comment.
should we also check if cap name is already prefixed?
There was a problem hiding this comment.
Yeah, I guess if we are allowing people to do setCapability() we should have some protection for that.
| Map<String, Object> toReturn = new TreeMap<>(); | ||
|
|
||
| Map<String, Object> capsMap = super.asMap(); | ||
| capsMap.forEach((key, value) -> { |
There was a problem hiding this comment.
I think we can build toReturn without having the intermediate capsMap variable, e.g.
return unmodifiableMap(super.asMap().entrySet().stream().map .... collect(Collectors.toMap ...)
I assume toUnmodifiableMap collector has only been added in java10
There was a problem hiding this comment.
I pretty much just copied it from Selenium code, which I think is what happens when you use Selenium 4 with current Appium code? And Selenium browser options I think did more things with the intermediate step, so maybe it can be replaced with your suggestion here, I can try.
There was a problem hiding this comment.
So, I can't figure out how to get the stream to work. This is what I'm working with right now, which removes the redundant capsMap, but still uses toReturn and forEach directly:
super.asMap().forEach((key, value) -> {
AcceptedW3CCapabilityKeys acceptedW3CCapabilityKeys = new AcceptedW3CCapabilityKeys();
if (acceptedW3CCapabilityKeys.test(key) || key.startsWith(APPIUM_PREFIX)) {
toReturn.put(key, value);
} else {
toReturn.put(APPIUM_PREFIX + key, value);
}
});There was a problem hiding this comment.
Ok, got a stream figured out. Let me know what you think.
| AcceptedW3CCapabilityKeys acceptedW3CCapabilityKeys = new AcceptedW3CCapabilityKeys(); | ||
|
|
||
| return unmodifiableMap(super.asMap().entrySet().stream().collect(Collectors.toMap( | ||
| entry -> (acceptedW3CCapabilityKeys.test(entry.getKey()) || entry.getKey().startsWith(APPIUM_PREFIX)) ? |
There was a problem hiding this comment.
I would move this handler into a separate method for better readability. Also, in Appium server code we don't add prefix if a capability name already contains : character, not just the appium: prefix. Should we consider to do the same here?
|
Closed in favour of #1540 |
2 participants
Change list
Types of changes
Details
The new
MobileOptionsclasses can force w3c compliance without concerns of backward compatibility since they have not yet been released. CurrentasMap()implementation is not w3c compliant because it needs to prependappium:. Essentially I'm looking to be able to merge together Capabilities, the same way Selenium does for browsersThe way I'm reading
getW3C()inNewAppiumSessionPayloadit should still do the correct thing for all inputs. The only place that seems to get a capability without using the helper is forFORCE_MJSONWPwhich shouldn't apply to the new options classes, so I don't think it should be a concern.Note: I can't figure out how to set up my machine for running the entire test suite locally and I can't find any documentation on it, so I can only confirm that the new mobile options classes are working correctly. Any assistance on this would be appreciated.