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] fix tests in correct_event_firing_tests.py #14510

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Sep 17, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixed the TODO tests in correct_event_firing_tests.py. The 2 tests were calling the wrong _assertEventFired function earlier instead of the correct _assert_event_fired function.

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

  • Fixed two tests in correct_event_firing_tests.py that were previously marked as TODO due to failures.
  • Corrected the function call from _assertEventFired to _assert_event_fired to properly verify event firing.
  • Removed comments indicating the tests were failing, as they are now functional.

Changes walkthrough 📝

Relevant files
Bug fix
correct_event_firing_tests.py
Fix and enable previously failing event firing tests         

py/test/selenium/webdriver/common/correct_event_firing_tests.py

  • Fixed two previously failing tests by using the correct
    _assert_event_fired function.
  • Removed TODO comments indicating the tests were failing.
  • Ensured the tests check for the correct event firing: blur and focus.
  • +14/-15 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Test Improvement
    Two previously failing tests have been fixed by using the correct function name _assert_event_fired instead of _assertEventFired.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use an explicit wait for the event to be fired instead of immediate assertion

    Consider adding an explicit wait for the event to be fired instead of using a fixed
    delay. This can make the test more reliable and faster in most cases.

    py/test/selenium/webdriver/common/correct_event_firing_tests.py [126-127]

     element.send_keys("foo")
    -_assert_event_fired(driver, "focus")
    +WebDriverWait(driver, 10).until(lambda d: _assert_event_fired(d, "focus"))
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using an explicit wait is a best practice that can make tests more reliable and faster, as it waits only as long as necessary for the event to fire.

    8
    Enhancement
    Add assertions to verify the state of elements after sending keys

    Consider adding assertions to verify the state of the elements after sending keys.
    This can help ensure that the keys were actually sent and processed correctly.

    py/test/selenium/webdriver/common/correct_event_firing_tests.py [125-127]

     element = driver.find_element(By.ID, "theworks")
     element.send_keys("foo")
     _assert_event_fired(driver, "focus")
    +assert element.get_attribute("value") == "foo"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding assertions to verify the state of elements enhances test robustness by ensuring that the keys were sent and processed correctly, improving test reliability.

    7
    Possible issue
    Add a small delay after sending keys to ensure events have time to fire

    Consider adding a small delay using time.sleep() after sending keys to ensure the
    events have time to fire before asserting. This can help prevent flaky tests.

    py/test/selenium/webdriver/common/correct_event_firing_tests.py [117-120]

     element.send_keys("foo")
     element2 = driver.find_element(By.ID, "changeable")
     element2.send_keys("bar")
    +time.sleep(0.1)  # Small delay to ensure event has time to fire
     _assert_event_fired(driver, "blur")
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a small delay can help prevent flaky tests by ensuring events have time to fire, but it may not be the most efficient solution compared to using explicit waits.

    5

    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.

    2 participants