-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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] Support Grid custom capability mutators #13672
base: trunk
Are you sure you want to change the base?
Conversation
Custom capability mutators(with ordering priority) can be placed as part of Grid setup and will be used. Fixes SeleniumHQ#13628
PR Description updated to latest commit (124a3cc)
|
PR Review
✨ Review tool usage guide:Overview:
With a configuration file, use the following template:
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
With a configuration file, use the following template:
See the improve usage page for a more comprehensive guide on using this tool. |
Custom capability mutators(with ordering priority) can be placed as part of Grid setup and will be used. Fixes SeleniumHQ#13628
|
||
public class CapabilitiesMutatorService { | ||
Capabilities stereotype; | ||
private final List<CapabilityMutator> customMutators = new CopyOnWriteArrayList<>(); |
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 think the customMutators
is expected to be populated as soon as the object gets created. After that Its only going to be iterated upon. So do we still need a CopyOnWriteArrayList
? Can we not just use ArrayList
instead ?
private void loadAllCustomMutators() { | ||
ServiceLoader<CapabilityMutator> loader = ServiceLoader.load(CapabilityMutator.class); | ||
|
||
List<CapabilityMutator> allMutators = StreamSupport.stream(loader.spliterator(), false) |
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.
It would be good if you could create a common method that internally takes care of building a stream, sorting the collection and returning back the list. This common method should be called via the loadAllCustomMutators
and setCustomMutators
. That way, the sorting part is also tested out.
import org.openqa.selenium.Platform; | ||
import org.openqa.selenium.SessionNotCreatedException; | ||
import org.openqa.selenium.WebDriverException; | ||
import org.openqa.selenium.*; |
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.
Can we please revert back this wildcard import?
} | ||
|
||
public Capabilities getMutatedCapabilities(Capabilities desiredCapabilities) { | ||
Objects.requireNonNull(desiredCapabilities, "desiredCapabilities must not be null"); |
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.
Selenium is using Require.nonNull
. So for consistency sake, maybe we can use that instead of Objects.requireNonNull
Objects.requireNonNull(desiredCapabilities, "desiredCapabilities must not be null"); | ||
|
||
// Always apply this default capability mutator before applying any other mutator | ||
SessionCapabilitiesMutator defaultMutator = new SessionCapabilitiesMutator(stereotype); |
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.
The defaultMutator seems to be based off of stereoType
which is the same throughout the lifecycle of this object. So does it need to be created multiple times?
import org.openqa.selenium.remote.http.ClientConfig; | ||
import org.openqa.selenium.remote.http.HttpClient; | ||
import org.openqa.selenium.remote.service.DriverFinder; | ||
import org.openqa.selenium.remote.service.DriverService; | ||
import org.openqa.selenium.remote.tracing.AttributeKey; |
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.
Please help revert the wild card imports.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #13672 +/- ##
=======================================
Coverage 58.48% 58.48%
=======================================
Files 86 86
Lines 5270 5270
Branches 220 220
=======================================
Hits 3082 3082
Misses 1968 1968
Partials 220 220 ☔ View full report in Codecov by Sentry. |
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.
Thank you for starting this work, @Purus.
I need help to grasp the general idea of the implementation. Why do we need a service for this? I do not understand the need to have several capability mutators. This implementation seems too complex for the requirement. I believe the idea is to overwrite the default capabilities mutator.
Why not follow the concept we have with other pieces? For example, SlotMatcher
and DefaultSlotMatcher
?
User description
Custom capability mutators(with ordering priority) can be placed as part of Grid setup and will be used.
Fixes #13628
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Custom capability mutators(with ordering priority) can be placed as part of Grid setup and will be used.
Motivation and Context
Provides option to update the capabilities based on various aspects. Use cases available in #13628.
Types of changes
Checklist
Type
Enhancement
Description
CapabilitiesMutatorService
to support custom capability mutations with ordering priority.CapabilityMutator
interface for implementing custom mutators.DriverServiceSessionFactory
to useCapabilitiesMutatorService
for mutating session capabilities.SessionCapabilitiesMutator
to implement the newCapabilityMutator
interface.CapabilitiesMutatorService
.Changes walkthrough
CapabilitiesMutatorService.java
Introduce Capabilities Mutator Service for Custom Capability Mutations
java/src/org/openqa/selenium/grid/node/config/CapabilitiesMutatorService.java
mutators.
mutators.
CapabilityMutator.java
Define Capability Mutator Interface
java/src/org/openqa/selenium/grid/node/config/CapabilityMutator.java
DriverServiceSessionFactory.java
Use CapabilitiesMutatorService in DriverServiceSessionFactory
java/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java
for mutating session capabilities.
SessionCapabilitiesMutator.java
Update SessionCapabilitiesMutator to Implement CapabilityMutator
java/src/org/openqa/selenium/grid/node/config/SessionCapabilitiesMutator.java
interface.
CapabilitiesMutatorServiceTest.java
Add Tests for CapabilitiesMutatorService
java/test/org/openqa/selenium/grid/node/config/CapabilitiesMutatorServiceTest.java
mutator order.
BUILD.bazel
Add Core Dependency to Distributor Config BUILD
java/src/org/openqa/selenium/grid/distributor/config/BUILD.bazel
//java/src/org/openqa/selenium:core
for thedistributor config.