Re: [pve-devel] [PATCH pve-network 8/8] test(vnets): add test_vnets_blackbox

[Stefan Lendl <s.lendl@proxmox.com>]

"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™ 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.