[grid] Improve SlotMatcher for Node relay to Appium server with hybrid browser and native app #15537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d browser and native app Signed-off-by: Viet Nguyen Duc <[email protected]>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the handling of relay sessions for Appium by refining capability matching and filtering logic in both DefaultSlotMatcher and RelaySessionFactory. Key changes include:
- Enhancements to DefaultSlotMatcher to incorporate Appium-specific capabilities.
- Integration of capability filtering in RelaySessionFactory to remove conflicting browserName settings.
- Addition of comprehensive test cases to validate both new matching and filtering behavior, along with an update to the Bazel build configuration for Mockito.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java | Enhanced matching logic for Appium-specific relay capabilities |
| java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java | Added filtering logic to remove conflicting browserName settings |
| java/test/org/openqa/selenium/grid/node/relay/RelaySessionFactoryTest.java | Added unit tests for capability filtering behavior |
| java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java | Added unit tests for new slot matching scenarios |
| java/test/org/openqa/selenium/grid/node/relay/BUILD.bazel | Updated build configuration to include the Mockito dependency |
Files not reviewed (1)
- java/test/org/openqa/selenium/grid/node/relay/BUILD.bazel: Language not supported
Comments suppressed due to low confidence (1)
java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java:178
- [nitpick] There are two overloaded methods named 'browserVersionMatch' (one accepting strings and one accepting Capabilities). Consider renaming one of them for better clarity of intent.
private boolean browserVersionMatch(String stereotype, String capabilities) {
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
CI Feedback 🧐(Feedback updated until commit 22b15db)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
diemol
requested changes
Apr 4, 2025
Comment on lines
197
to
210
| public static boolean specificRelayCapabilitiesAppMatch(Capabilities capabilities) { | ||
| /* | ||
| This match is specific for the Relay capabilities that are related to the Appium server for native application. | ||
| - If browserName is defined then we always assume it’s a hybrid browser session, so no app-related caps should be provided | ||
| - If app is provided then the assumption is that app should be fetched from somewhere first and then installed on the destination device | ||
| - If only appPackage is provided for uiautomator2 driver or bundleId for xcuitest then the assumption is we want to automate an app that is already installed on the device under test | ||
| - If both (app and appPackage) or (app and bundleId). This will then save some small-time for the driver as by default it tries to autodetect these values anyway by analyzing the fetched package’s manifest | ||
| */ | ||
| return SPECIFIC_RELAY_CAPABILITIES_APP.stream() | ||
| .anyMatch( | ||
| name -> | ||
| capabilities.getCapability(name) != null | ||
| && !capabilities.getCapability(name).toString().isEmpty()); | ||
| } |
There was a problem hiding this comment.
I don't understand how this fixes #14247. Moreover, I don't understand why #14247 is not working.
Maybe #14247 should have not been merged, the user had to configure the stereotypes correctly, and either only send browserName or appium:app in the session capabilities.
I like the refactoring but I don't see how specificRelayCapabilitiesAppMatch helps or improves the situation.
There was a problem hiding this comment.
@diemol, I think it not really fix, also there is no unit test to see the capabilities outcomes looks like.
I spent some time to setup end2end for 3 different scenarios, same as the ticket issue description and their PR describe, none of those work correctly to launch the native app in emulator - https://github.com/NDViet/test-grid-relay-appium/blob/main/reproduce/test_native_app.py
Note that, this browserName cap set in Node stereotype, which means they expect that Node can take both hybrid browser session and native app.
For example in a scenario, where request capabilities try to unset browserName, immediately, DefaultSlotMatcher could not match, since browserNameMatch guard the condition as mandatory there.
Signed-off-by: Viet Nguyen Duc <[email protected]>
VietND96
force-pushed
the
relay-node-match
branch
from
4079ca3 to
36f83af
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
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.
Motivation and Context
The fix in PR #14247 looks like not work.
In this fix, try to make logic in DefaultSlotMatcher and RelaySessionFactory to be same. Since before coming to RelaySessionFactory.test(), a basic match of W3C caps has already been done (in DefaultSlotMatcher).
Besides unit tests, here is the end2end tests
Relay Node config
This code for native app can be reached 2 slots
And this code for hybrid browser session, which can be reached 1st slot (platformVersion 14), where
browserNameis set in Node stereotype.Types of changes
Checklist
PR Type
Bug fix, Tests
Description
Enhanced
DefaultSlotMatcherto handle hybrid browser and native app sessions.Added logic to filter conflicting capabilities in
RelaySessionFactory.Introduced new test cases for relay node matching and capability filtering.
Updated Bazel build configuration to include Mockito dependency.
Changes walkthrough 📝
DefaultSlotMatcher.java
Enhanced SlotMatcher for Appium and hybrid session handlingjava/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
specificRelayCapabilitiesAppMatchfor Appium-relatedcapabilities.
RelaySessionFactory.java
Added capability filtering in RelaySessionFactoryjava/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java
filterRelayCapabilitiesto handle conflicting capabilities.DefaultSlotMatcherTest.java
Added tests for DefaultSlotMatcher enhancementsjava/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
RelaySessionFactoryTest.java
Added tests for RelaySessionFactory capability filteringjava/test/org/openqa/selenium/grid/node/relay/RelaySessionFactoryTest.java
filterRelayCapabilitiesmethod.BUILD.bazel
Updated Bazel build for Mockito dependencyjava/test/org/openqa/selenium/grid/node/relay/BUILD.bazel