Fix spellcheck and missing timeout in sshdriver #286
Conversation
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
======================================
Coverage 54.9% 54.9%
======================================
Files 106 106
Lines 6294 6294
======================================
Hits 3459 3459
Misses 2835 2835
Continue to review full report at Codecov.
|
| @@ -101,9 +101,9 @@ def _check_master(self): | |||
| @Driver.check_active | |||
| @step(args=['cmd'], result=True) | |||
| def run(self, cmd, codec="utf-8", decodeerrors="strict", timeout=None): # pylint: disable=unused-argument | |||
| return self._run(cmd, codec=codec, decodererrors=decodeerrors) | |||
| return self._run(cmd, codec=codec, decodeerrors=decodeerrors) | |||
There was a problem hiding this comment.
Shouldn't we pass on the timeout value to _run() here?
There was a problem hiding this comment.
Timeout is actually ignored. commandmixin just happen to pass it to sshdriver.
The new is that commandmixin now calls _run instead of run, so actually it could be removed here, but that would not be backwards compatible.
|
|
||
| def _run(self, cmd, codec, decodeerrors): # pylint: disable=unused-argument | ||
| def _run(self, cmd, codec="utf-8", decodeerrors="strict", timeout=None): # pylint: disable=unused-argument |
There was a problem hiding this comment.
If timeout value is passed on from run() to _run(), I don't see why any default values are needed here.
| @@ -34,14 +34,14 @@ def _run_check(self, cmd: str, timeout=30): | |||
| Returns: | |||
| List[str]: stdout of the executed command | |||
| """ | |||
| stdout, stderr, exitcode = self._run(cmd, timeout=timeout) | |||
| stdout, stderr, exitcode = self._run(cmd, timeout=timeout, codec=codec, decodeerrors=decodeerrors) | |||
There was a problem hiding this comment.
Where are the variables codec and decodeerrors supposed to come from? I guess you missed updating
def _run_check(self, cmd: str, timeout=30):
with these arguments, right?
The problem is not every _run() methods knows about codec and decodeerrors, so this breaks everywhere else.
There was a problem hiding this comment.
You should either transform the _run() method of BareboxDriver, UBootDriver and SmallUBootDriver or go without CommandMixin's convenience function run_check on targets with decode errors.
There was a problem hiding this comment.
You're right. I hope that these run functions will be stable soon and not have more special options.
I will add these but as unused parameters, because I don't see any use for codec's in these drivers.
Better test functions would help a lot. In my PR for the remote_tmpdir I actually have a test case which covers the SSH driver better.
|
I fixed this while already on it:
|
|
Thanks @BastianStender |
|
Squashed the fixup. @jluebbe could you review/merge this? |
Signed-off-by: Kjeld Flarup <kfa@deif.com>
Having None as default leads to user@hostname:None This does not make sense, make the default an empty string instead. Signed-off-by: Bastian Stender <bst@pengutronix.de>
Signed-off-by: Kjeld Flarup <kfa@deif.com>
The "FIXME: Handle pexpect Timeout" is outdated and is now removed. Signed-off-by: Bastian Stender <bst@pengutronix.de>
…d honor timeout Signed-off-by: Bastian Stender <bst@pengutronix.de>
…or timeout Signed-off-by: Bastian Stender <bst@pengutronix.de>
Fix errors introduced with commit 1f89ed9
Signed-off-by: Kjeld Flarup kfa@deif.com