[pve-devel] [PATCH v2 manager 2/2] vzdump: test: add first tests to the guest include logic

[Thomas Lamprecht]

On 5/4/20 4:08 PM, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
> v1 -> v2: adapt handling of return values, closer to what is used in
> production code.
> 

in general OK, two suggestions below.

>  test/Makefile                      |   5 +-
>  test/vzdump_guest_included_test.pl | 191 +++++++++++++++++++++++++++++
>  2 files changed, 195 insertions(+), 1 deletion(-)
>  create mode 100755 test/vzdump_guest_included_test.pl
> 
> diff --git a/test/Makefile b/test/Makefile
> index c26e16b1..44f3b0d7 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -5,7 +5,7 @@ all:
>  export PERLLIB=..
>  
>  .PHONY: check
> -check: replication_test balloon_test mail_test
> +check: replication_test balloon_test mail_test vzdump_test
>  
>  balloon_test:
>  	./balloontest.pl
> @@ -21,6 +21,9 @@ replication_test:
>  mail_test:
>  	./mail_test.pl
>  
> +vzdump_test:
> +	./vzdump_guest_included_test.pl
> +
>  .PHONY: install
>  install:
>  
> diff --git a/test/vzdump_guest_included_test.pl b/test/vzdump_guest_included_test.pl
> new file mode 100755
> index 00000000..378ad474
> --- /dev/null
> +++ b/test/vzdump_guest_included_test.pl
> @@ -0,0 +1,191 @@
> +#!/usr/bin/perl
> +
> +# Some of the tests can only be applied once the whole include logic is moved
> +# into one single method. Right now parts of it, (all, exclude)  are in the
> +# PVE::VZDump->exec_backup() method.
> +
> +use strict;
> +use warnings;
> +
> +use lib '..';
> +
> +use Test::More tests => 7;
> +use Test::MockModule;
> +
> +use PVE::VZDump;
> +
> +use Data::Dumper;
> +
> +my $vmlist = {
> +    'ids' => {
> +	100 => {
> +	    'type' => 'qemu',
> +	    'node' => 'node1',
> +	},
> +	101 => {
> +	    'type' => 'qemu',
> +	    'node' => 'node1',
> +	},
> +	112 => {
> +	    'type' => 'lxc',
> +	    'node' => 'node1',
> +	},
> +	113 => {
> +	    'type' => 'lxc',
> +	    'node' => 'node1',
> +	},
> +	200 => {
> +	    'type' => 'qemu',
> +	    'node' => 'node2',
> +	},
> +	201 => {
> +	    'type' => 'qemu',
> +	    'node' => 'node2',
> +	},
> +	212 => {
> +	    'type' => 'lxc',
> +	    'node' => 'node2',
> +	},
> +	213 => {
> +	    'type' => 'lxc',
> +	    'node' => 'node2',
> +	},
> +    }
> +};
> +
> +my $pve_cluster_module = Test::MockModule->new('PVE::Cluster');
> +$pve_cluster_module->mock(
> +    get_vmlist => sub {
> +	return $vmlist;
> +    }
> +);
> +
> +my $pve_inotify = Test::MockModule->new('PVE::INotify');
> +$pve_inotify->mock(
> +    nodename => sub {
> +	return 'node1';
> +    }
> +);
> +
> +my $pve_api2tools = Test::MockModule->new('PVE::API2Tools');
> +$pve_api2tools->mock(
> +    get_resource_pool_guest_members => sub {
> +	return [100, 101, 200, 201];

I'd rather have a "real" pool used here, similar to your $vmlist, before the mock you
could just define

my $pools = {
    testpool => [100, 101, 200, 201];
};

No biggie, but add a bit more flexibility from the start without real cost.

> +    }
> +);
> +
> +
> +
> +my $tests = {};
> +
> +# is handled in the PVE::VZDump->exec_backup() method for now
> +# $tests->{'Test all guests'} = {
> +#     expected_vmids => [ 100, 101, 112, 113, 200, 201, 212, 213 ],
> +#     expected_skiplist => [ ],
> +#     param => {
> +# 	all => 1,
> +#     }
> +# };
> +
> +# is handled in the PVE::VZDump->exec_backup() method for now
> +# $tests->{'Test all guests with cluster node limit'} = {
> +#     expected_vmids => [ 100, 101, 112, 113, 200, 201, 212, 213 ],
> +#     expected_skiplist => [],
> +#     param => {
> +# 	all => 1,
> +# 	node => 'node2',
> +#     }
> +# };
> +
> +# is handled in the PVE::VZDump->exec_backup() method for now
> +# $tests->{'Test all guests with local node limit'} = {
> +#     expected_vmids => [ 100, 101, 112, 113 ],
> +#     expected_skiplist => [ 200, 201, 212, 213 ],
> +#     param => {
> +# 	all => 1,
> +# 	node => 'node1',
> +#     }
> +# };
> +#
> +# TODO: all test variants with exclude
> +
> +$tests->{'Test pool members'} = {

This seems a bit like the QEMU way of defining some tests with hard coded manual
managed IDs, so prone to merge and test conflicts that the finally decided to
phase that out.

Please do something like

my $tests = [];
my $addtest = sub {
    my ($name, $test) = @_;
    push @$tests, {
        name => $name,
        test => $test;
    };
};

Which you would use like:

$addtest->('Test pool members', {
    expected_vmids => ...
    ...
});

Side benefit of that one, the order of defining tests matches the execute order,
which could be more expected.

> +    expected_vmids => [ 100, 101 ],
> +    expected_skiplist => [ 200, 201 ],
> +    param => {
> +	pool => 'testpool',
> +    }
> +};
> +
> +$tests->{'Test pool members with cluster node limit'} = {
> +    expected_vmids => [ 100, 101, 200, 201 ],
> +    expected_skiplist => [],
> +    param => {
> +	pool => 'testpool',
> +	node => 'node2',
> +    }
> +};
> +
> +$tests->{'Test pool members with local node limit'} = {
> +    expected_vmids => [ 100, 101 ],
> +    expected_skiplist => [ 200, 201 ],
> +    param => {
> +	pool => 'testpool',
> +	node => 'node1',
> +    }
> +};
> +
> +$tests->{'Test selected VMIDs'} = {
> +    expected_vmids => [ 100 ],
> +    expected_skiplist => [ 201, 212 ],
> +    param => {
> +	vmid => '100, 201, 212',
> +    }
> +};
> +
> +
> +$tests->{'Test selected VMIDs with cluster node limit'} = {
> +    expected_vmids => [ 100, 201, 212 ],
> +    expected_skiplist => [],
> +    param => {
> +	vmid => '100, 201, 212',
> +	node => 'node2',
> +    }
> +};
> +
> +$tests->{'Test selected VMIDs with local node limit'} = {
> +    expected_vmids => [ 100 ],
> +    expected_skiplist => [ 201, 212 ],
> +    param => {
> +	vmid => '100, 201, 212',
> +	node => 'node1',
> +    }
> +};
> +
> +$tests->{'Test selected VMIDs on other nodes'} = {
> +    expected_vmids => [],
> +    expected_skiplist => [ 201, 212 ],
> +    param => {
> +	vmid => '201, 212',
> +	node => 'node1',
> +    }
> +};
> +
> +
> +
> +foreach my $testname (sort keys %{$tests}) {
> +    my $testdata = $tests->{$testname};
> +
> +    note($testname);
> +    my $expected = [ $testdata->{expected_vmids}, $testdata->{expected_skiplist} ];
> +
> +    my ($local, $cluster)  = PVE::VZDump->get_included_guests($testdata->{param});
> +    my $result = [ $local, $cluster ];
> +
> +    # print "Expected: " . Dumper($expected);
> +    # print "Returned: " . Dumper($result);
> +
> +    is_deeply($result, $expected, $testname);
> +}
> +
> +exit(0);
>