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

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

On Wed, 30 Nov 2016 16:44:59 +0100
Pino Toscano <ptoscano@redhat.com> wrote:
> 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.
Ok, I'll merge the two files then.

> > > >  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.
As I see it, the main reason for removing path to vmdk was that the
output is in temporary directory and the path varies between test runs.
This is not the case here because $d is fixed in the test-*.sh file.

Also, the output almost always depends on the test. I don't see a reason
why the path should be treated somewhat differently here. If we remove
the path it still won't allow us to reuse the *.expect file in two tests
(not in this situation).


    Tomas

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

On Thursday, 1 December 2016 13:21:03 CET Tomáš Golembiovský
wrote:
> > > > > 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.
> 
> As I see it, the main reason for removing path to vmdk was that the
> output is in temporary directory and the path varies between test runs.
> This is not the case here because $d is fixed in the test-*.sh file.
Right, but renaming the test means also changing the expected output,
while there's nothing in the test result that really needs to know the
location of the input.
> Also, the output almost always depends on the test. I don't see a
reason
> why the path should be treated somewhat differently here. If we remove
> the path it still won't allow us to reuse the *.expect file in two
tests
> (not in this situation).
We usually leave the path when it matters as test result -- for example,
test-v2v-i-ova-subfolders.expected for test-v2v-i-ova-subfolders.sh,
which needs to check the subdirectory is the expected one.
In the case of your test, the location is where the test file exist,
which is not what needs to be checked.

-- 
Pino Toscano

On Wed, Nov 30, 2016 at 4:44 PM, Pino Toscano <ptoscano@redhat.com> wrote:
> 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:
> > > >  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.
>
>
It turned out that there already is "test-v2v-i-ova.ovf" so renaming
is not
possible. Any other ideas?

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

On Tuesday, 6 December 2016 15:40:33 CET Tomas Golembiovsky
wrote:
> On Wed, Nov 30, 2016 at 4:44 PM, Pino Toscano <ptoscano@redhat.com>
wrote:
> 
> > 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:
> > > > >  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.
> >
> >
> It turned out that there already is "test-v2v-i-ova.ovf" so
renaming is not
> possible. Any other ideas?
Let's drop this idea for now. Sorry for the extra time.

Thanks,
-- 
Pino Toscano