Total refactoring of os_ops::execute_command (#203)

* Total refactoring of os_ops::execute_command

Main
 - We check only an exit code to detect an error.
 - If someone utility returns a result through an exit code, a caller side should set ignore_errors=true and process this case itself.
 - If expect_error is true and no errors occurred, we raise an InvalidOperationException.

* The old behaviour of RaiseError.UtilityExitedWithNonZeroCode is restored

Let's rollback the new code to avoid problems with probackup2' tests.

Author: dmitry-lipetsk

testgres/operations/local_ops.py

@@ -23,20 +23,6 @@
     from distutils import rmtree
 
 CMD_TIMEOUT_SEC = 60
-error_markers = [b'error', b'Permission denied', b'fatal']
-err_out_markers = [b'Failure']
-
-
-def has_errors(output=None, error=None):
-    if output:
-        if isinstance(output, str):
-            output = output.encode(get_default_encoding())
-        return any(marker in output for marker in err_out_markers)
-    if error:
-        if isinstance(error, str):
-            error = error.encode(get_default_encoding())
-        return any(marker in error for marker in error_markers)
-    return False
 
 
 class LocalOperations(OsOperations):
@@ -134,19 +120,29 @@ def exec_command(self, cmd, wait_exit=False, verbose=False, expect_error=False,
         process, output, error = self._run_command(cmd, shell, input, stdin, stdout, stderr, get_process, timeout, encoding)
         if get_process:
             return process
-        if not ignore_errors and ((process.returncode != 0 or has_errors(output=output, error=error)) and not expect_error):
+
+        if expect_error:
+            if process.returncode == 0:
+                raise InvalidOperationException("We expected an execution error.")
+        elif ignore_errors:
+            pass
+        elif process.returncode == 0:
+            pass
+        else:
+            assert not expect_error
+            assert not ignore_errors
+            assert process.returncode != 0
             RaiseError.UtilityExitedWithNonZeroCode(
                 cmd=cmd,
                 exit_code=process.returncode,
                 msg_arg=error or output,
                 error=error,
-                out=output
-            )
+                out=output)
 
         if verbose:
             return process.returncode, output, error
-        else:
-            return output
+
+        return output
 
     # Environment setup
     def environ(self, var_name):

testgres/operations/raise_error.py

@@ -7,13 +7,27 @@ class RaiseError:
     def UtilityExitedWithNonZeroCode(cmd, exit_code, msg_arg, error, out):
         assert type(exit_code) == int  # noqa: E721
 
-        msg_arg_s = __class__._TranslateDataIntoString(msg_arg).strip()
+        msg_arg_s = __class__._TranslateDataIntoString(msg_arg)
         assert type(msg_arg_s) == str  # noqa: E721
 
+        msg_arg_s = msg_arg_s.strip()
         if msg_arg_s == "":
             msg_arg_s = "#no_error_message"
 
-        message = "Utility exited with non-zero code. Error: `" + msg_arg_s + "`"
+        message = "Utility exited with non-zero code (" + str(exit_code) + "). Error: `" + msg_arg_s + "`"
+        raise ExecUtilException(
+            message=message,
+            command=cmd,
+            exit_code=exit_code,
+            out=out,
+            error=error)
+
+    @staticmethod
+    def CommandExecutionError(cmd, exit_code, message, error, out):
+        assert type(exit_code) == int  # noqa: E721
+        assert type(message) == str  # noqa: E721
+        assert message != ""
+
         raise ExecUtilException(
             message=message,
             command=cmd,
@@ -23,6 +37,9 @@ def UtilityExitedWithNonZeroCode(cmd, exit_code, msg_arg, error, out):
 
     @staticmethod
     def _TranslateDataIntoString(data):
+        if data is None:
+            return ""
+
         if type(data) == bytes:  # noqa: E721
             return __class__._TranslateDataIntoString__FromBinary(data)
 
@@ -38,13 +55,3 @@ def _TranslateDataIntoString__FromBinary(data):
             pass
 
         return "#cannot_decode_text"
-
-    @staticmethod
-    def _BinaryIsASCII(data):
-        assert type(data) == bytes  # noqa: E721
-
-        for b in data:
-            if not (b >= 0 and b <= 127):
-                return False
-
-        return True

testgres/operations/remote_ops.py

@@ -100,41 +100,40 @@ def exec_command(self, cmd, wait_exit=False, verbose=False, expect_error=False,
             return process
 
         try:
-            result, error = process.communicate(input=input_prepared, timeout=timeout)
+            output, error = process.communicate(input=input_prepared, timeout=timeout)
         except subprocess.TimeoutExpired:
             process.kill()
             raise ExecUtilException("Command timed out after {} seconds.".format(timeout))
 
-        exit_status = process.returncode
-
-        assert type(result) == bytes  # noqa: E721
+        assert type(output) == bytes  # noqa: E721
         assert type(error) == bytes  # noqa: E721
 
-        if not error:
-            error_found = False
-        else:
-            error_found = exit_status != 0 or any(
-                marker in error for marker in [b'error', b'Permission denied', b'fatal', b'No such file or directory']
-            )
-
-        assert type(error_found) == bool  # noqa: E721
-
         if encoding:
-            result = result.decode(encoding)
+            output = output.decode(encoding)
             error = error.decode(encoding)
 
-        if not ignore_errors and error_found and not expect_error:
+        if expect_error:
+            if process.returncode == 0:
+                raise InvalidOperationException("We expected an execution error.")
+        elif ignore_errors:
+            pass
+        elif process.returncode == 0:
+            pass
+        else:
+            assert not expect_error
+            assert not ignore_errors
+            assert process.returncode != 0
             RaiseError.UtilityExitedWithNonZeroCode(
                 cmd=cmd,
-                exit_code=exit_status,
+                exit_code=process.returncode,
                 msg_arg=error,
                 error=error,
-                out=result)
+                out=output)
 
         if verbose:
-            return exit_status, result, error
-        else:
-            return result
+            return process.returncode, output, error
+
+        return output
 
     # Environment setup
     def environ(self, var_name: str) -> str:
@@ -165,8 +164,30 @@ def find_executable(self, executable):
 
     def is_executable(self, file):
         # Check if the file is executable
-        is_exec = self.exec_command("test -x {} && echo OK".format(file))
-        return is_exec == b"OK\n"
+        command = ["test", "-x", file]
+
+        exit_status, output, error = self.exec_command(cmd=command, encoding=get_default_encoding(), ignore_errors=True, verbose=True)
+
+        assert type(output) == str  # noqa: E721
+        assert type(error) == str  # noqa: E721
+
+        if exit_status == 0:
+            return True
+
+        if exit_status == 1:
+            return False
+
+        errMsg = "Test operation returns an unknown result code: {0}. File name is [{1}].".format(
+            exit_status,
+            file)
+
+        RaiseError.CommandExecutionError(
+            cmd=command,
+            exit_code=exit_status,
+            msg_arg=errMsg,
+            error=error,
+            out=output
+        )
 
     def set_env(self, var_name: str, var_val: str):
         """
@@ -251,15 +272,21 @@ def mkdtemp(self, prefix=None):
         else:
             command = ["mktemp", "-d"]
 
-        exit_status, result, error = self.exec_command(command, verbose=True, encoding=get_default_encoding(), ignore_errors=True)
+        exec_exitcode, exec_output, exec_error = self.exec_command(command, verbose=True, encoding=get_default_encoding(), ignore_errors=True)
 
-        assert type(result) == str  # noqa: E721
-        assert type(error) == str  # noqa: E721
+        assert type(exec_exitcode) == int  # noqa: E721
+        assert type(exec_output) == str  # noqa: E721
+        assert type(exec_error) == str  # noqa: E721
 
-        if exit_status != 0:
-            raise ExecUtilException("Could not create temporary directory. Error code: {0}. Error message: {1}".format(exit_status, error))
+        if exec_exitcode != 0:
+            RaiseError.CommandExecutionError(
+                cmd=command,
+                exit_code=exec_exitcode,
+                message="Could not create temporary directory.",
+                error=exec_error,
+                out=exec_output)
 
-        temp_dir = result.strip()
+        temp_dir = exec_output.strip()
         return temp_dir
 
     def mkstemp(self, prefix=None):
@@ -273,15 +300,21 @@ def mkstemp(self, prefix=None):
         else:
             command = ["mktemp"]
 
-        exit_status, result, error = self.exec_command(command, verbose=True, encoding=get_default_encoding(), ignore_errors=True)
+        exec_exitcode, exec_output, exec_error = self.exec_command(command, verbose=True, encoding=get_default_encoding(), ignore_errors=True)
 
-        assert type(result) == str  # noqa: E721
-        assert type(error) == str  # noqa: E721
+        assert type(exec_exitcode) == int  # noqa: E721
+        assert type(exec_output) == str  # noqa: E721
+        assert type(exec_error) == str  # noqa: E721
 
-        if exit_status != 0:
-            raise ExecUtilException("Could not create temporary file. Error code: {0}. Error message: {1}".format(exit_status, error))
+        if exec_exitcode != 0:
+            RaiseError.CommandExecutionError(
+                cmd=command,
+                exit_code=exec_exitcode,
+                message="Could not create temporary file.",
+                error=exec_error,
+                out=exec_output)
 
-        temp_file = result.strip()
+        temp_file = exec_output.strip()
         return temp_file
 
     def copytree(self, src, dst):

testgres/utils.py

@@ -18,6 +18,7 @@
 from .config import testgres_config as tconf
 from .operations.os_ops import OsOperations
 from .operations.remote_ops import RemoteOperations
+from .operations.helpers import Helpers as OsHelpers
 
 # rows returned by PG_CONFIG
 _pg_config_data = {}
@@ -79,13 +80,13 @@ def execute_utility2(os_ops: OsOperations, args, logfile=None, verbose=False, ig
     assert type(verbose) == bool  # noqa: E721
     assert type(ignore_errors) == bool  # noqa: E721
 
-    exit_status, out, error = os_ops.exec_command(args, verbose=True, ignore_errors=ignore_errors)
-    # decode result
+    exit_status, out, error = os_ops.exec_command(
+        args,
+        verbose=True,
+        ignore_errors=ignore_errors,
+        encoding=OsHelpers.GetDefaultEncoding())
+
     out = '' if not out else out
-    if isinstance(out, bytes):
-        out = out.decode('utf-8')
-    if isinstance(error, bytes):
-        error = error.decode('utf-8')
 
     # write new log entry if possible
     if logfile:

tests/test_local.py

@@ -40,10 +40,11 @@ def test_exec_command_failure(self):
             try:
                 self.operations.exec_command(cmd, wait_exit=True, shell=True)
             except ExecUtilException as e:
-                error = e.message
+                assert e.message == "Utility exited with non-zero code (127). Error: `/bin/sh: 1: nonexistent_command: not found`"
+                assert type(e.error) == bytes  # noqa: E721
+                assert e.error.strip() == b"/bin/sh: 1: nonexistent_command: not found"
                 break
             raise Exception("We wait an exception!")
-        assert error == "Utility exited with non-zero code. Error: `/bin/sh: 1: nonexistent_command: not found`"
 
     def test_exec_command_failure__expect_error(self):
         """

tests/test_remote.py

@@ -40,10 +40,11 @@ def test_exec_command_failure(self):
             try:
                 self.operations.exec_command(cmd, verbose=True, wait_exit=True)
             except ExecUtilException as e:
-                error = e.message
+                assert e.message == "Utility exited with non-zero code (127). Error: `bash: line 1: nonexistent_command: command not found`"
+                assert type(e.error) == bytes  # noqa: E721
+                assert e.error.strip() == b"bash: line 1: nonexistent_command: command not found"
                 break
             raise Exception("We wait an exception!")
-        assert error == 'Utility exited with non-zero code. Error: `bash: line 1: nonexistent_command: command not found`'
 
     def test_exec_command_failure__expect_error(self):
         """
@@ -114,10 +115,11 @@ def test_makedirs_and_rmdirs_failure(self):
             try:
                 self.operations.rmdirs(path, verbose=True)
             except ExecUtilException as e:
-                error = e.message
+                assert e.message == "Utility exited with non-zero code (1). Error: `rm: cannot remove '/root/test_dir': Permission denied`"
+                assert type(e.error) == bytes  # noqa: E721
+                assert e.error.strip() == b"rm: cannot remove '/root/test_dir': Permission denied"
                 break
             raise Exception("We wait an exception!")
-        assert error == "Utility exited with non-zero code. Error: `rm: cannot remove '/root/test_dir': Permission denied`"
 
     def test_listdir(self):
         """

tests/test_simple_remote.py

@@ -178,27 +178,32 @@ def test_init__unk_LANG_and_LC_CTYPE(self):
                 assert os.environ.get("LC_CTYPE") == unkData[1]
                 assert not ("LC_COLLATE" in os.environ.keys())
 
-                while True:
+                assert os.getenv('LANG') == unkData[0]
+                assert os.getenv('LANGUAGE') is None
+                assert os.getenv('LC_CTYPE') == unkData[1]
+                assert os.getenv('LC_COLLATE') is None
+
+                exc: ExecUtilException = None
+                with __class__.helper__get_node() as node:
                     try:
-                        with __class__.helper__get_node():
-                            pass
-                    except ExecUtilException as e:
-                        #
-                        # Example of an error message:
-                        #
-                        # warning: setlocale: LC_CTYPE: cannot change locale (UNKNOWN_CTYPE): No such file or directory
-                        # postgres (PostgreSQL) 14.12
-                        #
-                        errMsg = str(e)
-
-                        logging.info("Error message is: {0}".format(errMsg))
-
-                        assert "LC_CTYPE" in errMsg
-                        assert unkData[1] in errMsg
-                        assert "warning: setlocale: LC_CTYPE: cannot change locale (" + unkData[1] + "): No such file or directory" in errMsg
-                        assert ("postgres" in errMsg) or ("PostgreSQL" in errMsg)
-                        break
+                        node.init()  # IT RAISES!
+                    except InitNodeException as e:
+                        exc = e.__cause__
+                        assert exc is not None
+                        assert isinstance(exc, ExecUtilException)
+
+                if exc is None:
                     raise Exception("We expected an error!")
+
+                assert isinstance(exc, ExecUtilException)
+
+                errMsg = str(exc)
+                logging.info("Error message is {0}: {1}".format(type(exc).__name__, errMsg))
+
+                assert "warning: setlocale: LC_CTYPE: cannot change locale (" + unkData[1] + ")" in errMsg
+                assert "initdb: error: invalid locale settings; check LANG and LC_* environment variables" in errMsg
+                continue
+
         finally:
             __class__.helper__restore_envvar("LANG", prev_LANG)
             __class__.helper__restore_envvar("LANGUAGE", prev_LANGUAGE)