Re: [Qemu-devel] [PATCH] tests/acceptance: Improve failure reporting in linux_ssh_mips_malta.py

[Aleksandar Markovic]

On Jun 11, 2019 1:24 AM, "Cleber Rosa" <address@hidden> wrote:
>
> On Mon, Jun 10, 2019 at 09:49:10PM +0200, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <address@hidden>
> >
> > Rather than optputing a cryptic message:
> >
> > FAIL: True not found in [False],
> >
> > the following will be reported too, if the command output does not meet
> > specified expectations:
> >
> > 'lspci -d 11ab:4620' output doesn't contain the word 'GT-64120'
> >
> > Signed-off-by: Aleksandar Markovic <address@hidden>
> > ---
> >  tests/acceptance/linux_ssh_mips_malta.py | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/acceptance/linux_ssh_mips_malta.py
b/tests/acceptance/linux_ssh_mips_malta.py
> > index aafb0c3..cbf1b34 100644
> > --- a/tests/acceptance/linux_ssh_mips_malta.py
> > +++ b/tests/acceptance/linux_ssh_mips_malta.py
> > @@ -147,20 +147,27 @@ class LinuxSSH(Test):
> >
> >      def run_common_commands(self):
> >          stdout, stderr = self.ssh_command('lspci -d 11ab:4620')
> > -        self.assertIn(True, ["GT-64120" in line for line in stdout])
> > +        self.assertIn(True, ["GT-64120a" in line for line in stdout],
>
> Looks like there's an extra, unintended, "a" in the expected output, that
is,
> s/GT-64120a/GT-64120/.
>
> > +                      "'lspci -d 11ab:4620' output doesn't contain "
> > +                      "the word 'GT-64120'")
> >
> >          stdout, stderr = self.ssh_command('cat
/sys/bus/i2c/devices/i2c-0/name')
> > -        self.assertIn(True, ["SMBus PIIX4 adapter" in line
> > -                             for line in stdout])
> > +        self.assertIn(True, ["SMBus PIIX4 adaptera" in line
>
> Here too (s/adaptera/adapter/).
>
> > +                             for line in stdout],
> > +                      "cat /sys/bus/i2c/devices/i2c-0/name' doesn't
contain "
> > +                      "the words 'SMBus PIIX4 adapter'")
> >
> >          stdout, stderr = self.ssh_command('cat /proc/mtd')
> > -        self.assertIn(True, ["YAMON" in line
> > -                             for line in stdout])
> > +        self.assertIn(True, ["YAMONa" in line
>
> Also here (s/YAMONa/YAMONa/).
>
> > +                             for line in stdout],
> > +                      "'cat /proc/mtd' doesn't contain the word
'YAMON'")
> >
> >          # Empty 'Board Config'
> >          stdout, stderr = self.ssh_command('md5sum /dev/mtd2ro')
> > -        self.assertIn(True, ["0dfbe8aa4c20b52e1b8bf3cb6cbdf193" in line
> > -                             for line in stdout])
> > +        self.assertIn(True, ["0dfbe8aa4c20b52e1b8bf3cb6cbdf193a" in
line
> > +                             for line in stdout],
>
> And finnaly in the hash
(s/0dfbe8aa4c20b52e1b8bf3cb6cbdf193a/0dfbe8aa4c20b52e1b8bf3cb6cbdf193/).
>
> > +                      "'md5sum /dev/mtd2ro' doesn't contain "
> > +                      "the word '0dfbe8aa4c20b52e1b8bf3cb6cbdf193'")
> >
> >      def do_test_mips_malta(self, endianess, kernel_path, uname_m):
> >          self.boot_debian_wheezy_image_and_ssh_login(endianess,
kernel_path)
> > --
> > 2.7.4
> >
> >
>
> With those changes, the tests pass for me.  I'd recommend though:
>
> 1) Not related to your patch, but it's good practice to name unused
>    variables "_", that is:
>
>    stdout, _ = self.ssh_command('lspci -d 11ab:4620')
>
> 2) Avoid repeating the expected content (which lead to the trailing
>    "a"s in this patch).  Something like:
>
>    cmd = 'lspci -d 11ab:4620'
>    stdout, _ = self.ssh_command(cmd)
>    exp = "GT-64120"
>    self.assertIn(True, [exp in line for line in stdout],
>                  '"%s" output does not contain "%s"' % (cmd, exp))
>
> 3) Optionally, create an utility function that would make the check
>    more obvious and avoid looping through all lines of the output
>    (and creating a list, which a list comprehension will do).  Example:
>
>    def ssh_command_output_contains(self, cmd, exp):
>        stdout, _ = self.ssh_command(cmd)
>        for line in stdout:
>            if exp in line:
>                break
>        else:
>            self.fail('"%s" output does not contain "%s"' % (cmd, exp))
>
>     def run_common_commands(self):
>         self.ssh_command_output_contains('lspci -d 11ab:4620', 'GT-64120')
>         self.ssh_command_output_contains('cat
/sys/bus/i2c/devices/i2c-0/name',
>                                          'SMBus PIIX4 adapter')
>         self.ssh_command_output_contains('cat /proc/mtd', 'YAMON')
>         # Empty 'Board Config'
>         self.ssh_command_output_contains('md5sum /dev/mtd2ro',
>
 '0dfbe8aa4c20b52e1b8bf3cb6cbdf193')
>

Thank you for your review, Cleber! I am almost certain that in v2 (that I
am going to send soon), I will adopt the approach from point “3” of your
mail.

Yours,
Aleksandar

> Cheers,
> - Cleber.
>