Libguestfs - Nov 2016 - Re: [PATCH v2 5/5] v2v: update tests to match changes in OVA import

On Saturday, 12 November 2016 16:37:53 CET Tomáš Golembiovský
wrote:
> The virt-v2v behaviour for OVA input now depends on QEMU version
> available. The tests affected by this now have two *.expect files and
> the expected result now also depends on the QEMU used.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
This IMHO should be part of patch #4 as well, otherwise applying only
patch #4 without this could lead to test failures (e.g. when bisecting).
>  test-data/utils.sh                      |  21 +++++
qemu-utils.sh?
Also, it must be added to EXTRA_DIST in test-data/Makefile.am.
>  v2v/Makefile.am                         |   1 +
>  v2v/test-v2v-i-ova-formats.sh           |   5 +-
>  v2v/test-v2v-i-ova-subfolders.expected2 |  18 +++++
>  v2v/test-v2v-i-ova-subfolders.sh        |  12 ++-
>  v2v/test-v2v-i-ova-tar.expected         |  18 +++++
>  v2v/test-v2v-i-ova-tar.expected2        |  18 +++++
>  v2v/test-v2v-i-ova-tar.ovf              | 138
++++++++++++++++++++++++++++++++
The ovf and expected files are basically copies of the -formats
versions -- would it be possible to reuse them?
> diff --git a/test-data/utils.sh b/test-data/utils.sh
> new file mode 100755
> index 0000000..49e173f
> --- /dev/null
> +++ b/test-data/utils.sh
> @@ -0,0 +1,21 @@
> +#!/bin/bash -
> +
> +# Returns 0 if QEMU version is greater or equal to the arguments
> +qemu_version() {
This name would suggest it prints the version of qemu, while it really
checks against a wanted version -- I'd suggest qemu_is_version, just
like the simple tool we have for shell tests involving libvirt,
libvirt-is-version (see src/libvirt-is-version.c).
> diff --git a/v2v/test-v2v-i-ova-subfolders.sh
b/v2v/test-v2v-i-ova-subfolders.sh
> index c49f7cb..89a9a3e 100755
> --- a/v2v/test-v2v-i-ova-subfolders.sh
> +++ b/v2v/test-v2v-i-ova-subfolders.sh
> @@ -35,6 +35,7 @@ fi
>  export
VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
>  
>  . $srcdir/../test-data/guestfs-hashsums.sh
> +. $srcdir/../test-data/utils.sh
>  
>  d=test-v2v-i-ova-subfolders.d
>  rm -rf $d
> @@ -56,10 +57,15 @@ popd
>  # normalize the output.
>  $VG virt-v2v --debug-gc --quiet \
>      -i ova $d/test.ova \
> -    --print-source |
> -sed 's,[^ \t]*\(subfolder/disk.*\.vmdk\),\1,' > $d/source
> +    --print-source > $d/source
>  
>  # Check the parsed source is what we expect.
> -diff -u test-v2v-i-ova-subfolders.expected $d/source
> +if qemu_version 2 8 ; then
Here I'd normalize the output, by removing "$d/" (i.e. the path of
the
subdirectory plus the trailing slash):

  sed -i -e "s,$d/,." $d/source
