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.

Sign up for GitHub

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

Jump to bottom

[py] implement configurable configuration class for the http client #13286

Open

titusfortner wants to merge 3 commits into trunk from py_client_config_new

Member

titusfortner commented Dec 11, 2023 •

edited

Description

Updated implementation of [py] Use Config class for http client settings #12033
This deprecates and replaces existing functionality for:
- Remote Connection URL
- Keep Alive
- Ignore Proxy
This adds ability to pass in Proxy as part of implementing ignore proxy replacement
I added typing where it did not exist; I'm not sure I got the "Optional" ones correct
Many of these methods should have docstrings, but I didn't add any where they don't exist

Motivation and Context

Partial implementation of [🚀 Feature]: Configurable HTTP Client settings across bindings #12368

Still need to implement:

connection / open timeout
read timeout
Max Redirects
Certificates

titusfortner added the C-py label

titusfortner requested review from AutomatedTester, isaulv and symonk

December 11, 2023 21:26

titusfortner mentioned this pull request

[🚀 Feature]: HTTP Client Configurations #12268

Closed

codecov-commenter commented Dec 11, 2023 •

edited by codecov bot

Codecov Report

Attention: Patch coverage is 68.42105% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 58.30%. Comparing base (57f8398) to head (f19652f).
Report is 271 commits behind head on trunk.

❗ Current head f19652f differs from pull request most recent head d6f16e7. Consider uploading reports for the commit d6f16e7 to get more accurate results

Files	Patch %	Lines
py/selenium/webdriver/remote/client_config.py	41.81%	32 Missing ⚠️
py/selenium/webdriver/remote/remote_connection.py	82.14%	5 Missing ⚠️
py/selenium/webdriver/remote/webdriver.py	54.54%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #13286      +/-   ##
==========================================
- Coverage   58.48%   58.30%   -0.18%     
==========================================
  Files          86       89       +3     
  Lines        5270     5430     +160     
  Branches      220      224       +4     
==========================================
+ Hits         3082     3166      +84     
- Misses       1968     2040      +72     
- Partials      220      224       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

titusfortner force-pushed the py_client_config_new branch 2 times, most recently from 7696a64 to 923208f Compare

December 11, 2023 22:19

Member Author

titusfortner commented Dec 11, 2023

I had a circular dependency so I removed all references to Proxy from RemoteConnection and am calculating getProxyUrl() in ClientConfig(), which, I'm not sure is right, but I think it may fix the problem...


          [py] implement configurable configuration class for the http client

a57423a

titusfortner force-pushed the py_client_config_new branch from 923208f to a57423a Compare

December 11, 2023 22:52


          fix private method access in tests

3bc1889

titusfortner mentioned this pull request

[🚀 Feature]: Configurable HTTP Client settings across bindings #12368

Open

titusfortner mentioned this pull request

[🐛 Bug]: [Python] WebDriver constructor must not assume that BaseOptions instance has _ignore_local_proxy property #10755

Closed

mykola-mokhnach reviewed

View reviewed changes

py/selenium/webdriver/remote/client_config.py

+                  def proxy(self) -> Proxy:
+                      """:Returns: The proxy used for communicating to the driver/server."""
+                      self._proxy = self._proxy or Proxy(raw={"proxyType": ProxyType.SYSTEM})

mykola-mokhnach Dec 22, 2023

this looks like an unexpected side effect to me. Would it make sense to set the default value in the constructor?

mykola-mokhnach reviewed

View reviewed changes

py/selenium/webdriver/remote/client_config.py

+                      elif self.proxy.proxy_type == ProxyType.SYSTEM:
+                          _no_proxy = os.environ.get("no_proxy", os.environ.get("NO_PROXY"))
+                          if _no_proxy:
+                              for npu in _no_proxy.split(","):

mykola-mokhnach Dec 22, 2023

can we have better variable names? I have no idea what npu means :)

mykola-mokhnach reviewed

View reviewed changes

py/selenium/webdriver/remote/client_config.py

+                                      if n_url.path in remote_add.netloc:
+                                          return None
+                          if self.remote_server_addr.startswith("https://"):
+                              return os.environ.get("https_proxy", os.environ.get("HTTPS_PROXY"))

mykola-mokhnach Dec 22, 2023

I believe this type of call might be extracted to a separate method, so there is less unnecessary code duplication

mykola-mokhnach reviewed

View reviewed changes

py/selenium/webdriver/remote/client_config.py

+                      """
+                      self._proxy = proxy
+                  def get_proxy_url(self):

mykola-mokhnach Dec 22, 2023

can we define the type of the returned value?

mykola-mokhnach reviewed

View reviewed changes

py/selenium/webdriver/remote/client_config.py

+                      self,
+                      remote_server_addr: str,
+                      keep_alive: bool = True,
+                      proxy=None,

mykola-mokhnach Dec 22, 2023

the argument type is missing. also the spacing does not look correct


          Merge branch 'trunk' into py_client_config_new

d6f16e7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment