From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 96C50BA026 for ; Mon, 18 Mar 2024 16:00:43 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7977D1388E for ; Mon, 18 Mar 2024 16:00:13 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 18 Mar 2024 16:00:09 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 50FCA46736 for ; Mon, 18 Mar 2024 16:00:09 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 18 Mar 2024 16:00:08 +0100 Message-Id: From: "Max Carrara" To: "Stefan Lendl" , "Proxmox VE development discussion" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240103153753.407079-3-s.lendl@proxmox.com> <20240103153753.407079-11-s.lendl@proxmox.com> <87plvrk4pf.fsf@gmail.com> In-Reply-To: <87plvrk4pf.fsf@gmail.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.022 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH pve-network 8/8] test(vnets): add test_vnets_blackbox X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Mar 2024 15:00:43 -0000 On Mon Mar 18, 2024 at 3:14 PM CET, Stefan Lendl wrote: > "Max Carrara" writes: > > > On Wed Jan 3, 2024 at 4:37 PM CET, Stefan Lendl wrote: > >> Add several tests for Vnets. State setup as well as testing results is > >> done only via the API to test on the API boundaries not not against th= e > >> internal state. Internal state is mocked to avoid requiring access to > >> system files or pmxcfs. > >> > >> Mocking is done by reading and writing to a hash that holds the entire > >> state of SDN. The state is reset after every test run. > >> > >> Testing is done via helper functions: nic_join and nic_start. > >> When a nic joins a Vnet, currently it always - and only - calls > >> add_next_free_cidr(). The same is true if a nic starts on Vnet, which > >> only calles add_dhcp_mapping. > >> > >> These test functions homogenize the parameter list in contrast to the > >> current calls to the current functions. These functions should move t= o > >> Vnets.pm to be called from QemuServer and LXC! > >> > >> The run_test function takes a function pointer and passes the rest of > >> the arguments to the test functions after resetting the test state. > >> This allows fine-grained parameterization per-test directly in the cod= e > >> instead of separated files that require the entire state to be passed > >> in. > >> > >> The tests setup the SDN by creating a simple zone and a simple vnet. T= he > >> nic_join and nic_start function is called with different subnet > >> configuration wiht and without a dhcp-range configured and with or > >> without an already present IP in the IPAM. > > > > I really like where this is going! Now that I've read through this > > patch, it's become clear why you factored so many calls to commands etc= . > > into their own `sub`s. > > > > Since you mentioned that this is more of an RFC off-list, I get why > > there are a bunch of lines that are commented out at the moment. Those > > obviously shouldn't be committed later on. > > > >> > >> Several of the tests fail and uncovers bugs, that shall be fixed in > >> subsequent commits. > > > > Would be nice to perhaps also have those in the final series though ;) > > Yes agreed will uncomment them in a follow-up. > > > > > Another thing that stood out to me is that some cases could be > > declarative, e.g. the cases for `test_nic_join` and `test_nic_start` > > could be declared in an array for each. You could then just loop over > > the cases - that makes it easier to `plan` those cases later on. > > > > I totally agree it would be nice to have it like that. > I tried to get it there but found unrolling the calls to be more > readable and making the test sub body simpler not requiring to loop in > the test or a setup sub. > > If this approach would be refactored further with some Perl-magic=E2=84= =A2 this > would be nice but I chose this deliberatly for simplicity and readability= . Very fair! Then it's best to just keep it as it is, in that case. > > > That being said, you could perhaps structure the whole script so that > > you call a `sub` named e.g. `setup` where you - well - set up all the > > required logic and perform checks for the necessary pre-conditions, the= n > > another `sub` that runs the tests (and optionally one that cleans thing= s > > up if necessary). > > Yes, agreed as well. Also tried that but chose a "simpler" version for > the first iteration. > > I found that it is sometimes simpler to have dedicated test functions > for example if you have a dhcp-range instead of if-casing whether a a > property is present in the hash. I agree completely - it was more of an idea / nice-to-have. FWIW, if you want to go down that route, you can of course also save references to the `sub`s being run in the array of cases. Alternatively, it's also totally fine to have multiple arrays of cases for each specific function. That being said, if you just want to keep it as it is, that's also totally fine by me. Should the file grow, such a small refactor most likely won't cost that much time. > > I will re-consider a dedicated setup sub for a follow-up. > > > > > Though, please note that this is not a strict necessity from my side, > > just something I wanted to mention! I like the way you've written your > > tests a lot, it's just that I personally tend to prefer a more > > declarative approach. So, it's okay if you just leave the structure as > > it is right now, if you prefer it that way. > > > > There are some more comments inline that give a little more context > > regarding the above, but otherwise, LGTM - pretty good to see more > > testing to be done! > > > > Thanks for the review.=20 You're welcome!