[pve-devel] [PATCH storage 1/4] base plugin: Increase test coverage

[Dominik Csapak]

sry for taking so long with the review,
i am looking over now, after that i will test

comments inline

On 4/2/20 1:34 PM, Dominic Jäger wrote:
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> This did not exist separately in RFC
> 
>   test/Makefile            |   5 +-
>   test/run_plugin_tests.pl | 184 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 188 insertions(+), 1 deletion(-)
>   create mode 100755 test/run_plugin_tests.pl
> 
> diff --git a/test/Makefile b/test/Makefile
> index 833a597..c54b10f 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -1,6 +1,6 @@
>   all: test
>   
> -test: test_zfspoolplugin test_disklist test_bwlimit
> +test: test_zfspoolplugin test_disklist test_bwlimit test_plugin
>   
>   test_zfspoolplugin: run_test_zfspoolplugin.pl
>   	./run_test_zfspoolplugin.pl
> @@ -10,3 +10,6 @@ test_disklist: run_disk_tests.pl
>   
>   test_bwlimit: run_bwlimit_tests.pl
>   	./run_bwlimit_tests.pl
> +
> +test_plugin: run_plugin_tests.pl
> +	./run_plugin_tests.pl
> diff --git a/test/run_plugin_tests.pl b/test/run_plugin_tests.pl
> new file mode 100755
> index 0000000..cd93430
> --- /dev/null
> +++ b/test/run_plugin_tests.pl
> @@ -0,0 +1,184 @@
> +#!/usr/bin/perl
> +use strict;
> +use warnings;
> +
> +use lib ('.', '..');
> +use Test::More tests => 32;
> +use Test::MockModule qw(new);
> +use File::Temp qw(tempdir);
> +use File::Path qw(make_path);
> +use Data::Dumper qw(Dumper);
> +use Storable qw(dclone);
> +use PVE::Storage;
> +
> +my $plugin = 'PVE::Storage::Plugin';
> +my $basename = 'test';
> +
> +my $iso_type = 'iso';
> +my $iso_suffix = '.iso';
> +my $iso_notdir = "$basename$iso_suffix";

why 'notdir' ? isn't that just the filename?
i'd rather prefer 'filename' for that..
also one could argue that is just the basename, but i get
we want to differentiate between the name and suffix

anyway i find 'notdir' very confusing^^
any other opinion @all ?

> +my $iso_volname = "$iso_type/$iso_notdir";
> +
> +my $vztmpl_type = 'vztmpl';
> +my $vztmpl_suffix = '.tar.gz';
> +my $vztmpl_notdir = "$basename$vztmpl_suffix";
> +my $vztmpl_volname = "$vztmpl_type/$vztmpl_notdir";
> +
> +my $iso_with_dots = "$iso_type/../$iso_notdir";
> +my $vztmpl_with_dots = "$vztmpl_type/../$vztmpl_notdir";

this is only used in patch 2, would make more sense adding it there

> +
> +my $image_type = 'images';
> +my $vmid = '100';
> +my $image_basename = 'vm-100-disk-0';
> +my $raw_image_format = 'raw'; # Tests for parse_volname don't need the dot
> +my $raw_image_notdir = "$image_basename.$raw_image_format";
> +my $raw_image_volname = "$vmid/$raw_image_notdir";
> +my $qcow_image_format = 'qcow2';
> +my $qcow_image_notdir = "$image_basename.$qcow_image_format";
> +my $qcow_image_volname = "$vmid/$qcow_image_notdir";
> +my $vmdk_image_format = 'vmdk';
> +my $vmdk_image_notdir = "$image_basename.$vmdk_image_format";
> +my $vmdk_image_volname = "$vmid/$vmdk_image_notdir";
> +
> +my $type_index = 0;
> +my $notdir_index = 1;
> +my $format_index = 6;
> +
> +is (($plugin->parse_volname($iso_volname))[$type_index],
> +    $iso_type, 'parse_volname: type for iso');
> +is (($plugin->parse_volname($iso_volname))[$notdir_index],
> +    $iso_notdir, 'parse_volname: notdir for iso');
> +
> +is (($plugin->parse_volname($vztmpl_volname))[$type_index],
> +    $vztmpl_type, 'parse_volname: type for vztmpl');
> +is (($plugin->parse_volname($vztmpl_volname))[$notdir_index],
> +    $vztmpl_notdir, 'parse_volname: notdir for vztmpl');
> +
> +is (($plugin->parse_volname($raw_image_volname))[$type_index],
> +    $image_type, 'parse_volname: type for raw image');
> +is (($plugin->parse_volname($raw_image_volname))[$notdir_index],
> +    $raw_image_notdir, 'parse_volname: notdir for raw image');
> +is (($plugin->parse_volname($raw_image_volname))[$format_index],
> +    $raw_image_format, 'parse_volname: format for raw image');
> +
> +is (($plugin->parse_volname($qcow_image_volname))[$type_index],
> +    $image_type, 'parse_volname: type for qcow image');
> +is (($plugin->parse_volname($qcow_image_volname))[$notdir_index],
> +    $qcow_image_notdir, 'parse_volname: notdir for qcow image');
> +is (($plugin->parse_volname($qcow_image_volname))[$format_index],
> +    $qcow_image_format, 'parse_volname: format for qcow image');
> +
> +is (($plugin->parse_volname($vmdk_image_volname))[$type_index],
> +    $image_type, 'parse_volname: type for vmdk image');
> +is (($plugin->parse_volname($vmdk_image_volname))[$notdir_index],
> +    $vmdk_image_notdir, 'parse_volname: notdir for vmdk image');
> +is (($plugin->parse_volname($vmdk_image_volname))[$format_index],
> +    $vmdk_image_format, 'parse_volname: format for vmdk image');

