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] complete test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert #15559

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Apr 3, 2025

User description

💥 What does this PR do?

Implements alert test test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert as it was marked as TODO

🔧 Implementation Notes

Uses code from other tests as inspiration

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Tests


Description

  • Implemented the test test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert.

  • Added logic to handle and dismiss alerts in the test.

  • Marked the test as expected to fail on Safari using @pytest.mark.xfail_safari.


Changes walkthrough 📝

Relevant files
Tests
alerts_tests.py
Added and completed alert handling test                                   

py/test/selenium/webdriver/common/alerts_tests.py

  • Completed the TODO test
    test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert.
  • Added logic to trigger and handle alerts using WebDriverWait and
    exception handling.
  • Marked the test as expected to fail on Safari.
  • +9/-3     

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

    qodo-merge-pro bot commented Apr 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No major issues detected

    @titusfortner titusfortner added the C-py Python Bindings label Apr 3, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Missing verification after alert dismissal
    Suggestion Impact:The commit added verification after alert dismissal, but instead of checking the page title as suggested, it verifies the alert is no longer present by trying to access alert.text which should raise NoAlertPresentException

    code diff:

    +    with pytest.raises(NoAlertPresentException):
    +        alert.text

    The test is missing an assertion to verify that the alert was properly
    dismissed. After dismissing the alert, add an assertion to confirm that the
    alert is no longer present and that the page is in the expected state.

    py/test/selenium/webdriver/common/alerts_tests.py [187-196]

     @pytest.mark.xfail_safari
     def test_should_throw_an_exception_if_an_alert_has_not_been_dealt_with_and_dismiss_the_alert(driver, pages):
         pages.load("alerts.html")
         driver.find_element(By.ID, "alert").click()
         WebDriverWait(driver, 5).until(EC.alert_is_present())
     
         with pytest.raises(UnexpectedAlertPresentException):
             driver.find_element(By.ID, "select").click()
         alert = _wait_for_alert(driver)
         alert.dismiss()
    +    
    +    # Verify alert is dismissed
    +    assert "Testing Alerts" == driver.title

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the test lacks verification after dismissing the alert. Adding an assertion to check the page title is a good practice to ensure the alert was properly dismissed and the page returned to its expected state.

    Medium
    • Update

    @VietND96 VietND96 requested a review from titusfortner April 3, 2025 13:01
    @VietND96
    Copy link
    Member

    VietND96 commented Apr 3, 2025

    LGTM, adding @titusfortner to double check.

    @Delta456 Delta456 requested a review from titusfortner April 7, 2025 09:17
    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.

    3 participants