[ovs-dev] [RFC PATCH ovn 0/3] Unit Testing in OVN

[Ilya Maximets]

On 10/12/20 4:57 PM, Mark Gray wrote:
> On 09/10/2020 18:23, Ilya Maximets wrote:
>> On 10/9/20 5:29 PM, Mark Michelson wrote:
>>> OVN has had a test framework for as long as I've been working on the
>>> project. The test framework is designed for performing functional tests
>>> of OVN. That is, with the entirety of OVN up and running, we can provide
>>> configuration and test data and ensure that OVN does with that data what
>>> we expect. This is 100% a good thing and has helped us to detect lots of
>>> bugs before they can actually be merged in.
>>>
>>> What's missing, though, are smaller-scale unit tests. As an example, if
>>> I wanted to test ovn-northd's IPAM code, I would need to start up
>>> ovn-northd, create a logical switch, configure that logical switch to
>>> use IPAM, and then create logical switch ports to exercise the IPAM
>>> code. This can be overkill if my only goal is to ensure that IPAM's
>>> algorithm for selecting the next IP address is correct.
>>>
>>> This patch series proposes a unit test framework for OVN.
>>>
>>> If you want to run the unit tests, you can do so in a couple of ways.
>>>
>>> 1) Within the testsuite.
>>>    ./configure --with-ovs-source=/path/to/ovs --enable-unit-tests
>>>    make check TESTSUITEFLAGS="-k unit"
>>>
>>> 2) One-off from the command line
>>>    ./configure --with-ovs-source=/path/to/ovs --enable-unit-tests
>>>    make sandbox
>>>    ovn-appctl -t ovn-northd unit-test <test_name>
>>>
>>> Some notes on this patch series
>>> 1) Patch 1 is the most important one in the series. This is an RFC
>>> because I'm trying to find out if the unit test framework itself is
>>> good. The refactoring in patch 2 and the unit tests added in patch 3 are
>>> meant to illustrate examples of the framework. They do not necessarily
>>> need to be merged as-is. Feel free to comment on them if you'd like,
>>> though.
>>> 2) Ideally, new unit tests could be added to the testsuite via a script.
>>> They've been added manually in this patch series.
>>> 3) This patch series only adds unit test capabilities to ovn-northd.
>>> However, the patch series we actually merge should add unit test
>>> capabilities to ovn-controller as well.
>>
>> That is an interesting approach, thanks for working n this!
> 
> +1
> 
>> This is very similar to ovstest framework (tests/ovstest.{c,h}), however
>> there are few differences:
>>
>> - ovstest is a separate executable and tests are implemented as a separate
>>   source files.  So, usage is 'ovstest mytest' instead of
>>   'ovn-appctl -t ovn-northd mytest'.  Upside of ovstest is that it doesn't
>>   require test code being integrated to the main executable.  Downside
>>   is that it requires functions being in a separate library that could
>>   be included.  Not sure if this is a downside though, as it forces
>>   developers to avoid huge source files with static functions.
>>
> 
> I don't think this is a downside. If the code was broken down into
> smaller modules, we could do exactly what you are suggesting Ilya. If we
> enable the ability to test code like this, it may reduce the motivation
> for modularization of the code :)
> 
> However, if we did go down this path, the other way that I have seen
> this done (while minimizing the amount test code leaking into production
> code) is to include .c files in your test code.
> 
> #include "northd/ovn-northd.c"
> 
> That way the test code can access private functions and structure in
> the code under test.

That's nasty. :)  Anyway, you can not just include ovn-northd.c because
it contains main() function.  You need to get rid of it somehow at first.

> 
> 
>> - current implementation of ovn unit test framework doesn't allow passing
>>   datasets via cmdline, while each unit test with ovstest could be called
>>   with different cmdline arguments. This makes it easier to test, as you
>>   don't need to rebuild binaries under test.
>>
>> - since ovn unit test framework implements tests inside main source files
>>   these files will, ideally, grow significantly.  ovn-northd is already
>>   13K lines of code, so this, probably, is not a very good thing.
>>
>> You also did a good job decoupling init_ipam_info out of northd stuff,
>> so it basically self-sufficient now.  What my suggestion here is to take
>> one more step forward and move this function to its own library, e.g.
>> move all the ipam related code that could be decoupled to lib/ipam.c
>> and lib/ipam.h.  This might be a good thing to do not only for unit
>> testing purposes, but just as a generic refactoring step that will
>> reduce code complexity and increase readability and maintainability.
>> Also, this will open a road to write separate testing module like
>> tests/test-ipam.c and integrate it into ovstest-like testing framework.
>> Same could be done for all other logically separable parts of northd.
>> Even actual logic of a northd itself could be split in parts, e.g.
>> northd/ovs-northd-something.{c,h}.  In this case all the northd-specific
>> datastructures/functions needed in different modules could be moved to
>> northd/ovs-northd-private.{c,h}.  OVS uses this approach in many places,
>> e.g.  ovsdb/raft.{c,h}, ovsdb/raft-rpc.{c,h}, ovsdb/raft-private.{c,h}.
>> In this example rpc moved outside of main raft logic code and all required
>> common code moved to raft-private module.
>>
>> What do you think?
>>
>> Best regards, Ilya Maximets.
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>