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

[Max Carrara]

On Mon Mar 18, 2024 at 3:14 PM CET, Stefan Lendl wrote:
> "Max Carrara" <m.carrara at 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.

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, 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 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. 

You're welcome!