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] Use Config class for http client settings #12033

Closed
wants to merge 3 commits into from

Conversation

titusfortner
Copy link
Member

Description

  • New ClientConfig class with:
    • keep-alive
    • proxy
    • timeout
    • certificate path
  • keep_alive deprecation changed (it was never supported in Service class)
  • ignore_proxy deprecated in Options (not an option)

Motivation and Context

Python implementation of command_executor / RemoteConnection is similar to Java. A user can create the class themselves, but can only use it as a parameter for RemoteWebDriver. Local drivers are subclassed and have their RemoteConnection classes created for them.

Java addressed this with the ClientConfig class. (Ruby has always had a separate http_client, and I haven't looked at what JS or .NET do).

The only way to use a proxy right now is with environment variables. If a user wants to ignore the environment variables they use ignore_local_proxy() on Options class. ClientConfig takes a Proxy instance so the user does not need to set environment variables. Instead of having the user pass ignore_proxy: true, the user would pass proxy: Proxy({"ProxyType": ProxyType.DIRECT}). If that seems too odd, I can add back the ignore_proxy parameter, but only having the "right" proxy object seems more "correct."

Example usage:

proxy = Proxy()
proxy.http = "http://localhost:8080"
client_config = ClientConfig(proxy=proxy, keep_alive=False, timeout=180)
driver = webdriver.Chrome(client_config=client_config)

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2023

Codecov Report

Patch coverage: 60.43% and project coverage change: +0.06 🎉

Comparison is base (2a6b075) 54.85% compared to head (8cd2aa8) 54.91%.

❗ Current head 8cd2aa8 differs from pull request most recent head 8602eaa. Consider uploading reports for the commit 8602eaa to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #12033      +/-   ##
==========================================
+ Coverage   54.85%   54.91%   +0.06%     
==========================================
  Files          86       87       +1     
  Lines        5728     5789      +61     
  Branches      233      232       -1     
==========================================
+ Hits         3142     3179      +37     
- Misses       2353     2378      +25     
+ Partials      233      232       -1     
Impacted Files Coverage Δ
py/selenium/webdriver/webkitgtk/webdriver.py 27.27% <0.00%> (ø)
py/selenium/webdriver/remote/webdriver.py 41.39% <35.29%> (-0.57%) ⬇️
py/selenium/webdriver/remote/remote_connection.py 52.28% <40.00%> (-2.11%) ⬇️
py/selenium/webdriver/firefox/webdriver.py 53.65% <50.00%> (+0.24%) ⬆️
py/selenium/webdriver/remote/client_config.py 62.06% <62.06%> (ø)
py/selenium/webdriver/ie/webdriver.py 79.48% <75.00%> (+1.56%) ⬆️
py/selenium/webdriver/safari/webdriver.py 57.83% <75.00%> (+0.68%) ⬆️
py/selenium/webdriver/chromium/webdriver.py 58.02% <83.33%> (+3.07%) ⬆️
py/selenium/webdriver/chrome/webdriver.py 76.08% <100.00%> (+3.17%) ⬆️
...y/selenium/webdriver/chromium/remote_connection.py 100.00% <100.00%> (ø)
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@titusfortner titusfortner force-pushed the py_client_config branch 2 times, most recently from 15564a9 to b4fb1e1 Compare May 14, 2023 18:24
@AutomatedTester
Copy link
Member

Initial pass through and it looks ok

@titusfortner titusfortner added this to the 4.11 milestone May 18, 2023
@titusfortner titusfortner marked this pull request as draft May 27, 2023 17:47
@titusfortner
Copy link
Member Author

This needs rebasing after other PR merged

@titusfortner titusfortner removed this from the 4.11 milestone Jun 27, 2023
@@ -152,10 +159,17 @@ def _get_connection_manager(self):

return urllib3.PoolManager(**pool_manager_init_args)

def __init__(self, remote_server_addr: str, keep_alive: bool = False, ignore_proxy: bool = False):
self.keep_alive = keep_alive
def __init__(self, remote_server_addr: str, client_config: ClientConfig = ClientConfig()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in Python, any object that is a function default is "cached", unlike Ruby, where client_config will always get a new instance of ClientConfig.

This may affect scripts that run multiple browsers in one session, if they don't set a different default client config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the better pattern is to use None as default and then set it in the constructor body?

@isaulv
Copy link
Contributor

isaulv commented Jun 27, 2023

I like this idea of a config class. I would like an easier interface to swap out RemoteConnection for Python as well. I think it's kind of there but it's a process.

@titusfortner
Copy link
Member Author

Well, I'm still not sure the right way to get browser specific features in a remote session. Someone did some magic at one point, but then I think there was an issue. I never followed up.

@titusfortner
Copy link
Member Author

I'm breaking this up into pieces for my sanity and to make sure I'm doing this properly.
See: #12364

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants