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

[dotnet] Fix RelativeBy.Near and empty list return, port Java tests #14737

Merged
merged 11 commits into from
Nov 14, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 10, 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

Bug 1:
The Near method is now fixed, with a "blind" copy of what the Java code is doing.

Bug 2:
When querying for elements using the relative locator, sometimes you are returned nothing. In that case, the response JSON is [], and the de-serializer doesn't know you're getting back web elements.

As such, the response object has a runtime type of ReadOnlyCollection<object> and the cast as ReadOnlyCollection<IWebElement> fails (ie, returns null).

I can't think of a more elegant solution that doesn't require gutting the JavaScript execution process.

One more reason the as operator is not the right tool when the type is expected to be an exact match 🫠

Motivation and Context

Fixes #13610

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

Bug fix, Tests


Description

  • Fixed the issue where relative locators returned null instead of an empty collection when no matches are found.
  • Added logic to handle the deserialization quirk by returning an empty collection.
  • Introduced a new test to verify that an empty list is returned when no elements are found using relative locators.

Changes walkthrough 📝

Relevant files
Bug fix
RelativeBy.cs
Handle empty results in relative locators                               

dotnet/src/webdriver/RelativeBy.cs

  • Added handling for empty results when using relative locators.
  • Implemented conditional logic to return an empty collection when no
    elements are found.
  • Included debug assertion for unreachable code paths.
  • +27/-2   
    Tests
    RelativeLocatorTest.cs
    Add test for empty result in relative locators                     

    dotnet/test/common/RelativeLocatorTest.cs

  • Added a test to verify behavior when no elements are found using
    relative locators.
  • Ensured the test checks for an empty list return.
  • +11/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Code Smell
    The unreachable code block with Debug.Fail() should be reviewed. If this code path is truly unreachable, consider removing it entirely rather than keeping dead code.

    Type Safety
    The final fallback cast to ReadOnlyCollection could throw an InvalidCastException. Consider adding explicit type checking or error handling.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 10, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    ✅ Replace debug assertion with exception throw to properly handle unexpected states in production code
    Suggestion Impact:The Debug.Fail call was replaced with an exception throw, aligning with the suggestion to handle unexpected states properly. However, the exception type and message differ from the suggestion.

    code diff:

    -                Debug.Fail("Unreachable");
    -                return elementsObj.Select(element => (IWebElement)element).ToList().AsReadOnly();
    -            }
    -
    -            return (ReadOnlyCollection<IWebElement>)rawElements;
    +            }
    +
    +            throw new WebDriverException($"Could not de-serialize element list response{Environment.NewLine}{rawElements}");

    The Debug.Fail call with "Unreachable" suggests this code path should never be hit,
    but the code after it is still executed. Consider throwing an
    InvalidOperationException instead to fail fast if this "impossible" state is
    reached.

    dotnet/src/webdriver/RelativeBy.cs [133-134]

    -Debug.Fail("Unreachable");
    -return elementsObj.Select(element => (IWebElement)element).ToList().AsReadOnly();
    +throw new InvalidOperationException("Unexpected state: ReadOnlyCollection<object> with non-zero elements");
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is a significant improvement for production code reliability. Replacing Debug.Fail with an exception throw ensures proper error handling in all environments and prevents silent failures in production.

    8
    Enhancement
    Simplify type checking and casting logic using pattern matching for better code readability and maintainability

    Consider using pattern matching with 'is' to simplify the type checking and casting
    logic. This would make the code more concise and eliminate the need for an explicit
    cast at the end.

    dotnet/src/webdriver/RelativeBy.cs [114-137]

    -if (rawElements is ReadOnlyCollection<IWebElement> elements)
    +return rawElements switch
     {
    -    return elements;
    -}
    +    ReadOnlyCollection<IWebElement> elements => elements,
    +    ReadOnlyCollection<object> { Count: 0 } => 
    +        #if NET8_0_OR_GREATER
    +            ReadOnlyCollection<IWebElement>.Empty,
    +        #else
    +            new List<IWebElement>().AsReadOnly(),
    +        #endif
    +    _ => (ReadOnlyCollection<IWebElement>)rawElements
    +};
     
    -if (rawElements is ReadOnlyCollection<object> elementsObj)
    -{
    -    if (elementsObj.Count == 0)
    -    {
    -        #if NET8_0_OR_GREATER
    -            return ReadOnlyCollection<IWebElement>.Empty;
    -        #else
    -            return new List<IWebElement>().AsReadOnly();
    -        #endif
    -    }
    -
    -    Debug.Fail("Unreachable");
    -    return elementsObj.Select(element => (IWebElement)element).ToList().AsReadOnly();
    -}
    -
    -return (ReadOnlyCollection<IWebElement>)rawElements;
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion offers a more modern and concise approach using pattern matching, which improves code readability while maintaining the same functionality. However, it's primarily a stylistic improvement rather than a critical fix.

    6

    💡 Need additional feedback ? start a PR chat

    @RenderMichael RenderMichael changed the title [dotnet] Relative locators return empty when no matches found, instead of null [dotnet] Fix RelativeBy.Near, port Java tests Nov 10, 2024
    @RenderMichael RenderMichael changed the title [dotnet] Fix RelativeBy.Near, port Java tests [dotnet] Fix RelativeBy.Near and empty list return, port Java tests Nov 10, 2024
    @RenderMichael
    Copy link
    Contributor Author

    Tests are a one-to-one clone of the java tests: https://github.com/SeleniumHQ/selenium/blob/trunk/java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java

    With the exception of the one C#-deserializer-specific test at the bottom

    @nvborisenko nvborisenko merged commit dd0b2ba into SeleniumHQ:trunk Nov 14, 2024
    10 checks passed
    jkim2492 pushed a commit to jkim2492/selenium that referenced this pull request Nov 17, 2024
    @RenderMichael RenderMichael deleted the relative-by-empty branch November 17, 2024 22:47
    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.

    [🐛 Bug]: "Near" relative locator doesn't work for the dotNet
    2 participants