From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <s.lendl@proxmox.com> 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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 1D48BBA01E for <pve-devel@lists.proxmox.com>; Mon, 18 Mar 2024 15:14:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E8C65124E5 for <pve-devel@lists.proxmox.com>; Mon, 18 Mar 2024 15:14:06 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for <pve-devel@lists.proxmox.com>; Mon, 18 Mar 2024 15:14:05 +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 8CC4D466D8; Mon, 18 Mar 2024 15:14:05 +0100 (CET) From: Stefan Lendl <s.lendl@proxmox.com> To: Max Carrara <m.carrara@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com> In-Reply-To: <CZWVTNGOZPCY.12553VXSMMEQC@proxmox.com> References: <20240103153753.407079-3-s.lendl@proxmox.com> <20240103153753.407079-11-s.lendl@proxmox.com> <CZWVTNGOZPCY.12553VXSMMEQC@proxmox.com> Date: Mon, 18 Mar 2024 15:14:04 +0100 Message-ID: <87plvrk4pf.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 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 <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Mon, 18 Mar 2024 14:14:37 -0000 "Max Carrara" <m.carrara@proxmox.com> 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 the >> 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 to >> 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 code >> 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. The >> 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. > 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, then > another `sub` that runs the tests (and optionally one that cleans things > 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 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