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

[py] Updated Handling for DetachedShadowRoot Exception #14677

Merged
merged 10 commits into from
Nov 8, 2024

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Oct 29, 2024

User description

Added DetachedShadowRootException to python error handling

Description

Added DetachedShadowRootException
Added parameterized test

Motivation and Context

#13580

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

  • Added a new exception DetachedShadowRootException to handle cases where a shadow root is no longer attached to the DOM.
  • Updated the error handler to map a specific error code to DetachedShadowRootException.
  • Enhanced unit tests to include a test case for DetachedShadowRootException.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Enhancement
__init__.py
Add DetachedShadowRootException to common module                 

py/selenium/common/init.py

  • Added import for DetachedShadowRootException.
  • Included DetachedShadowRootException in the __all__ list.
  • +2/-0     
    exceptions.py
    Define DetachedShadowRootException class                                 

    py/selenium/common/exceptions.py

  • Introduced DetachedShadowRootException class.
  • Documented the purpose of the exception.
  • +3/-0     
    errorhandler.py
    Map DetachedShadowRootException in error handler                 

    py/selenium/webdriver/remote/errorhandler.py

  • Imported DetachedShadowRootException.
  • Mapped error code to DetachedShadowRootException.
  • +3/-0     
    Tests
    error_handler_tests.py
    Add test case for DetachedShadowRootException                       

    py/test/unit/selenium/webdriver/remote/error_handler_tests.py

  • Added test for DetachedShadowRootException.
  • Used parameterized test for error code.
  • +5/-1     

    💡 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

    Incorrect Test Name
    The test function name 'test_raises_exception_for_invalid_selector' does not accurately reflect the exception being tested (DetachedShadowRootException). Consider renaming it to 'test_raises_exception_for_detached_shadow_root' for clarity.

    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 29, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    ✅ Rename test function to accurately reflect its purpose of testing for detached shadow root exceptions
    Suggestion Impact:The test function name was changed from 'test_raises_exception_for_invalid_selector' to 'test_raises_exception_for_detached_shadow_root', aligning with the suggestion to accurately reflect its purpose.

    code diff:

    -def test_raises_exception_for_invalid_selector(handler, code):
    +def test_raises_exception_for_detached_shadow_root(handler, code):

    Rename the test function to better reflect its purpose. The current name suggests
    it's testing for an invalid selector, but it's actually testing for a detached
    shadow root exception.

    py/test/unit/selenium/webdriver/remote/error_handler_tests.py [244-247]

     @pytest.mark.parametrize("code", ErrorCode.DETACHED_SHADOW_ROOT)
    -def test_raises_exception_for_invalid_selector(handler, code):
    +def test_raises_exception_for_detached_shadow_root(handler, code):
         with pytest.raises(exceptions.DetachedShadowRootException):
             handler.check_response({"status": code, "value": "foo"})
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a misleading test function name and proposes a more accurate one, improving code readability and maintainability by aligning the function name with its actual purpose.

    8
    Enhance exception class with a more informative default error message

    Consider adding a more detailed error message to the DetachedShadowRootException
    class to provide more context about the nature of the error.

    py/selenium/common/exceptions.py [289-290]

     class DetachedShadowRootException(WebDriverException):
         """Raised when referenced shadow root is no longer attached to the DOM"""
    +    def __init__(self, msg=None):
    +        super().__init__(msg or "The shadow root is no longer attached to the DOM. This can occur if the element was removed or if the page was navigated.")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a more detailed error message to the exception class provides additional context, which can be helpful for debugging and understanding the nature of the error, thus improving the code's usability and clarity.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 added the C-py label Nov 1, 2024
    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 1, 2024

    @Delta456 Should be all good now

    @AutomatedTester
    Copy link
    Member

    Please change the commit message to reflect the change. The exception already exists and hasn't not been added by this PR

    @shbenzer shbenzer changed the title [py] Add DetachedShadowRoot Exception [py] Added Handling for DetachedShadowRoot Exception Nov 3, 2024
    @shbenzer shbenzer changed the title [py] Added Handling for DetachedShadowRoot Exception [py] Updated Handling for DetachedShadowRoot Exception Nov 3, 2024
    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 3, 2024

    Please change the commit message to reflect the change. The exception already exists and hasn't not been added by this PR

    @AutomatedTester is this better?

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 4, 2024

    It was a missing period... My bad. Should be ready for merging now @AutomatedTester

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 5, 2024

    Failed WebDriverWait test has nothing to do with pr @AutomatedTester

    py/test/selenium/webdriver/common/webdriverwait_tests.py::test_should_wait_until_all_visible_elements_are_found_when_searching_for_many[firefox] FAILED [ 24%]
    _ test_should_wait_until_all_visible_elements_are_found_when_searching_for_many[firefox] _
    driver = <selenium.webdriver.firefox.webdriver.WebDriver (session="9bda52a0-a27c-453f-b75e-7b683abe6c1a")>
    pages = <conftest.pages..Pages object at 0x7f3a1188a9a0>
    def test_should_wait_until_all_visible_elements_are_found_when_searching_for_many(driver, pages):
    pages.load("hidden_partially.html")
    add_visible = driver.find_element(By.ID, "addVisible")
    add_visible.click()
    add_visible.click()
    elements = WebDriverWait(driver, 3).until(EC.visibility_of_all_elements_located((By.CLASS_NAME, "redbox")))
    assert len(elements) == 2
    E assert 1 == 2
    E + where 1 = len([<selenium.webdriver.remote.webelement.WebElement (session="9bda52a0-a27c-453f-b75e-7b683abe6c1a", element="aa6c4e85-d4a9-4c8d-baf7-f5fb0c184eea")>])
    py/test/selenium/webdriver/common/webdriverwait_tests.py:112: AssertionError

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Nov 8, 2024

    @VietND96 these new failures have nothing to do with this code.

    @VietND96 VietND96 merged commit 7aabb8d into SeleniumHQ:trunk Nov 8, 2024
    13 of 14 checks passed
    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