Re: [PATCH guest-common v6 05/18] tests: add tests for used guest ID tracking

[Fiona Ebner <f.ebner@proxmox.com>]

Am 24.09.26 um 6:16 PM schrieb Michael Köppl:
> diff --git a/src/tests/guest-id-tests.pl b/src/tests/guest-id-tests.pl
> new file mode 100755
> index 0000000..3be492a
> --- /dev/null
> +++ b/src/tests/guest-id-tests.pl
> @@ -0,0 +1,201 @@
> +#!/usr/bin/perl
> +
> +use v5.36;
> +
> +use lib qw(..);
> +
> +use Test::MockModule;
> +use Test::More;
> +
> +my $FILE = '/etc/pve/virtual-guest/used-guest-ids';
> +
> +my ($parse, $write);
> +my $raw; # contents of the stubbed used-guest-ids file

Nit: maybe opinionated, but I'd prefer s/stubbed/mocked/

> +my $writes = 0;
> +my $write_error;
> +my $vmlist;
> +my $dc_conf = {};
> +
> +my $pve_cluster_module = Test::MockModule->new('PVE::Cluster');
> +$pve_cluster_module->mock(
> +    cfs_register_file => sub($filename, $parser, $writer) {
> +        ($parse, $write) = ($parser, $writer);
> +    },
> +    cfs_read_file => sub($filename) {
> +        return $dc_conf if $filename eq 'datacenter.cfg';
> +        return $parse->("/etc/pve/$filename", $raw);
> +    },
> +    cfs_write_file => sub($filename, $data) {
> +        die $write_error if defined($write_error);
> +        $writes++;
> +        $raw = $write->("/etc/pve/$filename", $data);
> +    },
> +    cfs_lock_file => sub($filename, $timeout, $code) {
> +        return eval { $code->() };
> +    },
> +    get_vmlist => sub() { return $vmlist },
> +);
> +
> +require PVE::GuestID;

Maybe add a short comment that this not just a 'use' at the beginning,
because cfs_register_file() needs to be mocked first, since PVE::GuestID
calls it at compile time.

> +
> +sub guests(@ids) {

The name doesn't tell what the function does. range_to_vmlist() maybe?

> +    return { ids => { map { $_ => { node => 'localhost', type => 'qemu' } } @ids } };
> +}
> +
> +subtest 'parse' => sub {
> +    my $tests = [
> +        ['canonical file', "100-250\n300\n900-1200\n", [[100, 250], [300, 300], [900, 1200]]],
> +        [
> +            'unordered file is sorted but not merged',
> +            "300-400\n100-200\n201-250\n150-160\n",
> +            [[100, 200], [150, 160], [201, 250], [300, 400]],
> +        ],

A test where there's a missing newline at the end might also be nice,
just to see how that is treated.

> +        ['missing file', undef, []],
> +        ['empty file', '', []],
> +        ['blank lines only', "\n  \n", []],
> +    ];
> +
> +    for my $test ($tests->@*) {
> +        my ($name, $input, $expected) = $test->@*;
> +        is_deeply($parse->($FILE, $input), $expected, $name);
> +    }
> +
> +    my @warnings;
> +    local $SIG{__WARN__} = sub { push @warnings, $_[0] };
> +
> +    is_deeply(
> +        $parse->($FILE, "100\nfoo\n50-10\n200\n"),
> +        [[100, 100], [200, 200]],
> +        'invalid entry and reversed range are skipped',
> +    );
> +    is_deeply(
> +        \@warnings,
> +        [
> +            "skipping invalid entry in $FILE: foo\n",
> +            "skipping reversed range in $FILE: 50-10\n",
> +        ],
> +        'skipped entries are warned about',
> +    );
> +};
> +
> +subtest 'write' => sub {
> +    my $tests = [
> +        [
> +            'canonical file is written unchanged',
> +            "100-250\n300\n900-1200\n",
> +            "100-250\n300\n900-1200\n",
> +        ],
> +        [
> +            'overlapping and adjacent ranges are merged',
> +            "300-400\n100-200\n201-250\n150-160\n",
> +            "100-250\n300-400\n",
> +        ],
> +        ['adjacent single IDs are merged', "100\n101\n102\n", "100-102\n"],
> +        ['contained range keeps its container', "100-500\n200-300\n", "100-500\n"],
> +        ['nothing recorded', '', ''],

A test for merging ranges with adjacent IDs might also be nice for
completeness, e.g. "1000-1004\n999\n100-200\n1005\n".

> +    ];
> +
> +    for my $test ($tests->@*) {
> +        my ($name, $input, $expected) = $test->@*;
> +        is($write->($FILE, $parse->($FILE, $input)), $expected, $name);
> +    }
> +};
> +
> +subtest 'get_next_unused_id' => sub {
> +    my $tests = [
> +        # name, file contents, existing guests, input ID, expected ID
> +        ['below all ranges', "100-200\n300-400\n", [], 50, 50],

Nit: since 50 is not a valid guest ID, I'm not sure we should use it in
such a test.

> +        ['start of a range', "100-200\n300-400\n", [], 100, 201],
> +        ['inside a range', "100-200\n300-400\n", [], 150, 201],
> +        ['in a gap', "100-200\n300-400\n", [], 250, 250],
> +        ['above all ranges', "100-200\n300-400\n", [], 500, 500],
> +        ['across adjacent ranges', "100-200\n201-300\n", [], 150, 301],
> +        ['existing guests are skipped', "100-105\n", [106, 107], 100, 108],
> +        ['used range after an existing guest', "100-105\n107-110\n", [106], 100, 111],
> +        ['existing guests not recorded in the file', '', [100, 101], 100, 102],
> +        ['no vmlist', "100-105\n", undef, 100, 106],
> +        ['nothing recorded, no guests', undef, [], 100, 100],
> +    ];
> +
> +    for my $test ($tests->@*) {
> +        my ($name, $file, $existing, $id, $expected) = $test->@*;
> +
> +        $raw = $file;
> +        $vmlist = defined($existing) ? guests($existing->@*) : undef;
> +
> +        is(PVE::GuestID::get_next_unused_id($id), $expected, $name);
> +    }
> +};
> +
> +subtest 'register_used_id' => sub {
> +    $raw = undef;
> +
> +    my $steps = [
> +        ['first ID', 100, "100\n"],
> +        ['extends a range at its end', 101, "100-101\n"],
> +        ['starts a new range after a gap', 103, "100-101\n103\n"],
> +        ['bridges two ranges', 102, "100-103\n"],
> +        ['extends a range at its start', 99, "99-103\n"],

Nit: Similar to above, I wouldn't use 99

> +        ['starts a new range above all others', 200, "99-103\n200\n"],

Nit: This is essentially the same test as 'starts a new range after a
gap' but is described differently.

> +    ];
> +
> +    for my $step ($steps->@*) {
> +        my ($name, $id, $expected) = $step->@*;
> +        PVE::GuestID::register_used_id($id);
> +        is($raw, $expected, $name);
> +    }
> +
> +    $writes = 0;
> +    PVE::GuestID::register_used_id(101);
> +    PVE::GuestID::register_used_id(200);
> +    is($writes, 0, 'already recorded IDs are not written again');
> +    is($raw, "99-103\n200\n", 'file unchanged by already recorded IDs');
> +
> +    $raw = "300-400\n100-200\n201-250\n150-160\n";
> +    PVE::GuestID::register_used_id(275);
> +    is($raw, "100-250\n275\n300-400\n", 'unordered file is normalized on write');
> +};
> +
> +subtest 'register_used_id errors' => sub {
> +    my $tests = [
> +        # name, datacenter.cfg, whether the error reaches the caller
> +        ['unique IDs requested', { 'next-id' => { unique => 1 } }, 1],
> +        ['unique IDs not requested', { 'next-id' => { lower => 100 } }, 0],
> +        ['no next-id settings', {}, 0],
> +    ];
> +
> +    for my $test ($tests->@*) {
> +        my ($name, $conf, $fatal) = $test->@*;

Nit: $fatal -> $expect_fatal

> +
> +        $dc_conf = $conf;
> +        $raw = "100\n";
> +        $write_error = "write failed\n";

Nit: $write_error -> $injected_write_error

> +
> +        my @warnings;
> +        local $SIG{__WARN__} = sub { push @warnings, $_[0] };
> +
> +        eval { PVE::GuestID::register_used_id(500) };
> +
> +        if ($fatal) {
> +            is(
> +                $@,
> +                "unable to record guest ID 500 as used - write failed\n",
> +                "$name: error is passed on to the caller",
> +            );
> +            is_deeply(\@warnings, [], "$name: no warning");
> +        } else {
> +            is($@, '', "$name: error is not passed on");
> +            is_deeply(
> +                \@warnings,
> +                ["unable to record guest ID 500 as used - write failed\n"],
> +                "$name: error is warned about",
> +            );
> +        }
> +        is($raw, "100\n", "$name: file unchanged");
> +    }
> +
> +    $write_error = undef;
> +    $dc_conf = {};
> +};
> +
> +done_testing();