> diff --git a/v2v/test-v2v-i-ova-tar.sh b/v2v/test-v2v-i-ova-tar.sh
> new file mode 100755
> index 0000000..49f7a4b
> --- /dev/null
> +++ b/v2v/test-v2v-i-ova-tar.sh
> @@ -0,0 +1,70 @@
> +#!/bin/bash -
> +# libguestfs virt-v2v test script
> +# Copyright (C) 2014-2016 Red Hat Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
> +
> +# Test -i ova option with ova file compressed in different ways
> +
> +unset CDPATH
> +export LANG=C
> +set -e
> +
> +if [ -n "$SKIP_TEST_V2V_I_OVA_FORMATS_SH" ]; then
> +    echo "$0: test skipped because environment variable is set"
> +    exit 77
> +fi
> +
> +if [ "$(guestfish get-backend)" = "uml" ]; then
> +    echo "$0: test skipped because UML backend does not support
network"
> +    exit 77
> +fi
> +
> +export
VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
> +
> +. $srcdir/../test-data/guestfs-hashsums.sh
> +. $srcdir/../test-data/utils.sh
> +
> +d=test-v2v-i-ova-tar.d
> +rm -rf $d
> +mkdir $d
> +
> +pushd $d
> +
> +# Create a phony OVA.  This is only a test of source parsing, not
> +# conversion, so the contents of the disks doesn't matter.
> +truncate -s 10k disk1.vmdk
> +sha=`do_sha1 disk1.vmdk`
> +echo -e "SHA1(disk1.vmdk)= $sha\r" > disk1.mf
> +tar -cf test-tar.ova ../test-v2v-i-ova-tar.ovf disk1.vmdk disk1.mf
> +
> +popd
> +
> +# Run virt-v2v but only as far as the --print-source stage
> +$VG virt-v2v --debug-gc --quiet \
> +    -i ova $d/test-tar.ova \
> +    --print-source > $d/source
> +
> +# Check the parsed source is what we expect.
> +if qemu_version 2 8 ; then
Ditto.
> diff --git a/v2v/test-v2v-i-ova-two-disks.sh
b/v2v/test-v2v-i-ova-two-disks.sh
> index aefd90e..8a4c877 100755
> --- a/v2v/test-v2v-i-ova-two-disks.sh
> +++ b/v2v/test-v2v-i-ova-two-disks.sh
> @@ -36,6 +36,7 @@ export
VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
>  export VIRTIO_WIN="$srcdir/../test-data/fake-virtio-win"
>  
>  . $srcdir/../test-data/guestfs-hashsums.sh
> +. $srcdir/../test-data/utils.sh
>  
>  d=test-v2v-i-ova-two-disks.d
>  rm -rf $d
> @@ -59,10 +60,15 @@ popd
>  # normalize the output.
>  $VG virt-v2v --debug-gc --quiet \
>      -i ova $d/test.ova \
> -    --print-source |
> -sed 's,[^ \t]*\(disk.*.vmdk\),\1,' > $d/source
> +    --print-source  > $d/source
>  
>  # Check the parsed source is what we expect.
> -diff -u test-v2v-i-ova-two-disks.expected $d/source
> +if qemu_version 2 8 ; then
Ditto.

Thanks,
-- 
Pino Toscano

On Mon, 21 Nov 2016 16:41:49 +0100
Pino Toscano <ptoscano@redhat.com> wrote:
> On Saturday, 12 November 2016 16:37:53 CET Tomáš Golembiovský wrote:
> > The virt-v2v behaviour for OVA input now depends on QEMU version
> > available. The tests affected by this now have two *.expect files and
> > the expected result now also depends on the QEMU used.
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---  
> 
> This IMHO should be part of patch #4 as well, otherwise applying only
> patch #4 without this could lead to test failures (e.g. when bisecting).
> 
> >  test-data/utils.sh                      |  21 +++++  
> 
> qemu-utils.sh?
> Also, it must be added to EXTRA_DIST in test-data/Makefile.am.
 
I wanted to make the name generic enough in case we want to add some
other functions in the future. If you prefer qemu-utils.sh I will rename
it. 

> >  v2v/Makefile.am                         |   1 +
> >  v2v/test-v2v-i-ova-formats.sh           |   5 +-
> >  v2v/test-v2v-i-ova-subfolders.expected2 |  18 +++++
> >  v2v/test-v2v-i-ova-subfolders.sh        |  12 ++-
> >  v2v/test-v2v-i-ova-tar.expected         |  18 +++++
> >  v2v/test-v2v-i-ova-tar.expected2        |  18 +++++
> >  v2v/test-v2v-i-ova-tar.ovf              | 138
++++++++++++++++++++++++++++++++
> 
> The ovf and expected files are basically copies of the -formats
> versions -- would it be possible to reuse them?
Wouldn't that be confusing?

Maybe I could rename it to test-v2v-i-ova.ovf and
test-v2v-i-ova.expected and then use it in both tests.

> > diff --git a/test-data/utils.sh b/test-data/utils.sh
> > new file mode 100755
> > index 0000000..49e173f
> > --- /dev/null
> > +++ b/test-data/utils.sh
> > @@ -0,0 +1,21 @@
> > +#!/bin/bash -
> > +
> > +# Returns 0 if QEMU version is greater or equal to the arguments
> > +qemu_version() {  
> 
> This name would suggest it prints the version of qemu, while it really
> checks against a wanted version -- I'd suggest qemu_is_version, just
> like the simple tool we have for shell tests involving libvirt,
> libvirt-is-version (see src/libvirt-is-version.c).
Good point.

> > diff --git a/v2v/test-v2v-i-ova-subfolders.sh
b/v2v/test-v2v-i-ova-subfolders.sh
> > index c49f7cb..89a9a3e 100755
> > --- a/v2v/test-v2v-i-ova-subfolders.sh
> > +++ b/v2v/test-v2v-i-ova-subfolders.sh
> > @@ -35,6 +35,7 @@ fi
> >  export
VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
> >  
> >  . $srcdir/../test-data/guestfs-hashsums.sh
> > +. $srcdir/../test-data/utils.sh
> >  
> >  d=test-v2v-i-ova-subfolders.d
> >  rm -rf $d
> > @@ -56,10 +57,15 @@ popd
> >  # normalize the output.
> >  $VG virt-v2v --debug-gc --quiet \
> >      -i ova $d/test.ova \
> > -    --print-source |
> > -sed 's,[^ \t]*\(subfolder/disk.*\.vmdk\),\1,' > $d/source
> > +    --print-source > $d/source
> >  
> >  # Check the parsed source is what we expect.
> > -diff -u test-v2v-i-ova-subfolders.expected $d/source
> > +if qemu_version 2 8 ; then  
> 
> Here I'd normalize the output, by removing "$d/" (i.e. the
path of the
> subdirectory plus the trailing slash):
> 
>   sed -i -e "s,$d/,." $d/source
> 
Any specific reason for that? Here $d is specific to the test. It does
not change between single runs of the test.


