pytester: slightly clean up LsofFdLeakChecker #7436
Conversation
| def matching_platform(self): | ||
| def matching_platform(self) -> bool: | ||
| try: | ||
| subprocess.check_output(("lsof", "-v")) |
There was a problem hiding this comment.
just out of curiosity, why do you prefer run to check_output here?
There was a problem hiding this comment.
There's no good reason, only that the subprocess docs start with describing only the run function and say
The run() function was added in Python 3.5; if you need to retain compatibility with older versions, see the Older high-level API section.
And that "Older high-level API" includes check_output. So I just use run for everything.
There was a problem hiding this comment.
thanks for explaining @bluetech
|
|
||
| def _parse_lsof_output(self, out): | ||
| def isopen(line): | ||
| def get_open_files(self) -> List[Tuple[str, str]]: |
There was a problem hiding this comment.
why do we not handle any errors? Just curious since the original method doesn't seem to have done it either. But, it looks like it is possible for subprocess.run to throw at least CalledProcessError and TimeoutExpired.
There was a problem hiding this comment.
Good point! I think it would be good to handle errors from the lsof call here.
If the lsof command doesn't exist, the LsofFdLeakChecker plugin isn't registered at all, so the issue can't be that lsof isn't found at all, but it can still return an error itself.
I don't think TimeoutExpired can happen because we don't pass timeout to subprocess.run, but CalledProcessError can happen.
Then there's a question what to do if an error occurs. I guess in this case the most reasonable thing is to just skip the check for the particular test, and issue a warning.
|
Thanks for reviewing @gnikonorov! |
2 participants
Just a little leftover from trying to hunt down
ResourceWarningsa few weeks ago.No semantic changes intended.