Skip to content

Commit

Permalink
Capabilities that does not match current platform should be ignored o…
Browse files Browse the repository at this point in the history 
…n node only. Fixes #5163
  • Loading branch information
@barancev
barancev committed Dec 16, 2017
1 parent ae0f764 commit 182ed38
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 6 deletions.
1 change: 1 addition & 0 deletions java/server/src/org/openqa/grid/common/RegistrationRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ public static RegistrationRequest build(GridNodeConfiguration configuration, Str
pendingRequest.configuration.fixUpHost();
// make sure the capabilities are updated with required fields
pendingRequest.configuration.fixUpCapabilities();
pendingRequest.configuration.dropCapabilitiesThatDoenNotMatchCurrentPlatform();

This comment has been minimized.

Sign in to view

Copy link
@kool79

kool79 Dec 21, 2017

Contributor

Is it good idea to drop those capabilities? Can you guarantee that current platform always will be detected correctly (for example, for new OS versions) and users will not be forced to switch to updated selenium library?

return pendingRequest;
}
Expand Down
17 changes: 14 additions & 3 deletions java/server/src/org/openqa/grid/internal/utils/configuration/GridNodeConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -530,16 +530,27 @@ public void fixUpCapabilities() {

Platform current = Platform.getCurrent();
capabilities = capabilities.stream()
.peek(cap -> cap.setCapability(CapabilityType.PLATFORM,
Optional.ofNullable(cap.getPlatform()).orElse(current)))
.filter(cap -> current.is(cap.getPlatform()))
.peek(cap -> cap.setCapability(
CapabilityType.PLATFORM,
Optional.ofNullable(cap.getCapability(CapabilityType.PLATFORM_NAME)).orElse(current)))

This comment has been minimized.

Sign in to view

Copy link
@kool79

kool79 Dec 21, 2017 •
edited
Loading

Contributor

Here we set (deprecated) capability PLATFORM either to PLATFORM_NAME or current platforms. I.e. when we don't set platform in json-> capabilities than it will be auto-detected.
But here we don't have any auto-detection for new capability PLATFORM_NAME. Is it OK?
P.S.: checked on macOS, High Sierra: when we omit platform/platformName then RegistrationRequest contains only deprecated capability plaftform="MAC"
.peek(cap -> cap.setCapability(RegistrationRequest.SELENIUM_PROTOCOL,
Optional.ofNullable(cap.getCapability(RegistrationRequest.SELENIUM_PROTOCOL))
.orElse(SeleniumProtocol.WebDriver.toString())))
.peek(cap -> cap.setCapability(CONFIG_UUID_CAPABILITY, UUID.randomUUID().toString()))
.collect(Collectors.toList());
}

public void dropCapabilitiesThatDoenNotMatchCurrentPlatform() {

This comment has been minimized.

Sign in to view

Copy link
@kool79

kool79 Dec 21, 2017

Contributor

please, fix method name: ...DoenNot... -> DoesNot
if (capabilities == null) {
return; // assumes the caller set it/wants it this way
}

Platform current = Platform.getCurrent();
capabilities = capabilities.stream()
.filter(cap -> current.is(cap.getPlatform()))

This comment has been minimized.

Sign in to view

Copy link
@kool79

kool79 Dec 21, 2017 •
edited
Loading

Contributor

this will fail when cap.getPlatform() is "more specific" than current.
Currently node on my mac drops capability with platform/platformName = "sierra" (observed in registration request from node). Additional info: Platform.getCurrent() on my node returns "MAC".
I suggest something like:

Platform current = Platform.getCurrent();
// extract family if we detected 'more specific' version
Platform currentFamily = current.family() != null ? current.family() : current;
capabilities = capabilities.stream()
    .filter(cap -> cap.getPlatform() == null || cap.getPlatform().is(currentFamily))

note: check for == null should always return 'false', but want to be on safe side

This comment has been minimized.

Sign in to view

Copy link
@barancev

barancev Dec 22, 2017

Author Member

Thank you for the comments! They are addressed in f2eec92
.collect(Collectors.toList());
}

public void fixUpHost() {
NetworkUtils util = new NetworkUtils();
if (host == null || "ip".equalsIgnoreCase(host)) {
Expand Down
26 changes: 23 additions & 3 deletions java/server/test/org/openqa/grid/common/RegistrationRequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.openqa.selenium.remote.DesiredCapabilities;

import java.net.URL;
import java.util.Arrays;

public class RegistrationRequestTest {

Expand Down Expand Up @@ -82,7 +83,6 @@ public void testToJson() {
req2.getConfiguration().capabilities.get(0).getPlatform());
}


@Test
public void basicCommandLineParam() {
GridNodeConfiguration config = new GridNodeConfiguration();
Expand All @@ -92,7 +92,6 @@ public void basicCommandLineParam() {
assertEquals(GridRole.NODE, GridRole.get(req.getConfiguration().role));
assertEquals("ABC", req.getConfiguration().getHubHost());
assertEquals(1234, req.getConfiguration().getHubPort().longValue());

}

@Test
Expand Down Expand Up @@ -128,7 +127,6 @@ public void registerParam() {
new JCommander(config, "-role", "wd", "-hubHost", "ABC", "-hubPort", "1234","-host","localhost", "-register","false");
RegistrationRequest req2 = RegistrationRequest.build(config);
assertEquals(false, req2.getConfiguration().register);

}

@Test
Expand Down Expand Up @@ -258,6 +256,28 @@ public void testBuildWithConfigurationAndNullCapabilities() {
assertNull(req.getConfiguration().capabilities);
}

@Test
public void testConstructorDoesNotPruneCapabilitiesWithUnknownPlatform() {
GridNodeConfiguration config = new GridNodeConfiguration();
MutableCapabilities capabilities = new MutableCapabilities();
capabilities.setCapability("browserName", "firefox");
capabilities.setCapability("platformName", "cheese");
config.capabilities = Arrays.asList(capabilities);
RegistrationRequest req = new RegistrationRequest(config);
assertEquals(req.getConfiguration().capabilities.size(), 1);
}

@Test
public void testBuilderPrunesCapabilitiesWithUnknownPlatform() {
GridNodeConfiguration config = new GridNodeConfiguration();
MutableCapabilities capabilities = new MutableCapabilities();
capabilities.setCapability("browserName", "firefox");
capabilities.setCapability("platformName", "cheese");
config.capabilities = Arrays.asList(capabilities);
RegistrationRequest req = RegistrationRequest.build(config);
assertEquals(req.getConfiguration().capabilities.size(), 0);
}

private void assertConstruction(RegistrationRequest req) {
assertNotNull(req);
assertNotNull(req.getConfiguration());
Expand Down

0 comments on commit 182ed38

Please sign in to comment.