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

Aleksandar Markovic posted 1 patch 4 years, 10 months ago
Test s390x passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1560196150-30436-1-git-send-email-aleksandar.markovic@rt-rk.com
Maintainers: Aleksandar Rikalo <arikalo@wavecomp.com>, Aurelien Jarno <aurelien@aurel32.net>
tests/acceptance/linux_ssh_mips_malta.py | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
[Qemu-devel] [PATCH] tests/acceptance: Improve failure reporting in linux_ssh_mips_malta.py
Posted by Aleksandar Markovic 4 years, 10 months ago
From: Aleksandar Markovic <amarkovic@wavecomp.com>

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 <amarkovic@wavecomp.com>
---
 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],
+                      "'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
+                             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
+                             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],
+                      "'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


Re: [Qemu-devel] [PATCH] tests/acceptance: Improve failure reporting in linux_ssh_mips_malta.py
Posted by Cleber Rosa 4 years, 10 months ago
On Mon, Jun 10, 2019 at 09:49:10PM +0200, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
> 
> 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 <amarkovic@wavecomp.com>
> ---
>  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')

Cheers,
- Cleber.

Re: [Qemu-devel] [PATCH] tests/acceptance: Improve failure reporting in linux_ssh_mips_malta.py
Posted by Aleksandar Markovic 4 years, 10 months ago
On Jun 11, 2019 1:24 AM, "Cleber Rosa" <crosa@redhat.com> wrote:
>
> On Mon, Jun 10, 2019 at 09:49:10PM +0200, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> >
> > 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 <amarkovic@wavecomp.com>
> > ---
> >  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.
>
Re: [Qemu-devel] [PATCH] tests/acceptance: Improve failure reporting in linux_ssh_mips_malta.py
Posted by Aleksandar Markovic 4 years, 10 months ago
On Jun 11, 2019 8:00 AM, "Aleksandar Markovic" <aleksandar.m.mail@gmail.com>
wrote:
>
>
> On Jun 11, 2019 1:24 AM, "Cleber Rosa" <crosa@redhat.com> wrote:
> >
> > On Mon, Jun 10, 2019 at 09:49:10PM +0200, Aleksandar Markovic wrote:
> > > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> > >
> > > 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 <amarkovic@wavecomp.com>
> > > ---
> > >  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