"Max Carrara" <m.carr...@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â„¢ 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. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel