py: Don't expose 0.0.0.0 as destination IP for tests

lauromoura commented 2 months ago

User description

Description

This commit falls back to use the default localhost IP instead of 0.0.0.0 if we don't want to use the real IP. And if the server is actually using 0.0.0.0, fallback to localhost when generating the URL for the tests.

Motivation and Context

Webkit recently[1] started blocking 0.0.0.0 as destination addresses, and Chrome is also in the process of enabling such change[2].

[1] https://github.com/WebKit/WebKit/commit/e59cd4a4330877f4692ab31caaf5039185e845bf [2] https://chromestatus.com/feature/5106143060033536

Types of changes

[x] 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

[x] I have read the contributing document.
[n/a] My change requires a change to the documentation.
[n/a] I have updated the documentation accordingly.
[n/a] I have added tests to cover my changes.
[x] All new and existing tests passed. (Note: testing locally)

PR Type

Bug fix

Description

Changed the default host from "0.0.0.0" to None in the webserver fixture.
Updated SimpleWebServer to use DEFAULT_HOST_IP if host is None.
Modified where_is method to fallback to localhost if host is "0.0.0.0".

Changes walkthrough 📝

Relevant files

Bug fix

conftest.py Default to `None` instead of "0.0.0.0" for webserver host py/conftest.py Changed the default host from "0.0.0.0" to `None` in the `webserver` fixture.	+1/-1
webserver.py Fallback to `DEFAULT_HOST_IP` and localhost in SimpleWebServer py/test/selenium/webdriver/common/webserver.py Updated `SimpleWebServer` to use `DEFAULT_HOST_IP` if host is `None`. Modified `where_is` method to fallback to localhost if host is "0.0.0.0".	+3/-2

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

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review

Code Redundancy
The line `port = port` seems redundant and does not change the state of the variable. Consider removing it to clean up the code.

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Implement a limit to the retry attempts to prevent infinite loops ___ To avoid potential infinite loops, consider implementing a mechanism to break out of the loop after a certain number of retries or a timeout. [py/test/selenium/webdriver/common/webserver.py [143-144]](https://github.com/SeleniumHQ/selenium/pull/14324/files#diff-d95713c8d59a3b97d00ec8a480caaea72c2709ded87c52781ebaed56c488c457R143-R144) ```diff -while True: +attempt_limit = 5 +while attempt_limit > 0: try: + # decrement attempt_limit on each iteration + attempt_limit -= 1 ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion addresses a significant potential issue by preventing infinite loops, which can cause the program to hang indefinitely. Implementing a retry limit or timeout is a good practice for robustness and reliability.	9
Best practice	Use explicit `None` check for clarity and to prevent bugs ___ Instead of checking for `None` implicitly, use an explicit check to improve readability and avoid potential bugs with falsy values other than `None`. [py/test/selenium/webdriver/common/webserver.py [141]](https://github.com/SeleniumHQ/selenium/pull/14324/files#diff-d95713c8d59a3b97d00ec8a480caaea72c2709ded87c52781ebaed56c488c457R141-R141) ```diff -host = host if host else DEFAULT_HOST_IP +host = host if host is not None else DEFAULT_HOST_IP ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion improves code readability and reduces the risk of bugs by explicitly checking for `None` rather than relying on the truthiness of the `host` variable. This makes the code more maintainable and understandable.	8
Enhancement	Initialize `host` with a default value to ensure it always has a defined value ___ Consider initializing `host` with a default value directly in the conditional expression to ensure that `host` always has a defined value, even if `request.config.getoption("use_lan_ip")` returns `None`. [py/conftest.py [323]](https://github.com/SeleniumHQ/selenium/pull/14324/files#diff-98e9e290ede5d0c7ac9e7df5b49edf63f5df5fcc946b7736e4a6139e4cda26e3R323-R323) ```diff -host = get_lan_ip() if request.config.getoption("use_lan_ip") else None +host = get_lan_ip() if request.config.getoption("use_lan_ip") else 'localhost' ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves code robustness by ensuring `host` always has a defined value, which can prevent potential issues if `request.config.getoption("use_lan_ip")` returns `None`. However, the default value 'localhost' may not be appropriate in all contexts, so it should be considered carefully.	7
Maintainability	Simplify the URL construction logic by consolidating conditions ___ Refactor the conditional logic to handle the `localhost` parameter and the `host` value in a single line to simplify the method logic. [py/test/selenium/webdriver/common/webserver.py [175]](https://github.com/SeleniumHQ/selenium/pull/14324/files#diff-d95713c8d59a3b97d00ec8a480caaea72c2709ded87c52781ebaed56c488c457R175-R175) ```diff -if localhost or self.host == "0.0.0.0": - return f"http://{DEFAULT_HOST}:{self.port}/{path}" +return f"http://{DEFAULT_HOST if localhost or self.host == '0.0.0.0' else self.host}:{self.port}/{path}" ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion simplifies the conditional logic, making the code more concise and potentially easier to read. However, it slightly reduces readability by combining multiple conditions into a single line, which might be harder to understand at a glance.	6

SeleniumHQ / selenium