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]: set consistent polling across java and python for WebDriverWait methods #14626

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

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Oct 20, 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

Fixes #14623

Moved the time.sleep(self._poll) call to occur after the if time.monotonic() > end_time condition in the until and until_not method.

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

  • Fixed the polling logic in the until and until_not methods by moving the time.sleep(self._poll) call to occur after the timeout check. This ensures that the sleep is only executed if the timeout has not been reached, improving the efficiency of the wait mechanism.

Changes walkthrough 📝

Relevant files
Bug fix
wait.py
Adjust polling logic in `until` and `until_not` methods   

py/selenium/webdriver/support/wait.py

  • Moved time.sleep(self._poll) to occur after the timeout check in until
    method.
  • Moved time.sleep(self._poll) to occur after the timeout check in
    until_not method.
  • +2/-2     

    💡 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 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Timing Precision
    The sleep duration might not be precise, potentially affecting the overall wait time. Consider using a more accurate timing mechanism or adjusting for elapsed time.

    Exception Handling
    The until_not method catches all exceptions defined in self._ignored_exceptions. Verify if this broad exception handling is intentional and doesn't mask important errors.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Implement more precise timeout checking and sleep duration calculation

    Consider using a more precise time measurement for the timeout check to ensure
    accurate timing, especially for short timeouts.

    py/selenium/webdriver/support/wait.py [102-104]

    -if time.monotonic() > end_time:
    +remaining_time = end_time - time.monotonic()
    +if remaining_time <= 0:
         break
    -time.sleep(self._poll)
    +time.sleep(min(self._poll, remaining_time))
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances the precision of timeout handling, which is particularly beneficial for short timeouts. It improves both the accuracy and efficiency of the wait mechanism, making it a valuable enhancement.

    8
    Utilize a context manager for improved timeout handling and code structure

    Consider using a context manager to handle the timeout and ensure proper cleanup,
    which can improve code readability and maintainability.

    py/selenium/webdriver/support/wait.py [96-105]

     def until(self, method: Callable[[D], Union[Literal[False], T]], message: str = "") -> T:
         """Calls the method provided with the driver as an argument until the \
         return value does not evaluate to ``False``.
     
         :param method: callable(WebDriver)
         :param message: optional message for :exc:`TimeoutException`
         :returns: the result of the last call to `method`
         :raises: :exc:`selenium.common.exceptions.TimeoutException` if timeout occurs
         """
         screen = None
         stacktrace = None
     
    -    end_time = time.monotonic() + self._timeout
    -    while True:
    -        value = method(self._driver)
    -        if value:
    -            return value
    -        except self._ignored_exceptions as exc:
    -            screen = getattr(exc, "screen", None)
    -            stacktrace = getattr(exc, "stacktrace", None)
    -        if time.monotonic() > end_time:
    -            break
    -        time.sleep(self._poll)
    +    with contextlib.suppress(self._ignored_exceptions):
    +        end_time = time.monotonic() + self._timeout
    +        while time.monotonic() <= end_time:
    +            try:
    +                value = method(self._driver)
    +                if value:
    +                    return value
    +            except self._ignored_exceptions as exc:
    +                screen = getattr(exc, "screen", None)
    +                stacktrace = getattr(exc, "stacktrace", None)
    +            time.sleep(self._poll)
         raise TimeoutException(message, screen, stacktrace)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a context manager can improve code readability and maintainability by handling exceptions more cleanly. However, the suggestion does not address a critical issue, so its impact is moderate.

    6
    Performance
    Add a small initial delay to prevent excessive CPU usage when the condition is immediately met

    Consider adding a small sleep at the beginning of the loop to prevent excessive CPU
    usage in case the condition is immediately met.

    py/selenium/webdriver/support/wait.py [96-104]

     while True:
    +    time.sleep(self._poll)
         value = method(self._driver)
         if value:
             return value
         except self._ignored_exceptions as exc:
             screen = getattr(exc, "screen", None)
             stacktrace = getattr(exc, "stacktrace", None)
         if time.monotonic() > end_time:
             break
    -    time.sleep(self._poll)
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding an initial delay can help reduce CPU usage if the condition is met immediately, but it could also introduce unnecessary latency. The suggestion is contextually relevant but may not be necessary depending on the specific use case.

    5

    💡 Need additional feedback ? start a PR chat

    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.

    [🐛 Bug]: WebDriverWait polling consistency between Java and Python implementations
    2 participants