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

[Numan Siddique]

On Mon, Oct 12, 2020 at 8:45 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> On 10/9/20 1:23 PM, 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!
> > 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.
> >
> > - 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?
>
> Thanks for the detailed feedback, Ilya.
>
> Quite a few things you mentioned ran through my mind as well. For
> instance, when I was doing the IPAM refactor, I considered that it would
> fit better in its own file instead of being in ovn-northd.c. If I put
> IPAM code into lib/ then I could add some code to test-ovn.c to test the
> public portions of it.
>
> I came to the conclusion that a likely endgame here is to move ipam code
> to northd/ipam.[hc]. I could then put a northd/test-ipam.c file in place
> and have it be the test entry point for IPAM testing. I could have this
> northd/test-ipam.c file use ovstest to run the IPAM testing. We would
> need to add something to only compile this test program when testing.
>
> So then the questions remain:
> 1) Can the same treatment be applied to other code? In other words, can
> all code be separated into its own files? With IPAM it was pretty easy
> to identify how it could be separated from the rest of ovn-northd's
> logic. Will other parts separate as easily?
> 2) Even if code is separated, is there a possibility that there will be
> static functions in the separated code that we should test individually?
> If so, then having an external test file won't allow for those functions
> to be tested.
>

I have not yet looked into the patches in detail. I just have one comment now.
I would suggest having a test file for each source file. Like
test_northd.c for example.

A while back I was looking into existing C test frameworks and I came
across this - https://github.com/google/cmockery.git
In order for the test file to access static functions,  cmockery
during preprocessing/compilation, makes all static functions
as non-static.

Can we do something similar ?

Thanks
Numan

> If it turns out that we want to test non-separable code or static
> functions, then having the unit test framework introduced here is a good
> way to perform those tests.
>
> Back on IPAM specifically, an alternative to having northd/test-ipam.c
> would be to put unit tests directly into northd/ipam.[hc]. This way, it
> could test static functions if desired, and it wouldn't require
> conditional compilation of a separate test program.
>
> I'm not familiar with the raft private code you referenced in ovsdb, so
> I can have a look at it to see how that might change the testing approach.
>
> The last major portion of what you suggested is the idea of using
> command line arguments. I left the door open to this by having unit
> tests take a void * parameter. Currently, this is unused in all the
> tests I've introduced. However, it would be possible to have individual
> tests parse command line arguments so that they can have test cases
> added without requiring recompilation.
>
> >
> > Best regards, Ilya Maximets.
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>