Skip to content

Fix relativeby annotations #15875

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

Fix relativeby annotations #15875

wants to merge 10 commits into from

Conversation

@ShauryaDusht
Copy link
Contributor

@ShauryaDusht ShauryaDusht commented Jun 8, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

Closes #14881
Closes #15050

💥 What does this PR do?

Adds missing RelativeBy type annotation support to:

  • find_element and find_elements
  • expected_conditions.py (conditions now support RelativeBy locators)

Also adds unit tests to validate these changes.

🔧 Implementation Notes

Updated type hints from Tuple[str, str] to Union[Tuple[ByType, str], Tuple[RelativeBy, None]] where needed.
Minor ShadowRoot update added as per doc coverage.
Bazel issues on Windows remain unresolved.

💡 Additional Considerations

None — ready for review. ShadowRoot usage can be revisited if needed.

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)

Files Changed/Added

  • selenium/webdriver/remote/shadowroot.py
  • selenium/webdriver/remote/webdriver.py
  • selenium/webdriver/remote/webelement.py
  • selenium/webdriver/support/expected_conditions.py
  • test/unit/selenium/webdriver/support/test_expected_conditions_relative_by.py
  • test/unit/selenium/webdriver/test_relative_by_annotations.py

PR Type

Bug fix, Enhancement, Tests

Description

  • Add RelativeBy support to type annotations for find_element/find_elements

    • Applies to WebDriver, WebElement, and ShadowRoot classes

  • Update expected_conditions to accept RelativeBy locators in type hints

  • Add unit tests to validate RelativeBy annotation support

  • Improve type safety and error handling in BiDi browsing context classes

Changes walkthrough 📝

Relevant files
Enhancement
4 files
webdriver.py
Add RelativeBy to find_element(s) type annotations             
+7/-6     
webelement.py
Add RelativeBy to WebElement find_element(s) type annotations
 +8/-3     
shadowroot.py
Add RelativeBy to ShadowRoot find_element(s) type annotations
 +6/-3     
expected_conditions.py
Support RelativeBy in expected_conditions locator type hints
 +16/-12 
Tests
2 files
test_relative_by_annotations.py
Add unit tests for RelativeBy annotation support                 
+65/-0   
test_expected_conditions_relative_by.py
Add unit tests for expected_conditions with RelativeBy     
+57/-0   
Bug fix
3 files
browsing_context.py
Improve type safety and error handling in BiDi context     
+141/-45
action_chains.py
Fix type for pause method argument                                             
+1/-1     
utils.py
Fix type handling in find_connectable_ip and keys_to_typing
 +4/-5     
Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Jun 8, 2025
    @selenium-ci
    Copy link
    Member

    selenium-ci commented Jun 8, 2025

    Thank you, @ShauryaDusht 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

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 8, 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

    Type Conversion

    The change to pause() method converts seconds to int for key_action but not for pointer_action. This could lead to inconsistent behavior if floating point values are passed.

     
    self.w3c_actions.pointer_action.pause(seconds)
self.w3c_actions.key_action.pause(int(seconds))
    String Conversion

    The changes in find_connectable_ip() now explicitly convert sockaddr[0] to string, but there's no validation that the conversion is always safe or necessary.

     
        connectable = is_connectable(port, str(sockaddr[0]))

if connectable and family == socket.AF_INET:
    return str(sockaddr[0])
if connectable and not ip and family == socket.AF_INET6:
    ip = str(sockaddr[0])
    Error Handling

    Several new validation checks were added but the error handling is inconsistent - some methods raise ValueError while others silently return incomplete objects.

     
    context = json.get("context")
if context is None or not isinstance(context, str):
    raise ValueError("context is required and must be a string")

navigation = json.get("navigation")
if navigation is not None and not isinstance(navigation, str):
    raise ValueError("navigation must be a string")

timestamp = json.get("timestamp")
if timestamp is None or not isinstance(timestamp, int):
    raise ValueError("timestamp is required and must be an integer")

url = json.get("url")
if url is None or not isinstance(url, str):
    raise ValueError("url is required and must be a string")

    @selenium-ci selenium-ci closed this Jun 8, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix inconsistent pause behavior

    The pause method is inconsistent between pointer_action and key_action. The
    pointer_action accepts float values while key_action forces conversion to int,
    which could cause unexpected behavior when using fractional seconds.

    py/selenium/webdriver/common/action_chains.py [275-276]

     self.w3c_actions.pointer_action.pause(seconds)
-self.w3c_actions.key_action.pause(int(seconds))
+self.w3c_actions.key_action.pause(seconds)
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies an inconsistency where pointer_action.pause() accepts float values while key_action.pause() forces int conversion. However, this suggests reverting an intentional change made in the PR, which may have been done for a specific reason.

    		Low
    • More

    @ShauryaDusht ShauryaDusht deleted the fix-relativeby-annotations branch June 8, 2025 10:54
    @ShauryaDusht
    Copy link
    Contributor Author

    ShauryaDusht commented Jun 8, 2025

    Closed : messed up in fetching from "trunk" branch

    @ShauryaDusht ShauryaDusht restored the fix-relativeby-annotations branch June 8, 2025 10:57
    @ShauryaDusht ShauryaDusht deleted the fix-relativeby-annotations branch June 8, 2025 14:31
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes C-py Python Bindings Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Missing support for RelativeBy in annotations

    3 participants

    @ShauryaDusht @selenium-ci @cgoldberg