Skip to content
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

[java] feat: Add method to select options containing the provided text #14426

Merged
merged 14 commits into from
Nov 8, 2024

Conversation

syber911911
Copy link
Contributor

@syber911911 syber911911 commented Aug 22, 2024

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.

Description

Add method to select options containing the provided text.

Motivation and Context

The existing selectByVisibleText method only allowed selecting options that exactly match the provided text. However, when using Selenium for crawling, situations arose where selecting options that contain the provided text became necessary.

As a Korean developer, I often encounter cases where suffixes or particles are appended to words, making exact matches insufficient.

For example, if the option text is "1년납" and the provided text is "1년", there was no way to match these semantically similar terms. Therefore, I added a new method to handle such cases by selecting options that contain the provided text.

Types of changes

  • 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

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

PR Type

Enhancement, Tests


Description

  • Introduced a new method selectByContainsVisibleText in the ISelect interface and its implementation.
  • The method allows selecting options that contain the provided text, supporting both exact and partial matches.
  • Added comprehensive tests to verify the new method's functionality and exception handling.

Changes walkthrough 📝

Relevant files
Enhancement
ISelect.java
Add method to select options containing text                         

java/src/org/openqa/selenium/support/ui/ISelect.java

  • Added a new method selectByContainsVisibleText.
  • Method allows selecting options containing the provided text.
  • +15/-0   
    Select.java
    Implement method to select options by partial text             

    java/src/org/openqa/selenium/support/ui/Select.java

  • Implemented selectByContainsVisibleText method.
  • Handles both exact and partial text matches.
  • Throws exception if no matching options are found.
  • +61/-0   
    Tests
    SelectElementTest.java
    Add tests for selecting options by partial text                   

    java/test/org/openqa/selenium/support/ui/SelectElementTest.java

  • Added test for selectByContainsVisibleText method.
  • Verified selection of options by partial text.
  • Tested exception handling for non-matching text.
  • +17/-0   
    SelectTest.java
    Add mock tests for selectByContainsVisibleText method       

    java/test/org/openqa/selenium/support/ui/SelectTest.java

  • Added mock test for selectByContainsVisibleText.
  • Verified behavior with mock elements.
  • Included exception handling tests.
  • +22/-0   

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

    @CLAassistant
    Copy link

    CLAassistant commented Aug 22, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Performance Concern
    The selectByContainsVisibleText method performs multiple searches and iterations over options, which may be inefficient for large select elements.

    Possible Bug
    The method uses text.contains(" ") to determine whether to use the longest substring without space, which may not cover all cases of compound words or phrases.

    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 22, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling for StaleElementReferenceException when interacting with WebElements

    The method is not handling potential StaleElementReferenceException that might occur
    when interacting with WebElements. Consider adding a try-catch block to handle this
    exception and retry the operation if it occurs.

    java/src/org/openqa/selenium/support/ui/Select.java [202-210]

     for (WebElement option : candidates) {
    -  if (option.getText().contains(trimmed)) {
    -    setSelected(option, true);
    -    if (!isMultiple()) {
    -      return;
    +  try {
    +    if (option.getText().contains(trimmed)) {
    +      setSelected(option, true);
    +      if (!isMultiple()) {
    +        return;
    +      }
    +      matched = true;
         }
    -    matched = true;
    +  } catch (StaleElementReferenceException e) {
    +    // Re-find the element and retry
    +    WebElement refreshedOption = element.findElement(By.xpath(".//option[contains(., " + Quotes.escape(option.getText()) + ")]"));
    +    if (refreshedOption.getText().contains(trimmed)) {
    +      setSelected(refreshedOption, true);
    +      if (!isMultiple()) {
    +        return;
    +      }
    +      matched = true;
    +    }
       }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling for potential StaleElementReferenceException is a significant improvement, increasing the robustness of the code when dealing with dynamic web elements.

    9
    Enhancement
    ✅ Simplify the search text handling by using the trimmed text directly
    Suggestion Impact:The commit simplified the search text handling by using the trimmed text directly in the method selectByContainsVisibleText and deSelectByContainsVisibleText.

    code diff:

           String trimmed = text.trim();
           for (WebElement option : candidates) {
             if (option.getText().contains(trimmed)) {
    +          if (!hasCssPropertyAndVisible(option))
    +            throw new NoSuchElementException("Invisible option with text: " + text);
               setSelected(option, true);
               if (!isMultiple()) {
                 return;
    @@ -343,6 +365,27 @@
         }
       }
     
    +  @Override
    +  public void deSelectByContainsVisibleText(String text) {
    +    if (!isMultiple()) {
    +      throw new UnsupportedOperationException("You may only deselect options of a multi-select");
    +    }
    +
    +    String trimmed = text.trim();
    +    List<WebElement> options =
    +        element.findElements(By.xpath(".//option[contains(., " + Quotes.escape(trimmed) + ")]"));

    The method is using a complex approach to handle spaces in the search text. Consider
    simplifying this logic by directly using the trimmed text for searching, which would
    handle both single and multiple word cases more efficiently.

    java/src/org/openqa/selenium/support/ui/Select.java [191-199]

    -String searchText = text.contains(" ") ? getLongestSubstringWithoutSpace(text) : text;
    +String trimmedText = text.trim();
    +List<WebElement> candidates = trimmedText.isEmpty() 
    +  ? element.findElements(By.tagName("option"))
    +  : element.findElements(By.xpath(".//option[contains(., " + Quotes.escape(trimmedText) + ")]"));
     
    -List<WebElement> candidates;
    -if (searchText.isEmpty()) {
    -  candidates = element.findElements(By.tagName("option"));
    -} else {
    -  candidates = element.findElements(
    -    By.xpath(".//option[contains(., " + Quotes.escape(searchText) + ")]"));
    -}
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Simplifying the logic by using trimmed text directly improves code readability and efficiency, addressing a minor enhancement in the code.

    8
    Performance
    Replace XPath selector with CSS selector for better performance in exact match search

    Consider using a more efficient approach for the initial exact match search. Instead
    of using XPath, you could use CSS selector which is generally faster. Replace the
    XPath selector with a CSS selector for better performance.

    java/src/org/openqa/selenium/support/ui/Select.java [178-180]

     List<WebElement> options =
       element.findElements(
    -    By.xpath(".//option[normalize-space(.) = " + Quotes.escape(text) + "]"));
    +    By.cssSelector("option[text()='" + StringEscapeUtils.escapeEcmaScript(text) + "']"));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to replace XPath with a CSS selector can improve performance, as CSS selectors are generally faster. However, the proposed CSS selector syntax is incorrect for matching text content, which reduces the score.

    7
    Possible issue
    Add a check to return after selecting the first match for single-select dropdowns

    The method is currently selecting all matching options, which might not be the
    intended behavior for single-select dropdowns. Consider adding a check to return
    after selecting the first match for single-select dropdowns.

    java/src/org/openqa/selenium/support/ui/Select.java [182-187]

     for (WebElement option : options) {
       setSelected(option, true);
       if (!isMultiple()) {
         return;
       }
     }
    +if (!isMultiple() && !options.isEmpty()) {
    +  return;
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion correctly identifies a potential issue with single-select dropdowns, but the proposed solution is redundant as the existing code already handles this case with the isMultiple() check.

    5

    Copy link
    Member

    @shs96c shs96c left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you for the PR. This looks good to me. Presumably we should add a matching deselect method, but that can be done in a follow-up PR.

    @syber911911
    Copy link
    Contributor Author

    Thank you for the review. I will request you again after adding the function to deselect with the same concept.

    syber911911 and others added 2 commits September 14, 2024 23:41
    feat:
     - Add method to deselect options containing the provided text
     - Add method for checking element visibility
    
    fix:
     - fix selectByContainsVisibleText method to check element visibility
    @syber911911 syber911911 requested a review from shs96c September 14, 2024 15:13
    @VietND96 VietND96 merged commit b4b8aab into SeleniumHQ:trunk Nov 8, 2024
    31 checks passed
    @VietND96
    Copy link
    Member

    VietND96 commented Nov 8, 2024

    Merged based on approval and CI passed. Thank you, @syber911911 !

    @syber911911 syber911911 deleted the syber911911-feature-branch branch November 9, 2024 13:42
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants