Re: [PATCH 4/4] dts: log stderr with failed remote commands

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Mon, Jan 22, 2024 at 7:26 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add the executed command stderr to RemoteCommandExecutionError. So that
> it is logged as an error, instead of just as debug.

Here's I'd add logged additionally as an error, as this sounds as if
we're changing debug to error.

>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> ---
>  dts/framework/exception.py                     | 10 +++++++---
>  dts/framework/remote_session/remote_session.py |  2 +-
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/dts/framework/exception.py b/dts/framework/exception.py
> index 658eee2c38..9fc3fa096a 100644
> --- a/dts/framework/exception.py
> +++ b/dts/framework/exception.py
> @@ -130,20 +130,24 @@ class RemoteCommandExecutionError(DTSError):
>      #: The executed command.
>      command: str
>      _command_return_code: int
> +    _command_stderr: str
>

I'd change the order here (and all other places) so that stderr is
before the return code.

> -    def __init__(self, command: str, command_return_code: int):
> +    def __init__(self, command: str, command_return_code: int, command_stderr: str):
>          """Define the meaning of the first two arguments.
>
>          Args:
>              command: The executed command.
>              command_return_code: The return code of the executed command.
> +            command_stderr: The stderr of the executed command.
>          """
>          self.command = command
>          self._command_return_code = command_return_code
> +        self._command_stderr = command_stderr
>
>      def __str__(self) -> str:
> -        """Include both the command and return code in the string representation."""
> -        return f"Command {self.command} returned a non-zero exit code: {self._command_return_code}"
> +        """Include the command, its return code and stderr in the string representation."""
> +        return (f"Command '{self.command}' returned a non-zero exit code: "
> +                f"{self._command_return_code}\n{self._command_stderr}")

We should mention that the last string is the stderr output. Maybe we
just add 'Stderr:' before {self._command_stderr}. And maybe we should
put quotes around {self._command_stderr}.

>
>
>  class RemoteDirectoryExistsError(DTSError):
> diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
> index 2059f9a981..345439f2de 100644
> --- a/dts/framework/remote_session/remote_session.py
> +++ b/dts/framework/remote_session/remote_session.py
> @@ -157,7 +157,7 @@ def send_command(
>              )
>              self._logger.debug(f"stdout: '{result.stdout}'")
>              self._logger.debug(f"stderr: '{result.stderr}'")
> -            raise RemoteCommandExecutionError(command, result.return_code)
> +            raise RemoteCommandExecutionError(command, result.return_code, result.stderr)
>          self._logger.debug(f"Received from '{command}':\n{result}")
>          self.history.append(result)
>          return result
> --
> 2.34.1
>