Skip to content

Conversation

@iFatRain
Copy link

@iFatRain iFatRain commented Apr 9, 2025

User description

🔗 Related Issues

Potentially addresses comment in #3680 -> #3680 (comment)

💥 What does this PR do?

This PR adds functionality and mechanisms to support out-of-the-box element extensibility and containerization of groupings for easy reuse. The intent is to provide a simple and backwards compatible way for a user to define their own types of elements and groupings, leveraging the default PageFactory annotations and its mechanisms, with the newly added AbstractExtendedElement. Previously if a user wanted this type of functionality, you typically had to create your own solution using custom page factories, decorators, handlers ect. This is still possible of course, but this PR provides a simpler out of the box solution. This implementation uses just simple classes that would extend the AbstractExtendedElement, as opposed to a solution involving Interfaces and Implementing classes. This keeps the usage simple and new classes smaller.

🔧 Implementation Notes

The base philosophy has been around for many years. Developers create reusable pieces that get populated with differing sets of data from API calls; Why don't we enable a framework design pattern which mimics that behavior? This PR aims to do just that. It seeks to maintain a PageObject as a high level abstraction, and EVERYTHING contained within to be elements (As opposed to Pages of Pages) and can be seen somewhat analogous to "Page Fragments" mentioned in some early literature, but maintains the WebElement as the main point of reference (anything with a tag can be an element after all 👍). Because we treat even the container as an element itself, we can also pass the container element into relevant functions ( like waits/checks for presence and interactibility ect.) which can be very useful.

💡 Additional Considerations

The changes are meant to be backwards compatible, as well as provide a potentially desired new way of doing Selects out of the box. Of course because of this implementation, the old way of doing things is still valid; ElementType element = new ElementType(baseElement); There is also the idea in the future to extend on top of the .findElement and .findElements to return beyond just a WebElement with something such as a .findElementAs(By by, Class typeToResolveAs) and .findElementsAs(By by, Class typeForListToContain) to allow for similar non-page factory usages and directly resolve to the custom elements a user can define.
By implementing as a class instead of interfaces and implementing classes it also opens the door to a future change I have been experimenting with; Annotations which function similar to some seen in Appium for context specific overrides over the class to resolve to. The base theory I have been testing for this annotation would allow a user to define under what conditions should the override take place, and when those conditions evaluate to true, what alternative element class should be created over the default. In Appium this is often seen between different OS platforms, but I wanted it to be more generic and user-defined allows potential for browser specific, environment specific, url specific ect; a example use case would be if a user had different urls or environments in which you need to test across where the locators or implementation may be different (A project overhaul or something).

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Documentation


Description

  • Introduced AbstractExtendedElement for custom reusable elements.

  • Enhanced DefaultFieldDecorator to support extended elements.

  • Updated LocatingElementListHandler for extended element lists.

  • Extended Select class to inherit from AbstractExtendedElement.

  • Added tests for extended elements and Select integration.

  • Updated Bazel build files for new dependencies.


Changes walkthrough 📝

