From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id C19BC1FF0A3 for ; Thu, 01 Oct 2026 14:04:23 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 45C502166E; Thu, 01 Oct 2026 14:04:11 +0200 (CEST) Message-ID: <5bcd6a19-8f92-4624-867f-17c3688eb886@proxmox.com> Date: Thu, 1 Oct 2026 14:03:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Fiona Ebner Subject: Re: [PATCH guest-common v6 05/18] tests: add tests for used guest ID tracking To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , pve-devel@lists.proxmox.com References: <20260924161510.847362-1-m.koeppl@proxmox.com> <20260924161510.847362-6-m.koeppl@proxmox.com> Content-Language: en-US In-Reply-To: <20260924161510.847362-6-m.koeppl@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790856236996 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.494 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 7BKO5ESY7O4ZYUQSL7ZBTEHLWXPW7XIF X-Message-ID-Hash: 7BKO5ESY7O4ZYUQSL7ZBTEHLWXPW7XIF X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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();