-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[grid] Eliminate vendor-specific handling of extension capabilities #14485
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
[grid] Eliminate vendor-specific handling of extension capabilities #14485
Conversation
PR Reviewer Guide 🔍
|
a82c998
to
14999b7
Compare
PR Code Suggestions ✨
|
14999b7
to
dbe50be
Compare
ac4802b
to
2d9609b
Compare
java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
Outdated
Show resolved
Hide resolved
java/test/org/openqa/selenium/grid/node/config/NodeOptionsTest.java
Outdated
Show resolved
Hide resolved
2d9609b
to
cca2c28
Compare
java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
Outdated
Show resolved
Hide resolved
d5aeb2e
to
84b6795
Compare
6a5663f
to
d993e8c
Compare
d993e8c
to
378cb93
Compare
790f82e
to
1ead41a
Compare
filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String) null); | ||
} | ||
|
||
capabilities = capabilities.merge(filteredStereotype); |
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.
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.
List of prefixed extension capabilities 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 = |
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.
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.
395488d
to
7bdb2b2
Compare
9c4cbbc
to
5c0d6a5
Compare
5c0d6a5
to
37ef803
Compare
83a1ae5
to
2f42ed8
Compare
2f42ed8
to
1369878
Compare
1369878
to
43534e9
Compare
User description
Description
The existing handling of extension capabilities assigns special significance to four recognized prefixes:
goog:
,moz:
,ms:
, andse:
. Capabilities with these prefixes are entirely ignored by the current default slot matcher implementation, but custom prefixes are considered, as well as those for Safari and Appium. This inconsistency means that properties in extension capabilities can cause affectednewSession
requests to fail even though extension capabilities are supposed to be vendor-specific and should probably not be evaluated by the default slot matcher.This PR retains the "special" status of the "se:" prefix and specifically filters out all extension capabilities with names that end with "options", "Options", "loggingPrefs", and "debuggerAddress". This maintains existing behavior while allowing node configurations and
newSession
requests to include extension capabilities that won't affect slot matching.Motivation and Context
Revolves #14461
The strategy employed by the PR is that extension capabilities which should be ignored for matching can be expressed as capabilities with name prefix "se:" or one of the "special" name suffixes. All other extension capabilities will be considered for slot matching. This is a generalization/extrapolation of existing patterns from current use cases.
Recommended slot matcher enhancements
To reduce the need for custom slot matchers, we could extend the WebDriverInfo interface to add a new method that vendors could implement to evaluate their own criteria:
The default implementation would return
null
to indicate that no evaluation has been performed. If the vendor chooses to implement thematches
method, their initial step must be to invoke their ownisSupporting
method, returningnull
if the corresponding driver doesn't support the specified capabilities.The implementation in DefaultSlotMatcher would be updated to iterate over the available WebDriverInfo providers, retaining only those whose
isSupporting
methods returntrue
. Thematches
methods of this filtered list of providers will then be applied to the available nodes to determine which of these satisfies the criteria specified by the client request. The nodes that satisfy these evaluations (if any) would then be evaluated against the remaining common criteria.Another potential enhancement would be to enable vendor-specific
matches
methods to return a weighted integer result instead of a simpleBoolean
. This would enable best-match behavior. Perhaps this is getting too complicated, though.Types of changes
Checklist
PR Type
Bug fix, Tests
Description
DefaultSlotMatcher
, opting to ignore all extension capabilities with names ending in "options" (case-insensitive).RelaySessionFactory
to streamline session creation by removing redundant capability filtering.DefaultSlotMatcherTest
to align with the new handling of extension capabilities.Changes walkthrough 📝
DefaultSlotMatcher.java
Revise extension capabilities handling in DefaultSlotMatcher
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
RelaySessionFactory.java
Simplify session creation in RelaySessionFactory
java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java
DefaultSlotMatcherTest.java
Update DefaultSlotMatcher tests for revised capability handling
java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
handling.