-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] improve driver logging #12103
[py] improve driver logging #12103
Conversation
Codecov ReportPatch coverage:
❗ 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 #12103 +/- ##
==========================================
+ Coverage 56.56% 56.89% +0.32%
==========================================
Files 86 86
Lines 5450 5494 +44
Branches 223 223
==========================================
+ Hits 3083 3126 +43
- Misses 2144 2145 +1
Partials 223 223
☔ View full report in Codecov by Sentry. |
3e5ddc0
to
e10df57
Compare
a31f9d4
to
f7876af
Compare
f7876af
to
7593eb1
Compare
7593eb1
to
7ef54af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @titusfortner!
Thanks for implementing this! |
if isinstance(log_output, str): | ||
self.log_output = open(log_output, "a+", encoding="utf-8") | ||
elif log_output is subprocess.STDOUT: | ||
self.log_output = None | ||
elif log_output is None or log_output is subprocess.DEVNULL: | ||
self.log_output = open(os.devnull, "wb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these file descriptors are never closed leading to ResourceWarnings when this is garbage collected (and leaked fds)
* don't leak a file descriptor to os.devnull by default since this is passed along to subprocess directly we can use the subprocess constants still regression in #12103 * adjust condition for closing as well
Description
log_file
orlog_path
Service class constructor parameters, which only accepts strings as filenameslog_output
(which matches Java'swithLogOutput()
) constructor parameter. This parameter that accepts:subprocess.STDOUT
orsubprocess.DEVNULL
)geckodriver.log
([🚀 Feature]: Default All Driver Log Output to dev/null #12016)--append-log
or--readable-timestamp
are passed (chromium) because those only work withlog-file
driver argumentMotivation and Context
Make things consistent between drivers and with Java & Ruby
One thing I'm not sure of....
This currently deprecates and warns about the change in default Firefox behavior; I think I'd prefer just changing it and maybe logging a message about the change with a link to the docs (https://www.selenium.dev/documentation/webdriver/drivers/service/#setting-log-output)
Also, I'm not sure I have the typing exactly right? I'm not sure what
typing.IO[typing.Any]
actually includes.