selenium icon indicating copy to clipboard operation
selenium copied to clipboard

[py] support for custom error messages through functions for `WebDriverWait.until/until_not`

Open Delta456 opened this issue 7 months ago • 4 comments

User description

🔗 Related Issues

Implements https://github.com/SeleniumHQ/selenium/issues/14552

💥 What does this PR do?

Allow specifying a callable for the message argument in WebDriverWait.until / until_not

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Tests


Description

  • Allow callable as message in WebDriverWait.until/until_not

  • Update method signatures and error handling for callables

  • Add tests for callable and string timeout messages


Changes walkthrough 📝

Relevant files
Enhancement
wait.py
Add callable support for timeout messages in WebDriverWait

py/selenium/webdriver/support/wait.py

  • Accept callable or string for message in until and until_not
  • Update method signatures and docstrings to reflect new type
  • Evaluate callable message at timeout before raising exception
  • Ensure backward compatibility with string messages
  • +14/-6   
    Tests
    webdriverwait_tests.py
    Add tests for callable and string timeout messages             

    py/test/selenium/webdriver/common/webdriverwait_tests.py

  • Add tests for callable and string timeout messages in WebDriverWait
  • Assert correct message appears in TimeoutException
  • +15/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Delta456 avatar May 08 '25 13:05 Delta456

    PR Reviewer Guide 🔍

    (Review updated until commit https://github.com/SeleniumHQ/selenium/commit/147913aef0c5686ad14df6fc8d53f44f5260a276)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14552 - PR Code Verified

    Compliant requirements:

    • Allow specifying a callable for the message argument in WebDriverWait.until / until_not • Enable constructing error messages using the driver when an error occurs • Make code more readable by avoiding try-catch structures

    Requires further human verification:

    • Support the usage example: WebDriverWait(driver, 10).until(lambda x: x.find_element(By.ID, "someId"), lambda x: f'Message: {x.current_url}') - The implementation accepts callables but the test doesn't verify this exact usage pattern with driver as parameter

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Callable Parameter

    The implementation calls the message callable without any parameters, but the ticket suggests passing the driver to the callable. The type hint suggests Any as the parameter type, but the implementation doesn't pass any parameter.

    final_msg = message() if callable(message) else message
    raise TimeoutException(final_msg, screen, stacktrace)
    
    Test Coverage

    The test uses a lambda without parameters, but doesn't test the case where the callable uses the driver to construct the message, which was the main motivation in the ticket.

        EC.presence_of_element_located((By.ID, "box0")), message=lambda: "custom timeout message"
    )
    

    qodo-code-review[bot] avatar May 08 '25 13:05 qodo-code-review[bot]

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

    selenium-ci avatar May 08 '25 13:05 selenium-ci

    PR Code Suggestions ✨

    Latest suggestions up to 147913a Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Pass exception to callable

    The current implementation calls the callable message without any arguments, but
    the type hint suggests it should accept an argument. Pass the exception
    information to the callable to allow for more informative error messages.

    py/selenium/webdriver/support/wait.py [150]

    -final_msg = message() if callable(message) else message
    +final_msg = message(exc) if callable(message) else message
    
    • [ ] Apply / Chat
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a mismatch between the type hint and implementation. The type hint Callable[[Any], str] indicates the callable should accept an argument, but the code calls message() without arguments. Passing the exception object would allow for more informative dynamic error messages.

    High
    • [ ] Update

    Previous suggestions

    ✅ Suggestions up to commit 147913a
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Pass exception to callable message

    The current implementation calls the callable message without any arguments, but
    the type hint suggests it should accept an argument. Pass the exception
    information to the callable to allow for more contextual error messages.

    py/selenium/webdriver/support/wait.py [150]

    -final_msg = message() if callable(message) else message
    +final_msg = message(exc) if callable(message) else message
    
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies that the callable message is defined to accept a parameter (per the type hint) but is being called without arguments. Passing the exception information would allow for more contextual error messages, which is a significant improvement to the error handling functionality.

    High
    General
    Improve type hint specificity
    Suggestion Impact:The commit changed the type hint for the message parameter, but instead of using Optional[Exception] as suggested, it used Callable[[], str] which removes the parameter entirely from the callable

    code diff:

    -        self, method: Callable[[D], Union[Literal[False], T]], message: Union[str, Callable[[Any], str]] = ""
    +        self, method: Callable[[D], Union[Literal[False], T]], message: Union[str, Callable[[], str]] = ""
    

    The type hint for the message parameter is too generic. Since the callable is
    intended to receive exception information, use a more specific type hint that
    reflects the actual usage pattern.

    py/selenium/webdriver/support/wait.py [19-97]

    -from typing import Any
    +from typing import Any, Optional
     ...
    -message: Union[str, Callable[[Any], str]] = ""
    +message: Union[str, Callable[[Optional[Exception]], str]] = ""
    
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly points out that the type hint for the message parameter could be more specific. Using Optional[Exception] instead of Any would better document the intended usage pattern, improving code clarity and maintainability, though it doesn't affect runtime behavior.

    Low

    qodo-code-review[bot] avatar May 08 '25 13:05 qodo-code-review[bot]

    I was watching it again just now and I was wondering... It hasn't been well thought out yet, but if the message function causes an error, will the original WebDriver error be lost? If so, has there been any consideration as to whether this specification is acceptable?

    moeyashi avatar May 12 '25 09:05 moeyashi

    I was watching it again just now and I was wondering... It hasn't been well thought out yet, but if the message function causes an error, will the original WebDriver error be lost? If so, has there been any consideration as to whether this specification is acceptable?

    I believe it should be acceptable as long you know that you are providing a custom error message. IF you don't want the new error message, you can always leave that parameter.

    Delta456 avatar Jun 18 '25 07:06 Delta456

    I thought we were no longer maintaining these support packages?

    shbenzer avatar Jun 18 '25 10:06 shbenzer

    That's true. We want to avoid making changes here and empower users to do their own.

    diemol avatar Jun 18 '25 10:06 diemol