Removing escape sequence from output in cmd_output function!

[Anushree-Mathur]

In console outputs we are getting escape sequences because of which most of the testcases are failing to use the output of few commands. This PR removes the escape sequence first then use the output.This PR fixes multiple testcases.
Signed-off-by: Anushree Mathur [email protected]

[Anushree-Mathur]

In total 2000+ testcases are failing but to give an example I am mentioning the following testcase!
Before applying the patch:

[stdout] escape output cat /proc/uptime
[stdout] ^[[?2004l168.18 720.90
[stdout] ^[[?2004h[root@localhost ~]#

 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: STARTED
 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: PASS (232.38 s)
 (2/3) type_specific.io-github-autotest-libvirt.virsh.migrate.there_and_back_with_numa.with_numa_only.with_mem_hotplug.with_cpu_hotplug.with_hotplug.hotplug_after_migration.compat_migration.non_compat_migration.with_postcopy: STARTED
 (2/3) type_specific.io-github-autotest-libvirt.virsh.migrate.there_and_back_with_numa.with_numa_only.with_mem_hotplug.with_cpu_hotplug.with_hotplug.hotplug_after_migration.compat_migration.non_compat_migration.with_postcopy: ERROR: could not convert string to float: '\x1b[?2004l168.18' (443.02 s)
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: STARTED
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: PASS (5.08 s)

After applying the patch:

[stdout] output cat /proc/uptime
[stdout] 168.18 720.90
[stdout] [root@localhost ~]# 

 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: STARTED
 (1/3) io-github-autotest-qemu.unattended_install.import.import.default_install.aio_native: PASS (236.23 s)
 (2/3) type_specific.io-github-autotest-libvirt.virsh.migrate.there_and_back_with_numa.with_numa_only.with_mem_hotplug.with_cpu_hotplug.with_hotplug.hotplug_after_migration.compat_migration.non_compat_migration.with_postcopy: STARTED
 (2/3) type_specific.io-github-autotest-libvirt.virsh.migrate.there_and_back_with_numa.with_numa_only.with_mem_hotplug.with_cpu_hotplug.with_hotplug.hotplug_after_migration.compat_migration.non_compat_migration.with_postcopy: PASS (614.98 s)
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: STARTED
 (3/3) io-github-autotest-libvirt.remove_guest.without_disk: PASS (5.07 s)

[ldoktor]

I don't think the strip_console_codes should be used at all times as get_output might be used to interact with any kind of output. IMO the consumers should decide whether they need to strip the console codes or leave them present in the output.

It's true that people interacting with odd outputs might use read_nonblocking directly to get the full output, but having get_output seems convenient and some only need to check the output after the execution (eg. tests needing to check whether arrows were used correctly).

I mean if there were thousands of lines where people need to clean the output, I'd consider adding an argument to do so, but doing it out of the box seems like removing useful feature for lower-level usage.

@smitterl, @pevogam would you have any opinions?

[Anushree-Mathur]

Hello @ldoktor , thanks alot for taking time and reviewing this commit! I understand your point, so do you suggest that we do it locally in the functions where these outputs are being used? Could you please guide me how i can proceed otherwise?

Thanks in advance!

[ldoktor]

Hello @ldoktor , thanks alot for taking time and reviewing this commit! I understand your point, so do you suggest that we do it locally in the functions where these outputs are being used? Could you please guide me how i can proceed otherwise?

Thanks in advance!

What I meant is to do it explicitly in the test itself:

from aexpect.utils.astring import strip_console_codes

...

out = session.cmd_output(...)
out_safe = strip_console_codes(out)

But if it proves to be massively used we might consider support for session.cmd_output(..., strip_console_codes=True) which would allow doing this inside the cmd_output.

[Anushree-Mathur]

Hello @ldoktor , understood your point clearly! Thank you so much. Will make the changes in the testcase itself.

[pevogam]

Hi, I will be able to test and review this towards the end of the week, I hope this is ok and thanks for the contribution!

[Anushree-Mathur]

Hi @ldoktor @pevogam , I have tried modifying my PR in the following way as suggested by @ldoktor! Many places cmd_output is used so I changed the code here and tried to run the same tescase after applying the strip_console_code=True in the local code where I wanted the O/P to be replaced and it worked totally fine! I would really like to have some suggestions here if I can proceed in this manner! Thanks in advance for your time.

    def cmd_output(self, cmd, timeout=60, internal_timeout=None,
                   print_func=None, safe=False, strip_console_codes=False):
        """
        Send a command and return its output.

        :param cmd: Command to send (must not contain newline characters)
        :param timeout: The duration (in seconds) to wait for the prompt to
                return
        :param internal_timeout: The timeout to pass to read_nonblocking
        :param print_func: A function to be used to print the data being read
                (should take a string parameter)
        :param safe: Whether using safe mode when execute cmd.
                In serial sessions, frequently the kernel might print debug or
                error messages that make read_up_to_prompt to timeout. Let's
                try to be a little more robust and send a carriage return, to
                see if we can get to the prompt when safe=True.

        :return: The output of cmd
        :raise ShellTimeoutError: Raised if timeout expires
        :raise ShellProcessTerminatedError: Raised if the shell process
                terminates while waiting for output
        :raise ShellError: Raised if an unknown error occurs
        """
        if safe:
            return self.cmd_output_safe(cmd, timeout)
        session_tag = f"[{self.output_prefix}] " if self.output_prefix else ""
        LOG.debug("%sSending command: %s", session_tag, cmd)
        self.read_nonblocking(0, timeout)
        self.sendline(cmd)
        try:
            out = self.read_up_to_prompt(timeout, internal_timeout, print_func)
        except ExpectTimeoutError as error:
            output = self.remove_command_echo(error.output, cmd)
            raise ShellTimeoutError(cmd, output) from error
        except ExpectProcessTerminatedError as error:
            output = self.remove_command_echo(error.output, cmd)
            raise ShellProcessTerminatedError(cmd, error.status, output) from error
        except ExpectError as error:
            output = self.remove_command_echo(error.output, cmd)
            raise ShellError(cmd, output) from error
        # Remove the echoed command and the final shell prompt
        if strip_console_codes:
            return self.remove_last_nonempty_line(self.remove_command_echo(astring.strip_console_codes(out),
                                                                       cmd))
        else:
            return self.remove_last_nonempty_line(self.remove_command_echo(out,
                                                                       cmd))

[pevogam]

Hi @Anushree-Mathur, could you perhaps provide a diff instead so that it is easy to parse what modification entails?

[Anushree-Mathur]

Hi @pevogam , here is the git diff result:

diff --git a/aexpect/client.py b/aexpect/client.py
index e0460f4..6fa9f1e 100644
--- a/aexpect/client.py
+++ b/aexpect/client.py
@@ -1188,7 +1188,7 @@ class ShellSession(Expect):
                                                  print_func)[1]
 
     def cmd_output(self, cmd, timeout=60, internal_timeout=None,
-                   print_func=None, safe=False):
+                   print_func=None, safe=False, strip_console_codes=False):
         """
         Send a command and return its output.
 
@@ -1227,9 +1227,12 @@ class ShellSession(Expect):
         except ExpectError as error:
             output = self.remove_command_echo(error.output, cmd)
             raise ShellError(cmd, output) from error
-
         # Remove the echoed command and the final shell prompt
-        return self.remove_last_nonempty_line(self.remove_command_echo(out,
+        if strip_console_codes:
+            return self.remove_last_nonempty_line(self.remove_command_echo(astring.strip_console_codes(out),
+                                                                       cmd))
+        else:
+            return self.remove_last_nonempty_line(self.remove_command_echo(out,
                                                                        cmd))
 
     def cmd_output_safe(self, cmd, timeout=60):

[ldoktor]

This seems fine by me, provided the code is polished a bit. Mainly it needs to tackle the cmd_output_safe as well as the normal cmd_output and I'd prefer having just a single return statement with the out variable being modified before the return if asked for. @pevogam does this new argument sounds fine to you?

[pevogam]

I don't think the strip_console_codes should be used at all times as get_output might be used to interact with any kind of output. IMO the consumers should decide whether they need to strip the console codes or leave them present in the output.

It's true that people interacting with odd outputs might use read_nonblocking directly to get the full output, but having get_output seems convenient and some only need to check the output after the execution (eg. tests needing to check whether arrows were used correctly).

I mean if there were thousands of lines where people need to clean the output, I'd consider adding an argument to do so, but doing it out of the box seems like removing useful feature for lower-level usage.

I second that opinion. This change is invasive and should preserve the behavior for anyone not wishing to remove the console codes and likely has their own handling logic down the line.

Hi @pevogam , here is the git diff result:

I think this diff is much more reasonable and perhaps you could push the suggested change for easier review now?

This seems fine by me, provided the code is polished a bit. Mainly it needs to tackle the cmd_output_safe as well as the normal cmd_output and I'd prefer having just a single return statement with the out variable being modified before the return if asked for. @pevogam does this new argument sounds fine to you?

I can definitely do in-line commenting and recommendations once I see this pushed but @Anushree-Mathur it would be great if you take in @ldoktor's recommendation before the push so we can save on the feedback cycles.

Also, I wonder what is the difference between this PR and #141? Could we instead close one of them and focus on just one pull request regarding escaping such characters?

[ldoktor]

Well the #141 is solely about the last_line_matches, which is mainly used to detect console. It's safer to use filtering there as the whole output is returned while the filtered one is only used for the detection.

Note I'd classify the #141 actually as a bugfix as I think we ought to treat the output that way and I'm surprised we're not hitting problems with it too frequently. As for this one it's a convenience for test-writers who could use this feature to get filtered output and I like @Anushree-Mathur kept the default behaviour as is so unless one deliberately opts-in for it they get the same behaviour.

[pevogam]

Well the #141 is solely about the last_line_matches, which is mainly used to detect console. It's safer to use filtering there as the whole output is returned while the filtered one is only used for the detection.

Note I'd classify the #141 actually as a bugfix as I think we ought to treat the output that way and I'm surprised we're not hitting problems with it too frequently. As for this one it's a convenience for test-writers who could use this feature to get filtered output and I like @Anushree-Mathur kept the default behaviour as is so unless one deliberately opts-in for it they get the same behaviour.

Even then the changes seem quite related and could have at least be two commits in the same branch like "fix critical problem" followed by "enhance use for others for related problem" but your arguments also make a lot of sense.

Alright, in any case still good to see the suggested changes pushed so that we can test them.