-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Update all subprocess
calls to use errors="backslashreplace"
#1145
Update all subprocess
calls to use errors="backslashreplace"
#1145
Conversation
383bf2b
to
4fedada
Compare
- The use of `readline()` in the output streamer can raise `ValueError` exceptions for reasons other than `stdout` unexpectedly closing. So, ensure other exceptions are properly caught and reported in the log.
So, I thought through specific tests for this functionality....and I can't think of any that wouldn't simply be testing whether If others have tests they think would be valuable, let me know and I'll add them. |
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.
All looks good to me. One minor tweak to the formatting, mostly in service of making Black easier to deal with, but otherwise good to go.
I have a minor reservation about the sub_kw
optimisation - it's a local test configuration we need to remember to use, and I don't necessarily consider verbosity a flaw in tests if the verbosity is explicit. However, I don't feel strongly enough to revert this myself (or ask you to revert it), and I can see some value in having a common definition so it's obvious and explicit when we're not using default values.
As for testing - I can't think of any obvious additional tests either. As you note, any testing of subprocess output really falls into the "testing that subprocess does what it says it does", which doesn't really improve quality.
Tbh, the consolidation was more about avoiding having to update 100 tests again in the future. I think this was the third time I've done it because something in |
I figured as much :-) You will insist on working on gnarly bits of code, though... so you have no-one to blame by yourself :-P |
Changes
subprocess
to includeerrors="backslashreplace"
to avoidUnicodeDecodeError
exceptionsValueError
exceptions to only those arising from the process unexpectedly closingstdout
.Example of effect of
errors="backslashreplace"
Issues
UnicodeDecodeError
Exceptions frompopen_process.stdout.readline()
#1141PR Checklist: