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

[Aaron Lauterer]

Besides sending a new version for this series, I will also send a new 
version of series with the actual changes to the backup logic as the 
hints given here also apply there.

On 6/5/20 8:57 AM, Thomas Lamprecht wrote:
> On 5/4/20 4:08 PM, Aaron Lauterer wrote:
>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
[...]
>> +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.
> 

Good point.

>> +    }
>> +);
>> +
>> +
>> +
>> +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.
> 

Thanks for the explanation. :)