-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Wednesday, 23 November 2016 16:40:59 CET Tomáš Golembiovský
wrote:
> On Mon, 21 Nov 2016 16:41:49 +0100
> Pino Toscano <ptoscano@redhat.com> wrote:
> 
> > On Saturday, 12 November 2016 16:37:53 CET Tomáš Golembiovský wrote:
> > > The virt-v2v behaviour for OVA input now depends on QEMU version
> > > available. The tests affected by this now have two *.expect files
and
> > > the expected result now also depends on the QEMU used.
> > > 
> > > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > > ---  
> > 
> > This IMHO should be part of patch #4 as well, otherwise applying only
> > patch #4 without this could lead to test failures (e.g. when
bisecting).
> > 
> > >  test-data/utils.sh                      |  21 +++++  
> > 
> > qemu-utils.sh?
> > Also, it must be added to EXTRA_DIST in test-data/Makefile.am.
>  
> I wanted to make the name generic enough in case we want to add some
> other functions in the future. If you prefer qemu-utils.sh I will rename
> it.
In the past I've added test-data/guestfs-hashsums.sh -- maybe that one
could be renamed to test-utils.sh, and then add the new qemu_is_version
there.
> > >  v2v/Makefile.am                         |   1 +
> > >  v2v/test-v2v-i-ova-formats.sh           |   5 +-
> > >  v2v/test-v2v-i-ova-subfolders.expected2 |  18 +++++
> > >  v2v/test-v2v-i-ova-subfolders.sh        |  12 ++-
> > >  v2v/test-v2v-i-ova-tar.expected         |  18 +++++
> > >  v2v/test-v2v-i-ova-tar.expected2        |  18 +++++
> > >  v2v/test-v2v-i-ova-tar.ovf              | 138
++++++++++++++++++++++++++++++++
> > 
> > The ovf and expected files are basically copies of the -formats
> > versions -- would it be possible to reuse them?
> 
> Wouldn't that be confusing?
> 
> Maybe I could rename it to test-v2v-i-ova.ovf and
> test-v2v-i-ova.expected and then use it in both tests.
Good idea.
> > > diff --git a/v2v/test-v2v-i-ova-subfolders.sh
b/v2v/test-v2v-i-ova-subfolders.sh
> > > index c49f7cb..89a9a3e 100755
> > > --- a/v2v/test-v2v-i-ova-subfolders.sh
> > > +++ b/v2v/test-v2v-i-ova-subfolders.sh
> > > @@ -35,6 +35,7 @@ fi
> > >  export
VIRT_TOOLS_DATA_DIR="$srcdir/../test-data/fake-virt-tools"
> > >  
> > >  . $srcdir/../test-data/guestfs-hashsums.sh
> > > +. $srcdir/../test-data/utils.sh
> > >  
> > >  d=test-v2v-i-ova-subfolders.d
> > >  rm -rf $d
> > > @@ -56,10 +57,15 @@ popd
> > >  # normalize the output.
> > >  $VG virt-v2v --debug-gc --quiet \
> > >      -i ova $d/test.ova \
> > > -    --print-source |
> > > -sed 's,[^ \t]*\(subfolder/disk.*\.vmdk\),\1,' >
$d/source
> > > +    --print-source > $d/source
> > >  
> > >  # Check the parsed source is what we expect.
> > > -diff -u test-v2v-i-ova-subfolders.expected $d/source
> > > +if qemu_version 2 8 ; then  
> > 
> > Here I'd normalize the output, by removing "$d/" (i.e.
the path of the
> > subdirectory plus the trailing slash):
> > 
> >   sed -i -e "s,$d/,." $d/source
> > 
> 
> Any specific reason for that? Here $d is specific to the test. It does
> not change between single runs of the test.
Just like we normalize the line in the output containing the vmdk path,
IMHO the reference test output should not contain any path to the test
itself. Easier to read and maintain IMHO.

Thanks,
-- 
Pino Toscano