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?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
