Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bracketed-paste default breaks tests #3116

Closed
smitterl opened this issue Jun 29, 2021 · 2 comments
Closed

bracketed-paste default breaks tests #3116

smitterl opened this issue Jun 29, 2021 · 2 comments

Comments

@smitterl
Copy link
Contributor

Bash 5.1/libread 8.1 seemingly have opted for enabling 'bracketed-paste' per default. (Refer for example to pexpect issue pexpect/pexpect#669.)
This leads to control sequences being written to aexpects ShellSession.
E.g. for a running libvirt vm:

from aexpect.client import ShellSession
s = ShellSession("virsh console avocado-vt-vm1")
...
s.sendline("echo $?")
s.read_nonblocking()

==> 'echo $?\n\x1b[?2004l127\n\x1b[?2004h[root@localhost ~]# '

Therefore, cmd_status_output raises a ShellStatusError ("Could not get exit status of command") in https://github.com/avocado-framework/aexpect/blob/master/aexpect/client.py#L1267

As suggested by @huth, by setting export TERM=dumb after successful login (vm.wait_for_login) this behavior can be suppressed:

s.sendline("echo $?")
s.read_nonblocking()

==> 'echo $?\n\x1b[?2004l127\n\x1b[?2004h[root@localhost ~]# '

s.sendline("export TERM=dumb")
s.read_nonblocking()

==> 'export TERM=dumb\n\x1b[?2004l[root@localhost ~]# '

s.sendline("echo $?")
s.read_nonblocking()

==> 'echo $?\n0\n[root@localhost ~]# '

I don't think setting TERM=dumb should be added in each failing test due to this after the vm.wait_for_login() call.
I don't know if this should be fixed in aexpect but maybe it makes more sense that aexpect still allows for control characters to be read per default because there might be test scenarios where this would be important?

Maybe we could solve this by adding a new flag to the virt_vm.wait_for_login method so that export TERM=dumb is executed before the method returns the session to the caller (both qemu_kvm and libvirt_vm inherit that method)? https://github.com/avocado-framework/avocado-vt/blob/master/virttest/virt_vm.py#L1152

As control characters have now become breaking the default behavior should better be to set the TERM always, if I'm not missing anything.

So, something like the following could work:

# virt_vm.py
class BaseVM(object):
...
    def wait_for_login(self, nic_index=0, timeout=LOGIN_WAIT_TIMEOUT,
                       internal_timeout=LOGIN_TIMEOUT,
                       serial=False, restart_network=False,
                       username=None, password=None, status_check=True, term="dump"):
     ...
     session = self.wait_for_serial_login(timeout, internal_timeout,
                                                 restart_network, username,
                                                 password, False)
     if term:
         session.cmd("export TERM=%s" % term)
     if serial:
         return session
     ...
     if serial:
         session = self.wait_for_serial_login(timeout, internal_timeout,
                                                                  False, username, password)
         session.cmd("export TERM=%s" % term)
         return session
     ...

When I played around with this the control characters were not shown in an ssh session within the ShellSession object, therefore, https://github.com/avocado-framework/avocado-vt/blob/master/virttest/virt_vm.py#L1218 might not need this.

@smitterl
Copy link
Contributor Author

Maybe the solution doesn't lie in changing the code but rather documentation and the jeos builds that are downloaded per default? What do you think, @chunfuwen @dzhengfy @ldoktor ?
The documentation could mention this maybe in a separate section "Providing guests":
"Per default, avocado will download the image [...].
If you want to use a customized image you should keep the following in mind:

  1. Image size: Test runs might copy or backup the image. If the image is too big for the host, this might lead to host running out of space.
  2. Terminal type: Many tests will log into a running machine. The selected terminal type will determine if, for example, control characters will be print to stdout. In order to avoid this, we recommend you set TERM=dumb as default in your guest image.
    [...]
    "

@ldoktor
Copy link
Contributor

ldoktor commented Jan 4, 2022

Hello guys, thank you for spotting and addressing this. While reading this I noticed our strip_console_codes does not include bracketed-paste yet, could you please review it here? avocado-framework/aexpect#96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants