On Mon, Oct 12, 2020 at 8:45 PM Mark Michelson <mmich...@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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev