refactor(tests): move return code check for ssh into ssh.run #4943
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4943 +/- ##
=======================================
Coverage 84.07% 84.07%
=======================================
Files 251 251
Lines 28059 28059
=======================================
Hits 23592 23592
Misses 4467 4467
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I looked at the same thing back when I introduced check_output, and there are quite a few locations where we do not want to fail on a non-zero return code (e.g. we run some command inside the guest that we expect to return with non-zero code). So I don't think we can blanket do assert rc != 0.
However, I think we can do a blanket assert rc != 255 inside ssh_connection.run, because ssh specifies that a 255 return code indicates something going wrong with the connection.
As for converting more .runs to .check_output, and dropping the rc return value from check_output: yes please
ShadowCurse
force-pushed
the
ssh_assert
branch
from
f38eae4 to
42051ba
Compare
|
My idea here is that if anything goes wrong with ssh connection "by default" we fail the test and it does not matter if it is an ssh error or a command error. In cases where we want to check the output manually (like in the |
ShadowCurse
force-pushed
the
ssh_assert
branch
3 times, most recently
from
ed45e17 to
17e287f
Compare
|
Ah, we've kept the APIs as run/check_output to be consistent with the subprocess APIs. If we change this, then we should also change |
ShadowCurse
force-pushed
the
ssh_assert
branch
4 times, most recently
from
0cc5e8c to
ae951cc
Compare
Ssh connection failure should be considered an error by default. In order to unify handling of this case, move check for non zero return code into the function itself. Because of this `check_output` method is not needed anymore and was removed. Signed-off-by: Egor Lazarchuk <[email protected]>
ShadowCurse
force-pushed
the
ssh_assert
branch
from
ae951cc to
ced8c02
Compare
| def check_output(self, cmd_string, timeout=None, *, debug=False): | ||
| """Same as `run`, but raises an exception on non-zero return code of remote command""" | ||
| return self.run(cmd_string, timeout, check=True, debug=debug) |
There was a problem hiding this comment.
We modeled check_output/run on subprocess.check_output/run. I think we should keep them as they are, as they are both useful. Most cases can be handled by check_output, but run is useful if the caller wants to handle the error.
Why not move more run calls to explicitly use check_output instead?
There was a problem hiding this comment.
Wouldn't it be easier if there was only 1 method to call? By default run will check output and if we need some exception, we will explicitly set check=False.
I did not touch supbprocess.run yet here, but was going to give it the same treatment.
There was a problem hiding this comment.
I agree with Pablo. Ideally out run/check_output would behave exactly like subprocess.run/check_output.
There was a problem hiding this comment.
Well, to me it's two different use cases, and how I see the subprocess API used:
- If I want to run a command, I use
check_outputbecause it's convenient and I don't have to mess with parameters. - If I want fine control over what the command does, I use
run.
subprocess.run has check=False. If we change that here to check=True, it'd be the opposite and surprising for anybody who is used to subprocess
Changes
Move return code check for ssh into ssh.run.
Reason
Ssh connection should be considered an error by default. In order to unify handling of this case, move check for non zero return code into the function itself.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.