Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Mar 15, 2025

User description

Motivation and Context

Contributes to #15407

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

  • Refactored Locator types to non-nested classes for better extensibility.

  • Updated Locator type references in test cases to new non-nested types.

  • Simplified and clarified type definitions for AccessibilityLocator, CssLocator, InnerTextLocator, and XPathLocator.


Changes walkthrough 📝

Relevant files
Enhancement
Locator.cs
Refactored `Locator` types to non-nested classes                 

dotnet/src/webdriver/BiDi/Modules/BrowsingContext/Locator.cs

  • Refactored Locator types to non-nested classes.
  • Renamed nested types like Locator.Css to CssLocator.
  • Simplified AccessibilityLocator and other related type definitions.
  • +19/-20 
    Tests
    BrowsingContextTest.cs
    Updated test to use new `CssLocator` type                               

    dotnet/test/common/BiDi/BrowsingContext/BrowsingContextTest.cs

    • Updated test to use CssLocator instead of Locator.Css.
    +1/-1     
    CombinedInputActionsTest.cs
    Updated test to use new `CssLocator` type                               

    dotnet/test/common/BiDi/Input/CombinedInputActionsTest.cs

    • Updated test to use CssLocator instead of Locator.Css.
    +1/-1     
    SetFilesTest.cs
    Updated test to use new `CssLocator` type                               

    dotnet/test/common/BiDi/Input/SetFilesTest.cs

    • Updated test to use CssLocator instead of Locator.Css.
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15407 - PR Code Verified

    Compliant requirements:

    • Convert nested DTO types to non-nested types
    • Change Locator.Css nested type to CssLocator type
    • Make similar changes to all types with similar pattern

    Requires further human verification:

    • Ensure this change enables future static factory methods like Locator.Css("div")
    • Ensure this change enables static singleton patterns
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Accessibility Naming

    The AccessibilityLocator now has a nested type with the same name pattern (AccessibilityLocatorValue). This might lead to similar naming conflicts in the future that this PR aims to solve.

    public record AccessibilityLocatorValue {
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 15, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Remove nested record type

    The nested record AccessibilityLocatorValue inside AccessibilityLocator creates
    an unnecessary nesting that contradicts the PR's goal of making locator types
    non-nested. Consider moving AccessibilityLocatorValue to the namespace level
    like the other locator types.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/Locator.cs [31-38]

    -public record AccessibilityLocator(AccessibilityLocator.AccessibilityLocatorValue Value) : Locator +public record AccessibilityLocator(AccessibilityLocatorValue Value) : Locator; + +public record AccessibilityLocatorValue { - public record AccessibilityLocatorValue - { - public string? Name { get; set; } - public string? Role { get; set; } - } + public string? Name { get; set; } + public string? Role { get; set; } }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies an inconsistency in the PR's refactoring approach. While the PR moved other locator types from nested to non-nested classes, it left AccessibilityLocatorValue as a nested record, contradicting the overall refactoring goal of flattening the class hierarchy.

    High
    • Update
    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Cool progress!

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Nice to see the missing locator type filled in!

    @nvborisenko nvborisenko merged commit fab21ed into SeleniumHQ:trunk Mar 17, 2025
    8 of 10 checks passed
    @nvborisenko nvborisenko deleted the biid-locator-neted branch March 17, 2025 16:17
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    2 participants