Skip to content

Conversation

@gryznar
Copy link

@gryznar gryznar commented Jan 8, 2025

User description

Fixes: #14881

Description

This change adds support for RelativeBy in type annotations

Motivation and Context

Lack of these annotations causes linting issues

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


Description

  • Add RelativeBy to type annotations in element-finding methods

  • Update expected conditions to accept RelativeBy in locators

  • Improve type hinting for better linting and static analysis


Changes walkthrough 📝

Relevant files
Bug fix
shadowroot.py
Add RelativeBy to ShadowRoot element-finding type annotations

py/selenium/webdriver/remote/shadowroot.py

  • Added RelativeBy to type annotations for find_element and
    find_elements
  • Updated import statements for type hinting
  • Improved return type annotations for element-finding methods
  • +15/-5   
    webdriver.py
    Add RelativeBy to WebDriver element-finding type annotations

    py/selenium/webdriver/remote/webdriver.py

  • Updated find_element and find_elements to accept RelativeBy in type
    hints
  • Added ByType import for improved type hinting
  • +3/-2     
    event_firing_webdriver.py
    Add RelativeBy to EventFiringWebDriver element-finding type
    annotations

    py/selenium/webdriver/support/event_firing_webdriver.py

  • Updated find_element and find_elements to accept RelativeBy in type
    hints
  • Added necessary imports for type hinting
  • +6/-2     
    expected_conditions.py
    Add RelativeBy to expected_conditions locator type annotations

    py/selenium/webdriver/support/expected_conditions.py

  • Introduced LocatorType as a union including RelativeBy
  • Updated all locator-based expected condition functions to accept
    RelativeBy
  • Improved type annotations throughout for better static analysis
  • +18/-15 

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

    CLAassistant commented Jan 8, 2025

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 8, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 012d7f7)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14881 - Fully compliant

    Compliant requirements:

    • Add support for RelativeBy in type annotations for find_element() and find_elements() methods
    • Update annotations in expected_conditions.py to support RelativeBy in locators
    • Change locator type annotations from Tuple[str, str] to a union type that includes RelativeBy
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Definition

    The new LocatorType definition uses Tuple[ByType, str] but should also handle the case where value is None for regular locators, not just for RelativeBy.

    LocatorType = Union[Tuple[ByType, str], Tuple[RelativeBy, None]]
    Import Path

    The import for RelativeBy uses an incorrect path. It should be imported from selenium.webdriver.support.relative_locator instead of just .relative_locator.

    from .relative_locator import RelativeBy

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 8, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 012d7f7
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incorrect type definition

    The current LocatorType definition incorrectly assumes RelativeBy always has a
    None value parameter. However, RelativeBy objects are passed as the first
    parameter with no second parameter needed. Update the type definition to
    properly represent both locator styles.

    py/selenium/webdriver/support/expected_conditions.py [48]

    -LocatorType = Union[Tuple[ByType, str], Tuple[RelativeBy, None]]
    +LocatorType = Union[Tuple[ByType, str], RelativeBy]
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the current LocatorType type annotation is inaccurate and could lead to confusion or type errors; updating it to Union[Tuple[ByType, str], RelativeBy] accurately reflects the actual usage and improves type safety.

    Medium
    Handle RelativeBy parameter

    The find_element method accepts RelativeBy in type hints but the implementation
    doesn't handle this case, unlike the WebDriver implementation. Add logic to
    handle RelativeBy objects similar to how it's done in the WebDriver class.

    py/selenium/webdriver/remote/shadowroot.py [48-54]

    +def find_element(self, by: Union[ByType, RelativeBy] = By.ID,
    +                 value: Optional[str] = None) -> WebElement:
    +    """Find an element inside a shadow root given a By strategy and
    +    locator.
     
    +    Parameters:
    +    ----------
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion highlights a real gap: the method advertises support for RelativeBy but does not actually handle it, which could lead to runtime errors or unexpected behavior. However, the improved code does not show the actual fix, so the suggestion is valid but incomplete in its demonstration.

    Medium
    • Update

    Previous suggestions

    Suggestions up to commit 25124be
    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add support for RelativeBy locators in shadow root element search

    The find_element method should handle RelativeBy locators by checking the type and
    handling it appropriately, similar to the implementation in webdriver.py.

    py/selenium/webdriver/remote/shadowroot.py [48-87]

     def find_element(self, by: Union[ByType, RelativeBy] = By.ID,
                      value: Optional[str] = None) -> WebElement:
    +    if isinstance(by, RelativeBy):
    +        elements = by.find_elements(self)
    +        if not elements:
    +            raise NoSuchElementException(f"Cannot locate relative element with: {by.root}")
    +        return elements[0]
    +
         if by == By.CLASS_NAME:
             by = By.CSS_SELECTOR
             value = f".{value}"
         elif by == By.NAME:
             by = By.CSS_SELECTOR
             value = f'[name="{value}"]'
     
         return self._execute(Command.FIND_ELEMENT_FROM_SHADOW_ROOT, {"using": by, "value": value})["value"]
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion adds critical functionality to handle RelativeBy locators in shadow root searches, ensuring consistent behavior with the main WebDriver implementation and preventing potential failures when using relative locators.

    9
    Fix type hint to properly handle RelativeBy locators with values

    The LocatorType type hint should handle the case where RelativeBy is used with a
    non-None value parameter, as RelativeBy locators can have values. Update the type
    hint to Union[Tuple[ByType, str], Tuple[RelativeBy, Optional[str]]].

    py/selenium/webdriver/support/expected_conditions.py [48]

    -LocatorType = Union[Tuple[ByType, str], Tuple[RelativeBy, None]]
    +LocatorType = Union[Tuple[ByType, str], Tuple[RelativeBy, Optional[str]]]
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves type safety by correctly representing that RelativeBy locators can have string values, making the type hint more accurate and preventing potential type checking issues.

    7

    @codecov
    Copy link

    codecov bot commented Jan 9, 2025

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.86%. Comparing base (57f8398) to head (28d4f96).
    Report is 1141 commits behind head on trunk.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #15050      +/-   ##
    ==========================================
    + Coverage   58.48%   58.86%   +0.38%     
    ==========================================
      Files          86       94       +8     
      Lines        5270     6012     +742     
      Branches      220      259      +39     
    ==========================================
    + Hits         3082     3539     +457     
    - Misses       1968     2214     +246     
    - Partials      220      259      +39     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @shbenzer
    Copy link
    Contributor

    shbenzer commented Jan 9, 2025

    @gryznar Thanks for the PR!

    Please run scripts/format.sh to pass the formatting tests

    @gryznar
    Copy link
    Author

    gryznar commented Jan 9, 2025

    Sure. I was not aware of this script and therefore I've formatted manually

    @shbenzer
    Copy link
    Contributor

    shbenzer commented Jan 9, 2025

    Sure. I was not aware of this script and therefore I've formatted manually

    No worries, I didn't know about it either until someone directed me to it

    @gryznar gryznar force-pushed the annotate-relative-by branch from 28d4f96 to 474d3b7 Compare January 9, 2025 23:04
    @gryznar
    Copy link
    Author

    gryznar commented Jan 9, 2025

    Please run scripts/format.sh to pass the formatting tests

    @shbenzer Done

    @gryznar gryznar force-pushed the annotate-relative-by branch from 474d3b7 to d8c871a Compare January 9, 2025 23:19
    @gryznar
    Copy link
    Author

    gryznar commented Jan 9, 2025

    I've also reverted changes in shadowroot.py, because they were causing circular imports

    @VietND96 VietND96 changed the title Support RelativeBy in type annotations [py] Support RelativeBy in type annotations Jan 20, 2025
    @VietND96 VietND96 requested a review from shbenzer February 9, 2025 15:40
    @VietND96 VietND96 added the C-py Python Bindings label Feb 9, 2025
    @shbenzer
    Copy link
    Contributor

    shbenzer commented Feb 9, 2025

    @gryznar Could you please provide some tests for this enhancement? We'll need to ensure that RelativeBy is fully handleable here before we can roll this out.

    @gryznar
    Copy link
    Author

    gryznar commented Feb 10, 2025

    I'll try

    @gryznar
    Copy link
    Author

    gryznar commented Mar 6, 2025

    Sorry, that this is taking so long from my side, but I am unsure where to put these tests and what would they to cover?

    @cgoldberg
    Copy link
    Member

    Is this still relevant? If so, can you fix the merge conflicts?

    @shbenzer
    Copy link
    Contributor

    shbenzer commented May 13, 2025

    Sorry, that this is taking so long from my side, but I am unsure where to put these tests and what would they to cover?

    To answer this question:

    • You would add additional tests to where these files are currently tested (eg. /py/test/.../path/to/test)
    • The tests would ensure that RelativeBys can be passed in each instance you altered

    @gryznar
    Copy link
    Author

    gryznar commented May 13, 2025

    To answer this question:

    • You would add additional tests to where these files are currently tested (eg. /py/test/.../path/to/test)
    • The tests would ensure that RelativeBys can be passed in each instance you altered

    Thanks for the answer! I'll try to both resolve conflicts and add these tests this week

    @cgoldberg
    Copy link
    Member

    @gryznar if you have any questions about adding tests, join us on Slack: https://inviter.co/seleniumhq

    @gryznar gryznar force-pushed the annotate-relative-by branch 6 times, most recently from e95b544 to 1e84a2b Compare May 17, 2025 13:38
    @gryznar gryznar force-pushed the annotate-relative-by branch 9 times, most recently from d242ef8 to 0f00068 Compare May 17, 2025 14:10
    @gryznar gryznar closed this May 17, 2025
    @gryznar gryznar force-pushed the annotate-relative-by branch from 0f00068 to 7d8068d Compare May 17, 2025 14:14
    @shbenzer
    Copy link
    Contributor

    Are you no longer working on this @gryznar?

    @gryznar
    Copy link
    Author

    gryznar commented May 17, 2025

    I've broken something and trying to fix

    Edit: Done, conflicts also solved. Now I am trying to add tests and check if everything works

    @gryznar gryznar force-pushed the annotate-relative-by branch from 012d7f7 to 080a63e Compare May 17, 2025 15:39
    @cgoldberg
    Copy link
    Member

    This broke every single test :)

    Check out the CI logs... I think there is a circular import.

    @gryznar
    Copy link
    Author

    gryznar commented May 20, 2025

    Yeah, I am aware of this. I was trying to set up Bazel environment (this is not obvious) and almost figure out that. I will take care of it in a bit

    @cgoldberg
    Copy link
    Member

    cgoldberg commented May 20, 2025

    no problem. Feel free to ask for help in our Slack if you can't get bazel working:
    https://www.selenium.dev/support/#ChatRoom

    You can also do the build with bazel (./go py:local_dev -- --trace) and then run tests with PyTest. That's what I do for local development since bazel is kind of slow.

    @gryznar
    Copy link
    Author

    gryznar commented May 20, 2025

    Nice trick, thanks!

    @gryznar
    Copy link
    Author

    gryznar commented Jun 8, 2025

    I didn't manage to run bazel tests on Windows (some strange errors from js I think). Closing and sorry for the whole churn

    @gryznar gryznar closed this Jun 8, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Missing support for RelativeBy in annotations

    5 participants