selenium icon indicating copy to clipboard operation
selenium copied to clipboard

Only ignore extension caps with object/array values

Open sbabcoc opened this issue 1 year ago • 2 comments
trafficstars

User description

Description

The existing handling of extension capabilities assigns special significance to four recognized prefixes: goog:, moz:, ms:, and se:. 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 with complex values (objects and arrays) can cause affected newSession requests to fail even though extension capabilities are supposed to be vendor-specific and should probably not be evaluated by the default slot matcher.

As a compromise, this PR eliminates the "special" status of the four recognized prefixes, opting instead to ignore all extension capabilities with complex values. This maintains the existing behavior regarding the Options objects of Chrome, Firefox, and Edge while allowing node configurations and newSession requests to include extension capabilities that won't affect slot matching.

Motivation and Context

Revolves #14461

Note that this is technically a breaking change, because it revises the default slot matcher to consider extension capabilities with simple values that would previously have been ignored for the "special" prefixes, and the matcher will now ignore extension capabilities with complex values that would previously have been considered for "non-special" prefixes. However, I'm unaware of any current use cases that will be adversely affected by this change.

The strategy employed by the PR is that extension capabilities which should be considered for matching can be expressed as capabilities with simple values and those which should be ignored can be specified as map elements or list items. This is a generalization of existing patterns from current use cases:

  • The "special" extension prefixes for browser options are expressed as maps, and these are currently ignored for purposes of slot matching.
  • The extension capabilities for Appium are expressed as simple values, and these are currently considered for purposes of slot matching.

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:

Boolean matches(Capabilities stereotype, Capabilities capabilities);

The default implementation would return null to indicate that no evaluation has been performed. If the vendor chooses to implement the matches method, their initial step must be to invoke their own isSupporting method, returning null 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 return true. The matches 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 simple Boolean. This would enable best-match behavior. Perhaps this is getting too complicated, though.

Types of changes

  • [X] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [X] I have read the contributing document.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [X] I have added tests to cover my changes.
  • [X] All new and existing tests passed.

PR Type

Bug fix, Tests


Description

  • Removed the special status of certain extension capability prefixes in DefaultSlotMatcher, opting to ignore all extension capabilities with complex values.
  • Simplified the logic for matching capabilities by removing unnecessary checks and conditions.
  • Updated RelaySessionFactory to streamline session creation by removing redundant capability filtering.
  • Revised test cases in DefaultSlotMatcherTest to align with the new handling of extension capabilities, using maps for complex values.

Changes walkthrough 📝

Relevant files
Bug fix
DefaultSlotMatcher.java
Revise extension capabilities handling in DefaultSlotMatcher

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java

  • Removed special handling for specific extension capability prefixes.
  • Updated logic to ignore all extension capabilities with complex
    values.
  • Simplified capability matching logic.
  • +31/-51 
    RelaySessionFactory.java
    Simplify session creation in RelaySessionFactory                 

    java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java

  • Removed logic to filter out browserName when appium:app is present.
  • Simplified session creation process.
  • +0/-11   
    Tests
    DefaultSlotMatcherTest.java
    Update DefaultSlotMatcher tests for revised capability handling

    java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java

  • Updated test cases to reflect changes in extension capability
    handling.
  • Used maps for complex capability values in tests.
  • +10/-8   

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    sbabcoc avatar Sep 10 '24 18:09 sbabcoc