Relevant files
Enhancement
4 files
DefaultFieldDecorator.java
Enhanced DefaultFieldDecorator to support extended elements
+36/-11 
LocatingElementListHandler.java
Updated LocatingElementListHandler for extended element lists
+32/-1   
AbstractExtendedElement.java
Introduced AbstractExtendedElement for reusable elements 
+45/-0   
Select.java
Extended Select class to support AbstractExtendedElement 
+2/-2     
Tests
2 files
PageFactoryTest.java
Added tests for extended elements in PageFactory                 
+79/-5   
SelectElementTest.java
Added tests for Select integration with extended elements
+33/-0   
Configuration changes
2 files
BUILD.bazel
Updated Bazel build file for new dependencies                       
+2/-0     
BUILD.bazel
Updated Bazel build file for AbstractExtendedElement         
+2/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • iFatRain added 4 commits April 9, 2025 11:54
    This change adds an abstract extensible element which provide an 'out-of-the-box' way for users to define custom containerized element groupings, or singular elements with specific behaviors. The design uses only classes instead of interfaces with implementing classes to keep usage simplistic and allows for future annotations to enable context specific overriding. The change should be backwards compatible.
    
    Potentially addresses comment in SeleniumHQ#3680 SeleniumHQ#3680 (comment)
    @CLAassistant
    Copy link

    CLAassistant commented Apr 9, 2025

    CLA assistant check
    All committers have signed the CLA.

    @selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations B-support Issue or PR related to support classes labels Apr 9, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential NullPointerException

    The invoke method creates elementList but only populates it when isExtendedElement is true. If isExtendedElement is false, the method attempts to return method.invoke on elements, but if an exception occurs, elementList would be empty when referenced in the catch block.

    public Object invoke(Object object, Method method, Object[] objects) throws Throwable {
      List<Object> elementList = new ArrayList<>();
      List<WebElement> elements = locator.findElements();
    
      if(isExtendedElement && null != cons) {
        for (WebElement element : elements) {
          Object extension = cons.newInstance(element);
          elementList.add(listType.cast(extension));
        }
      }
      try {
          return method.invoke(
            isExtendedElement ? elementList : elements,
            objects);
      } catch (InvocationTargetException e) {
        // Unwrap the underlying exception
        throw e.getCause();
      }
    Method Delegation

    The class extends RemoteWebElement but doesn't delegate WebElement method calls to the wrapped element. This could lead to unexpected behavior when WebElement methods are called on the extended element.

    public abstract class AbstractExtendedElement extends RemoteWebElement implements WrapsElement {
    
      private WebElement wrappedElement;
    
      protected AbstractExtendedElement(WebElement element) {
        if (null == element) {
          throw new IllegalArgumentException("Wrapped element must not be null");
        }
        this.wrappedElement = element;
      }
    
      @Override
      public WebElement getWrappedElement() {
        return this.wrappedElement;
      }
    
      @Override
      public String toString() {
        return this.wrappedElement.toString();
      }
    
    }

    @selenium-ci
    Copy link
    Member

    Thank you, @iFatRain for this code suggestion.

    The support packages contain example code that many users find helpful, but they do not necessarily represent
    the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

    We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
    If you have any questions, please contact us

    @selenium-ci selenium-ci closed this Apr 9, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix potential NullPointerException

    The method is missing initialization of elementList when isExtendedElement is
    true but the constructor is null. This can lead to a NullPointerException when
    trying to invoke a method on an empty list. Add a check to ensure elementList is
    properly populated in all cases.

    java/src/org/openqa/selenium/support/pagefactory/internal/LocatingElementListHandler.java [56-74]

     @Override
     public Object invoke(Object object, Method method, Object[] objects) throws Throwable {
       List<Object> elementList = new ArrayList<>();
       List<WebElement> elements = locator.findElements();
     
       if(isExtendedElement && null != cons) {
         for (WebElement element : elements) {
           Object extension = cons.newInstance(element);
           elementList.add(listType.cast(extension));
         }
    +  } else if (isExtendedElement) {
    +    // Handle case where isExtendedElement is true but cons is null
    +    return method.invoke(elements, objects);
       }
       try {
           return method.invoke(
             isExtendedElement ? elementList : elements,
             objects);
       } catch (InvocationTargetException e) {
         // Unwrap the underlying exception
         throw e.getCause();
       }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion correctly identifies a potential NullPointerException when isExtendedElement is true but cons is null. The fix properly handles this edge case by falling back to using the original elements list, preventing a runtime error.

    Medium
    Improve error handling

    Silently returning null when constructor instantiation fails can lead to
    hard-to-debug issues. Instead, throw a more specific exception with details
    about what went wrong to help users understand and fix the problem.

    java/src/org/openqa/selenium/support/pagefactory/DefaultFieldDecorator.java [62-70]

     if (WebElement.class.isAssignableFrom(type)) {
       WebElement elementProxy = proxyForLocator(loader, locator);
       if(AbstractExtendedElement.class.isAssignableFrom(type)) {
         try {
           return type.getConstructor(WebElement.class).newInstance(elementProxy);
         } catch (Exception e) {
    -      return null;
    +      throw new RuntimeException("Failed to instantiate " + type.getName() + 
    +          ". Ensure it has a constructor that accepts WebElement.", e);
         }
       }
       return elementProxy;
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential issue with silently returning null on constructor instantiation failure. Throwing a specific exception with details about what went wrong would make debugging much easier for users.

    Medium
    Learned
    best practice
    Add null checks for collection elements before accessing their members or methods to prevent NullPointerExceptions

    The code doesn't check if each element in the elements list is null before using
    it to create an extension. This could lead to a NullPointerException if any
    element in the list is null. Add a null check for each element before creating
    the extension.

    java/src/org/openqa/selenium/support/pagefactory/internal/LocatingElementListHandler.java [61-66]

     if(isExtendedElement && null != cons) {
       for (WebElement element : elements) {
    -    Object extension = cons.newInstance(element);
    -    elementList.add(listType.cast(extension));
    +    if (element != null) {
    +      Object extension = cons.newInstance(element);
    +      elementList.add(listType.cast(extension));
    +    }
       }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    General
    Remove redundant field

    The element field is redundant since Select now extends AbstractExtendedElement
    which already stores the wrapped element. You should remove this field and use
    getWrappedElement() instead to avoid duplicating state.

    java/src/org/openqa/selenium/support/ui/Select.java [29-41]

    -private final WebElement element;
     private final boolean isMulti;
     
     /**
      * Constructor. A check is made that the given element is, indeed, a SELECT tag. If it is not,
      * then an UnexpectedTagNameException is thrown.
      *
      * @param element SELECT element to wrap
      * @throws UnexpectedTagNameException when element is not a SELECT
      */
     public Select(WebElement element) {
       super(element);
       String tagName = element.getTagName();
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies that the element field is redundant since Select now extends AbstractExtendedElement which already stores the wrapped element. Removing it would improve code maintainability and reduce duplication of state.

    Low
    • More

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    B-build Includes scripting, bazel and CI integrations B-support Issue or PR related to support classes C-java Java Bindings Review effort 3/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants