Skip to content
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.

Sign up for GitHub

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

[grid] Eliminate vendor-specific handling of extension capabilities #14485

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+127 −67
Open

[grid] Eliminate vendor-specific handling of extension capabilities #14485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 52 additions & 46 deletions java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,27 @@
* Then the {@code stereotype} must contain the same values.
* </ul>
*
* <p>One thing to note is that extension capabilities are not considered when matching slots, since
* the matching of these is implementation-specific to each driver.
* <p>Note that extension capabilities are considered for slot matching, with the following exceptions:
*
* <ul>
* <li>Extension capabilities with prefix "se:"
* <li>Extension capabilities with these suffixes:
* <ul>
* <li>"options"
* <li>"Options"
* <li>"loggingPrefs"
* <li>"debuggerAddress"
* </ul>
* </ul>
*/
public class DefaultSlotMatcher implements SlotMatcher, Serializable {

/*
List of prefixed extension capabilities we never should try to match, they should be
List of extension capability suffixes we never should try to match, they should be
matched in the Node or in the browser driver.
*/
private static final List<String> EXTENSION_CAPABILITIES_PREFIXES =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep this because the browser vendors can make changes and we don't want to change things here every time they do it. For example, moz:debuggerAddress is now being sent from the client side to enable CDP in Firefox.

Copy link
Contributor Author

@sbabcoc sbabcoc Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core objective of this PR is to migrate all vendor-specific capabilities that should be ignored for purposes of slot matching into "options" objects. (I had initially implemented this to ignore all extension capabilities with "complex" values.) The current strategy of ignoring all extension capabilities with a small number of "special" prefixes while considering all extension capabilities with "non-special" prefixes produces inconsistent behavior and requires vendors to implement special-case slot matchers, each of which is mutually exclusive to every other custom slot matcher.

In your example of moz:debuggerAddress, the desire is to ignore this capability for slot matching. What happens if Mozilla adds a new extension capability that should be considered for slot matching? The current strategy doesn't allow for this possibility, forcing Mozilla to implement a custom slot matcher, and for all affected clients to update their Grid configurations to add this matcher. For clients with Grid configurations that supply sessions of browsers from multiple vendors, this could force complete reconfiguration of their automation infrastructure, segregating their session providers into multiple grids by vendor.

With the revised behavior implemented by this PR, moz:debuggerAddress would be considered for slot matching. To ignore this option for slot matching, it would be migrated into moz:firefoxOptions, I believe this is a more sensible approach, because this capability truly is an option, not an identity specifier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current strategy of ignoring all extension capabilities with a small number of "special" prefixes while considering all extension capabilities with "non-special" prefixes produces inconsistent behavior and requires vendors to implement special-case slot matchers, each of which is mutually exclusive to every other custom slot matcher

This is an assumption.

If Mozilla adds a new capability for matching, we ignore it and pass it along to GeckoDriver.

Copy link
Contributor Author

@sbabcoc sbabcoc Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is precisely my point. Outside of mutually exclusive custom slot matchers, vendors have no control over how their extension capabilities are handled. Capabilities with "special" prefixes will always be ignored and capabilities with "non-special" prefixes will always be considered. This PR eliminates the inconsistency and adds the ability for vendors to control whether their extension capabilities will be treated as identifying characteristics or as end-node configuration options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave the prefixes. Then we can merge the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it an inconsistency? Browser vendors just need those capabilities to be passed along. They do not use the Grid and configure stereotypes to do matching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every extension capability for Google, Mozilla, and Microsoft is ignored for slot matching; every extension capability for everyone else (including Apple and Appium) is considered for slot matching. This behavior is inconsistent. It also fails to provide vendors with the ability to control which capabilities are treated as identity values and which are treated as configuration options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apple does not need to be there; this might change in the future.

However, Appium does need to be considered, except the options one.

Browser vendors just need to have their capabilities passed along. Why do you keep saying that they will use their capabilities to do matching in Grid when this is not true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appium extension capabilities are most certainly being considered for slot matching. This is still true in the revised functionality implemented by this PR. I think the behavior of the current matcher is what necessitated the merge operation and explicit check for 'appium:app'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my current implementation, all existing Appium extension capabilities are considered except for the four suffixes: "options", "Options", "loggingPrefs", and "debuggerAddress". There's implementation in the Appium server that merges the prefixed capabilities into their "options" collection with no thought to which capabilities are "identity" values and which are configuration options. I don't know if this gets reflected back to Grid in a way that's visible to DefaultSlotMatcher, but their configuration examples indicate that every option can be declared in either place. I'm attempting to engage the Appium folks to see how they feel about deprecating this pattern in favor of clear differentiation between "identity" values and configuration options.

Arrays.asList("goog:", "moz:", "ms:", "se:");
private static final List<String> EXTENSION_CAPABILITY_SUFFIXES =
Arrays.asList("Options", "options", "loggingPrefs", "debuggerAddress");

@Override
public boolean matches(Capabilities stereotype, Capabilities capabilities) {
Expand All @@ -76,14 +86,14 @@ public boolean matches(Capabilities stereotype, Capabilities capabilities) {

// At the end, a simple browser, browserVersion and platformName match
boolean browserNameMatch =
(capabilities.getBrowserName() == null || capabilities.getBrowserName().isEmpty())
capabilities.getBrowserName() == null
|| capabilities.getBrowserName().isEmpty()
|| Objects.equals(stereotype.getBrowserName(), capabilities.getBrowserName());
boolean browserVersionMatch =
(capabilities.getBrowserVersion() == null
|| capabilities.getBrowserVersion().isEmpty()
|| Objects.equals(capabilities.getBrowserVersion(), "stable"))
|| browserVersionMatch(
stereotype.getBrowserVersion(), capabilities.getBrowserVersion());
capabilities.getBrowserVersion() == null
|| capabilities.getBrowserVersion().isEmpty()
|| Objects.equals(capabilities.getBrowserVersion(), "stable")
|| browserVersionMatch(stereotype.getBrowserVersion(), capabilities.getBrowserVersion());
boolean platformNameMatch =
capabilities.getPlatformName() == null
|| Objects.equals(stereotype.getPlatformName(), capabilities.getPlatformName())
Expand All @@ -102,21 +112,17 @@ private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities)
.filter(name -> !name.contains(":"))
// Platform matching is special, we do it later
.filter(name -> !"platformName".equalsIgnoreCase(name))
.map(
.filter(name -> capabilities.getCapability(name) != null)
.allMatch(
name -> {
if (capabilities.getCapability(name) instanceof String) {
sbabcoc marked this conversation as resolved.
Show resolved Hide resolved
return stereotype
.getCapability(name)
.toString()
.equalsIgnoreCase(capabilities.getCapability(name).toString());
} else {
return capabilities.getCapability(name) == null
|| Objects.equals(
stereotype.getCapability(name), capabilities.getCapability(name));
if (stereotype.getCapability(name) instanceof String
&& capabilities.getCapability(name) instanceof String) {
return ((String) stereotype.getCapability(name))
.equalsIgnoreCase((String) capabilities.getCapability(name));
}
})
.reduce(Boolean::logicalAnd)
.orElse(true);
return Objects.equals(
stereotype.getCapability(name), capabilities.getCapability(name));
});
}

private Boolean managedDownloadsEnabled(Capabilities stereotype, Capabilities capabilities) {
Expand All @@ -140,39 +146,39 @@ private Boolean platformVersionMatch(Capabilities stereotype, Capabilities capab
*/
return capabilities.getCapabilityNames().stream()
.filter(name -> name.contains("platformVersion"))
.map(
.allMatch(
platformVersionCapName ->
Objects.equals(
stereotype.getCapability(platformVersionCapName),
capabilities.getCapability(platformVersionCapName)))
.reduce(Boolean::logicalAnd)
.orElse(true);
capabilities.getCapability(platformVersionCapName)));
}

private Boolean extensionCapabilitiesMatch(Capabilities stereotype, Capabilities capabilities) {
/*
We match extension capabilities when they are not prefixed with any of the
EXTENSION_CAPABILITIES_PREFIXES items. Also, we match them only when the capabilities
of the new session request contains that specific extension capability.
We match extension capabilities in new session requests whose names do not have the prefix "se:" or
one of the reserved suffixes ("options", "Options", "loggingPrefs", or "debuggerAddress"). These are
forwarded to the matched node for use in configuration, but are not considered for node matching.
*/
return stereotype.getCapabilityNames().stream()
// examine only extension capabilities
.filter(name -> name.contains(":"))
.filter(name -> capabilities.asMap().containsKey(name))
.filter(name -> EXTENSION_CAPABILITIES_PREFIXES.stream().noneMatch(name::contains))
.map(
// ignore Selenium extension capabilities
.filter(name -> !name.startsWith("se:"))
// ignore special extension capability suffixes
.filter(name -> EXTENSION_CAPABILITY_SUFFIXES.stream().noneMatch(name::endsWith))
// ignore capabilities not specified in the request
.filter(name -> capabilities.getCapability(name) != null)
.allMatch(
name -> {
if (capabilities.getCapability(name) instanceof String) {
return stereotype
.getCapability(name)
.toString()
.equalsIgnoreCase(capabilities.getCapability(name).toString());
} else {
return capabilities.getCapability(name) == null
|| Objects.equals(
stereotype.getCapability(name), capabilities.getCapability(name));
// evaluate capabilities with String values
if (stereotype.getCapability(name) instanceof String
&& capabilities.getCapability(name) instanceof String) {
return ((String) stereotype.getCapability(name))
.equalsIgnoreCase((String) capabilities.getCapability(name));
}
})
.reduce(Boolean::logicalAnd)
.orElse(true);
// evaluate capabilities with Number or Boolean values
return Objects.equals(
stereotype.getCapability(name), capabilities.getCapability(name));
});
}
}
11 changes: 0 additions & 11 deletions java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import java.util.logging.Logger;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
import org.openqa.selenium.MutableCapabilities;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.WebDriverException;
import org.openqa.selenium.grid.data.CreateSessionRequest;
Expand All @@ -50,7 +49,6 @@
import org.openqa.selenium.internal.Debug;
import org.openqa.selenium.internal.Either;
import org.openqa.selenium.internal.Require;
import org.openqa.selenium.remote.CapabilityType;
import org.openqa.selenium.remote.Command;
import org.openqa.selenium.remote.Dialect;
import org.openqa.selenium.remote.DriverCommand;
Expand Down Expand Up @@ -149,15 +147,6 @@ public Either<WebDriverException, ActiveSession> apply(CreateSessionRequest sess
"New session request capabilities do not " + "match the stereotype."));
}

