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

[Aleksandar Markovic]

On Jun 11, 2019 8:00 AM, "Aleksandar Markovic" <address@hidden>
wrote:
>
>
> 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.
> >

Trailing “a”s are just leftover of my testing the script (forcing it to
report failures), and it is good that you spotted them, but they will
disappear in v2.

Thanks, Aleksandar