mhmm.. the tests should work (not tested yet) and are ok,
but this whole thing calls for refactoring, e.g.

my $tests =
[
	{
		name => "raw image",
		volname => "$vmid/$image.raw",
		format => 'raw',
		notdir => "$image.raw",
		type => "images",
	},
	{
		...
	},
	...
];

my $helper = sub {
	my ($volname, $index, $value, $msg) = @_;
	is (($plugin->parse_volname($volname))[$index], $value, "parse_volname: 
$msg";
};

for my $test (@$tests) {
	$helper->($test->{volname}, $type_index, $test->{type}, "type for 
$test->{name}");
	$helper->($test->{volname}, $notdir_index, $test->{notdir}, "notdir for 
$test->{name}");
	$helper->($test->{volname}, $format_index, $test->{format}, "format for 
$test->{name}") if defined($test->{format});
}

this would make it much clearer what the actual test cases are
the helper is maybe not even helpful here, but the
separation of the execution and test data is

this is ofc very rough and can be ignored if thomas/fabian consider
the tests ok

> +
> +
> +my $scfg_with_path = { path => '/some/path' };
> +is ($plugin->get_subdir($scfg_with_path, 'iso'),
> +    "$scfg_with_path->{path}/template/iso", 'get_subdir for iso' );
> +is ($plugin->get_subdir($scfg_with_path, 'vztmpl'),
> +    "$scfg_with_path->{path}/template/cache", 'get_subdir for vztmpl');
> +is ($plugin->get_subdir($scfg_with_path, 'backup'),
> +    "$scfg_with_path->{path}/dump", 'get_subdir for backup');
> +is ($plugin->get_subdir($scfg_with_path, 'images'),
> +    "$scfg_with_path->{path}/images", 'get_subdir for images');
> +is ($plugin->get_subdir($scfg_with_path, 'rootdir'),
> +    "$scfg_with_path->{path}/private", 'get_subdir for rootdir');
> +
> +is ($plugin->filesystem_path($scfg_with_path, $iso_volname),
> +    "$scfg_with_path->{path}/template/$iso_volname",
> +    'filesystem_path for iso');
> +is ($plugin->filesystem_path($scfg_with_path, $vztmpl_volname),
> +    "$scfg_with_path->{path}/template/cache/$vztmpl_notdir",
> +    'filesystem_path for vztmpl');
> +is ($plugin->filesystem_path($scfg_with_path, $raw_image_volname),
> +    "$scfg_with_path->{path}/images/$raw_image_volname",
> +    'filesystem_path for raw images');
> +is ($plugin->filesystem_path($scfg_with_path, $qcow_image_volname),
> +    "$scfg_with_path->{path}/images/$qcow_image_volname",
> +    'filesystem_path for qcow image');
> +is ($plugin->filesystem_path($scfg_with_path, $vmdk_image_volname),
> +    "$scfg_with_path->{path}/images/$vmdk_image_volname",
> +    'filesystem_path for vmdk image');

same with those tests, it is very hard for me (on the first glance)
what we are actually testing...

> +
> +
> +# $expected_volnames may be unsorted
> +sub test_list_volumes {
> +    my ($scfg, $required_type, $expected_volnames, $param) = @_;
> +    my $storage_id = 'sid';
> +    my $volumes_ref = $plugin->list_volumes($storage_id, $scfg, undef,
> +	$required_type, $param);
> +    my $get_volname = sub { my $i = $_->{volid}; $i =~ s/$storage_id://g; $i };
> +    my @received = map ($get_volname->(), @$volumes_ref);
> +    my @x = sort @received;
> +    my @y = sort(@$expected_volnames);
> +    ok (@x ~~ @y, "list_volumes with required type [@$required_type]")
> +    || diag ("Expected\n", Dumper (@y), "but received\n", Dumper(@x));
> +}
> +
> +sub add_test_files {
> +    my ($paths) = @_;
> +    foreach my $path (@$paths) {
> +#	note "Adding test files to path $path";
> +	make_path($path) || BAIL_OUT "Could not create directory $path!";
> +	IO::File->new("$path/$iso_notdir", 'w');
> +	IO::File->new("$path/$iso_notdir", 'w');
> +	IO::File->new("$path/$raw_image_notdir", 'w');
> +	IO::File->new("$path/$qcow_image_notdir", 'w');
> +	IO::File->new("$path/$vmdk_image_notdir", 'w');
> +	IO::File->new("$path/$vztmpl_notdir", 'w');
> +	IO::File->new("$path/garbage", 'w');
> +    }
> +}
> +
> +my $storage_dir_no_rec = File::Temp->newdir();
> +my $paths = [
> +    "$storage_dir_no_rec/template/cache",
> +    "$storage_dir_no_rec/template/iso",
> +    "$storage_dir_no_rec/images/$vmid"
> +];
> +add_test_files($paths);
> +
> +note 'Tests without recursion';
> +# note "Temp dir is:\n", `tree $storage_dir_no_rec`;

i guess this is a leftover?

> +my $scfg_no_rec = { path => $storage_dir_no_rec };
> +test_list_volumes($scfg_no_rec, [$iso_type], [$iso_volname]);
> +test_list_volumes($scfg_no_rec, [$vztmpl_type], [$vztmpl_volname]);
> +
> +my $scfg_with_type = { path => $storage_dir_no_rec, type => 'dir' };
> +my $plugin_mock = Test::MockModule->new('PVE::Cluster');
> +$plugin_mock->redefine('get_vmlist' => sub { return undef });
> +my $expected_volnames_guests = [$qcow_image_volname, $raw_image_volname,
> +   $vmdk_image_volname];
> +note 'Tests for undefined VM type => KVM/Qemu';
> +test_list_volumes($scfg_with_type, ['images'], $expected_volnames_guests);
> +test_list_volumes($scfg_with_type, ['rootdir'], []);
> +test_list_volumes($scfg_with_type, ['images', 'rootdir'],
> +    $expected_volnames_guests);
> +
> +note 'Tests for VM type lxc';
> +$plugin_mock->redefine('get_vmlist' => sub
> +    { return { ids => { $vmid => { type => 'lxc' }}}});
> +test_list_volumes($scfg_with_type, ['images'], []);
> +test_list_volumes($scfg_with_type, ['rootdir'], $expected_volnames_guests);
> +test_list_volumes($scfg_with_type, ['images', 'rootdir'],
> +    $expected_volnames_guests);
> +
> +
> +# see bug report
> +# https://pve.proxmox.com/pipermail/pve-devel/2020-January/041096.html
> +sub test_unmodified_cluster_vmlist {
> +    my ($scfg, $original_vmlist) = @_;
> +    my $tested_vmlist = dclone($original_vmlist);
> +    my $plugin_mock = Test::MockModule->new('PVE::Cluster');
> +    $plugin_mock->redefine('get_vmlist' => sub { return $tested_vmlist });
> +    $plugin->list_volumes('sid', $scfg, undef, ['images']);
> +    is_deeply ($tested_vmlist, $original_vmlist,
> +	'PVE::Cluster::vmlist remains unmodified')
> +    || diag ("Expected vmlist to remain\n", Dumper($original_vmlist),
> +	"but it turned to\n", Dumper($tested_vmlist));
> +}
> +test_unmodified_cluster_vmlist($scfg_with_type, { ids => {} });
>