// remove browserName capability if 'appium:app' is present as it breaks appium tests when app
diemol marked this conversation as resolved.
Show resolved Hide resolved
// is provided
// they are mutually exclusive
MutableCapabilities filteredStereotype = new MutableCapabilities(stereotype);
if (capabilities.getCapability("appium:app") != null) {
filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String) null);
}

capabilities = capabilities.merge(filteredStereotype);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I remember. This was done because the user can specify something in the stereotype that will be sent to the Node that will receive the request.

LOG.info("Starting session for " + capabilities);

try (Span span = tracer.getCurrentContext().createSpan("relay_session_factory.apply")) {
Expand Down
79 changes: 70 additions & 9 deletions java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Map;
import org.junit.jupiter.api.Test;
import org.openqa.selenium.Capabilities;
import org.openqa.selenium.ImmutableCapabilities;
Expand Down Expand Up @@ -540,7 +541,37 @@ void multipleExtensionPrefixedCapabilitiesDoNotMatchWhenOneIsDifferent() {
}

@Test
void vendorExtensionPrefixedCapabilitiesAreIgnoredForMatching() {
void seleniumExtensionCapabilitiesAreIgnoredForMatching() {
Capabilities stereotype =
new ImmutableCapabilities(
CapabilityType.BROWSER_NAME,
"chrome",
CapabilityType.BROWSER_VERSION,
"84",
CapabilityType.PLATFORM_NAME,
Platform.WINDOWS,
"se:cdpVersion",
1,
"se:downloadsEnabled",
true);

Capabilities capabilities =
new ImmutableCapabilities(
CapabilityType.BROWSER_NAME,
"chrome",
CapabilityType.BROWSER_VERSION,
"84",
CapabilityType.PLATFORM_NAME,
Platform.WINDOWS,
"se:cdpVersion",
2,
"se:downloadsEnabled",
false);
assertThat(slotMatcher.matches(stereotype, capabilities)).isTrue();
}

@Test
void vendorOptionsCapabilitiesAreIgnoredForMatching() {
Capabilities stereotype =
new ImmutableCapabilities(
CapabilityType.BROWSER_NAME,
Expand All @@ -549,10 +580,10 @@ void vendorExtensionPrefixedCapabilitiesAreIgnoredForMatching() {
"84",
CapabilityType.PLATFORM_NAME,
Platform.WINDOWS,
"goog:cheese",
"amsterdam",
"ms:fruit",
"mango");
"food:fruitOptions",
"mango",
"dairy:options",
Map.of("cheese", "amsterdam"));

Capabilities capabilities =
new ImmutableCapabilities(
Expand All @@ -562,10 +593,40 @@ void vendorExtensionPrefixedCapabilitiesAreIgnoredForMatching() {
"84",
CapabilityType.PLATFORM_NAME,
Platform.WINDOWS,
"goog:cheese",
"gouda",
"ms:fruit",
"orange");
"food:fruitOptions",
"orange",
"dairy:options",
Map.of("cheese", "gouda"));
assertThat(slotMatcher.matches(stereotype, capabilities)).isTrue();
}

@Test
void specialExtensionCapabilitiesAreIgnoredForMatching() {
Capabilities stereotype =
new ImmutableCapabilities(
CapabilityType.BROWSER_NAME,
"chrome",
CapabilityType.BROWSER_VERSION,
"84",
CapabilityType.PLATFORM_NAME,
Platform.WINDOWS,
"food:loggingPrefs",
"mango",
"food:debuggerAddress",
Map.of("cheese", "amsterdam"));

Capabilities capabilities =
new ImmutableCapabilities(
CapabilityType.BROWSER_NAME,
"chrome",
CapabilityType.BROWSER_VERSION,
"84",
CapabilityType.PLATFORM_NAME,
Platform.WINDOWS,
"food:loggingPrefs",
"orange",
"food:debuggerAddress",
Map.of("cheese", "gouda"));
assertThat(slotMatcher.matches(stereotype, capabilities)).isTrue();
}

Expand Down
6 changes: 5 additions & 1 deletion java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import org.openqa.selenium.internal.Either;
import org.openqa.selenium.json.Json;
import org.openqa.selenium.net.NetworkUtils;
import org.openqa.selenium.remote.Browser;
import org.openqa.selenium.safari.SafariDriverInfo;

@SuppressWarnings("DuplicatedCode")
Expand Down Expand Up @@ -148,7 +149,10 @@ boolean isDownloadEnabled(WebDriverInfo driver, String customMsg) {
reported.add(caps);
return Collections.singleton(HelperFactory.create(config, caps));
});
String expected = driver.getDisplayName();
String expected =
"Edge".equals(driver.getDisplayName())
? Browser.EDGE.browserName()
: driver.getDisplayName();

Capabilities found =
reported.stream()
Expand Down