You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR remove support for GLOBAL_DEFAULT_TIMEOUT environment variable (which is currently not working anyway).
I also added a test to verify that you can set the HTTP client timeout on webdriver.Remote. This doesn't currently work for other drivers (see: #15672)
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix, Tests
Description
Remove support for GLOBAL_DEFAULT_TIMEOUT environment variable
Update timeout logic to use socket default only
Add test verifying HTTP client timeout in Remote WebDriver
Improve test skipping logic for remote driver tests
Changes walkthrough 📝
Relevant files
Bug fix
client_config.py
Remove GLOBAL_DEFAULT_TIMEOUT and simplify timeout logic
py/selenium/webdriver/remote/client_config.py
Remove logic for GLOBAL_DEFAULT_TIMEOUT environment variable
The exception handling is too broad. If request.config.option.drivers exists but is empty, it will raise an IndexError, but your code only catches AttributeError and TypeError. This could lead to unexpected errors.
try:
driver_option = request.config.option.drivers[0]
-except (AttributeError, TypeError):+except (AttributeError, TypeError, IndexError):
raise Exception("This test requires a --driver to be specified")
Apply this suggestion
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a potential bug where an IndexError could occur if request.config.option.drivers exists but is empty. Adding IndexError to the exception handling would make the code more robust and prevent unexpected crashes.
Medium
General
Improve timeout test reliability
The test assumes that ReadTimeoutError will always be raised, but this might not be reliable. The timeout behavior could vary based on network conditions or server response times, making the test flaky.
def test_remote_webdriver_with_http_timeout(firefox_options, webserver):
"""This test starts a remote webdriver with an http client timeout
set less than the implicit wait timeout, and verifies the http timeout
is triggered first when waiting for an element.
"""
http_timeout = 6
wait_timeout = 8
server_addr = f"http://{webserver.host}:{webserver.port}"
client_config = ClientConfig(remote_server_addr=server_addr, timeout=http_timeout)
assert client_config.timeout == http_timeout
with webdriver.Remote(options=firefox_options, client_config=client_config) as driver:
driver.get(f"{server_addr}/simpleTest.html")
driver.implicitly_wait(wait_timeout)
+ start_time = time.time()
with pytest.raises(ReadTimeoutError):
driver.find_element(By.ID, "no_element_to_be_found")
+ elapsed_time = time.time() - start_time+ # Verify timeout occurred closer to http_timeout than wait_timeout+ assert elapsed_time < wait_timeout, f"Test took {elapsed_time}s, expected < {wait_timeout}s"
Apply this suggestion
Suggestion importance[1-10]: 7
__
Why: The suggestion adds time measurement and verification to ensure the test is actually failing due to the HTTP timeout rather than the implicit wait timeout. This makes the test more reliable and verifiable, providing better validation of the timeout behavior being tested.
Medium
Learned best practice
Improve error message clarity
Improve error message clarity by providing more specific details about what went wrong and how to resolve it. The current error message doesn't explain what the --driver option is or how to use it.
try:
driver_option = request.config.option.drivers[0]
except (AttributeError, TypeError):
- raise Exception("This test requires a --driver to be specified")+ raise Exception("This test requires a --driver option to be specified. Use '--driver=firefox' or another supported driver name when running the test.")
✅ Ensure proper resource cleanupSuggestion Impact:The suggestion recommended using a try-finally block to ensure driver cleanup. The commit implemented this concept but used a more elegant solution with a context manager (using 'with' statement) which automatically handles resource cleanup, achieving the same goal of ensuring the driver is properly closed.
code diff:
+ with webdriver.Remote(options=firefox_options, client_config=client_config) as driver:+ driver.get(f"{server_addr}/simpleTest.html")+ driver.implicitly_wait(wait_timeout)+ with pytest.raises(ReadTimeoutError):+ driver.find_element(By.ID, "no_element_to_be_found")
Ensure the driver is properly closed even if the test fails by using a try-finally block. Currently, if the test fails before reaching driver.quit(), the driver won't be properly cleaned up.
def test_remote_webdriver_with_http_timeout(firefox_options, webserver):
"""This test starts a remote webdriver with an http client timeout
set less than the implicit wait timeout, and verifies the http timeout
is triggered first when waiting for an element.
"""
http_timeout = 6
wait_timeout = 8
server_addr = f"http://{webserver.host}:{webserver.port}"
client_config = ClientConfig(remote_server_addr=server_addr, timeout=http_timeout)
assert client_config.timeout == http_timeout
driver = webdriver.Remote(options=firefox_options, client_config=client_config)
- driver.get(f"{server_addr}/simpleTest.html")- driver.implicitly_wait(wait_timeout)- with pytest.raises(ReadTimeoutError):- driver.find_element(By.ID, "no_element_to_be_found")- driver.quit()+ try:+ driver.get(f"{server_addr}/simpleTest.html")+ driver.implicitly_wait(wait_timeout)+ with pytest.raises(ReadTimeoutError):+ driver.find_element(By.ID, "no_element_to_be_found")+ finally:+ driver.quit()
[Suggestion has been applied]
Suggestion importance[1-10]: 8
__
Why: This suggestion addresses a potential resource leak by ensuring the driver is properly closed even if the test fails. This is an important improvement for test reliability and system resource management.
Medium
Improve error message clarity
Use a more specific exception message that explains what the user needs to do. The current message doesn't provide enough guidance on how to fix the issue.
try:
driver_option = request.config.option.drivers[0]
except (AttributeError, TypeError):
- raise Exception("This test requires a --driver to be specified")+ raise Exception("This test requires a --driver to be specified. Run with '--driver=firefox' or another supported driver.")
Suggestion importance[1-10]: 5
__
Why: The suggestion improves the error message by providing specific guidance on how to fix the issue, making it more user-friendly. This is a moderate improvement to usability but doesn't fix a critical bug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
🔗 Related Issues
Fixes #15604
Supersedes #15628
💥 What does this PR do?
This PR remove support for GLOBAL_DEFAULT_TIMEOUT environment variable (which is currently not working anyway).
I also added a test to verify that you can set the HTTP client timeout on
webdriver.Remote. This doesn't currently work for other drivers (see: #15672)🔄 Types of changes
PR Type
Bug fix, Tests
Description
Remove support for GLOBAL_DEFAULT_TIMEOUT environment variable
Update timeout logic to use socket default only
Add test verifying HTTP client timeout in Remote WebDriver
Improve test skipping logic for remote driver tests
Changes walkthrough 📝
client_config.py
Remove GLOBAL_DEFAULT_TIMEOUT and simplify timeout logicpy/selenium/webdriver/remote/client_config.py
remote_connection.py
Remove GLOBAL_DEFAULT_TIMEOUT from RemoteConnectionpy/selenium/webdriver/remote/remote_connection.py
remote_connection_tests.py
Add test for Remote WebDriver HTTP timeoutpy/test/selenium/webdriver/remote/remote_connection_tests.py
conftest.py
Improve remote test skipping in firefox_options fixturepy/conftest.py
drivers