Re: [ovs-dev] [RFC PATCH ovn v3 3/3] Add ipam unit tests
On Thu, Nov 12, 2020 at 7:24 PM Mark Gray wrote: > > On 02/11/2020 14:28, Mark Michelson wrote: > > This adds unit tests for IPAM IPv6 initialization, IPv4 initialization, > > and IPv4 address retrieval. It also adds testsuite tests corresponding > > to these unit tests. > > > > The IPv6 initialization and IPv4 initialization tests make use of the > > new unit test framework. They use ovn-appctl to get access to internal > > functions in northd/ipam.c. They require ENABLE_UNIT_TESTS to be > > defined, otherwise the internal unit test code will not be compiled in. > > > > The IPv4 address retrieval test makes use of the pre-existing ovstest > > utility. > > > > Signed-off-by: Mark Michelson > > --- > > northd/ipam.c | 56 ++ > > northd/ovn-northd.c | 3 + > > tests/automake.mk | 8 +- > > tests/ovn-unit-tests.at | 237 > > tests/testsuite.at | 1 + > > 5 files changed, 303 insertions(+), 2 deletions(-) > > create mode 100644 tests/ovn-unit-tests.at > > > > diff --git a/northd/ipam.c b/northd/ipam.c > > index e5383c46f..fb367700b 100644 > > --- a/northd/ipam.c > > +++ b/northd/ipam.c > > @@ -326,3 +326,59 @@ ipam_get_unused_mac(ovs_be32 ip) > > > > return mac64; > > } > > + > > +#ifdef ENABLE_UNIT_TESTS > > + > > > Thanks for looking at this further. IMHO I have a preference to not add > test code in production code. Admittedly, because this is just adding > text at the end of the file, it should not cause any problems at this > stage. However, this makes it too easy to add this #ifdef anywhere in > the file to do things like mocking out objects and this could start > getting messy as the test code paths and production code paths could > start to deviate. There are other patterns (like #include "file.c" and > others suggested by others on this list) that can get around the problem > of testing static functions in the file. > > However, I question the need to do this at all. If we had a need to > achieve some kind of code coverage target (in which we have to make sure > we test X% of all code paths) then we probably would need some method > like this in order to allow this. I think the main problem is the > difficultly testing large files (e.g northd.c, ovn-controller.c). If we > try to find a way to test these private static functions then we are > just covering up the problem that these large files need to be broken up > into something more modular. Therefore, I think we should be splitting > out logical functionality (like you did with ipam.c), defining a > "public" interface, testing against that interface which follows the > pattern in OVS. If we can't achieve our goals with this, then maybe we > should look at methods like this. I would agree with Mark G. Thanks Numan > > ___ > 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
Re: [ovs-dev] [RFC PATCH ovn v3 3/3] Add ipam unit tests
On 02/11/2020 14:28, Mark Michelson wrote: > This adds unit tests for IPAM IPv6 initialization, IPv4 initialization, > and IPv4 address retrieval. It also adds testsuite tests corresponding > to these unit tests. > > The IPv6 initialization and IPv4 initialization tests make use of the > new unit test framework. They use ovn-appctl to get access to internal > functions in northd/ipam.c. They require ENABLE_UNIT_TESTS to be > defined, otherwise the internal unit test code will not be compiled in. > > The IPv4 address retrieval test makes use of the pre-existing ovstest > utility. > > Signed-off-by: Mark Michelson > --- > northd/ipam.c | 56 ++ > northd/ovn-northd.c | 3 + > tests/automake.mk | 8 +- > tests/ovn-unit-tests.at | 237 > tests/testsuite.at | 1 + > 5 files changed, 303 insertions(+), 2 deletions(-) > create mode 100644 tests/ovn-unit-tests.at > > diff --git a/northd/ipam.c b/northd/ipam.c > index e5383c46f..fb367700b 100644 > --- a/northd/ipam.c > +++ b/northd/ipam.c > @@ -326,3 +326,59 @@ ipam_get_unused_mac(ovs_be32 ip) > > return mac64; > } > + > +#ifdef ENABLE_UNIT_TESTS > + Thanks for looking at this further. IMHO I have a preference to not add test code in production code. Admittedly, because this is just adding text at the end of the file, it should not cause any problems at this stage. However, this makes it too easy to add this #ifdef anywhere in the file to do things like mocking out objects and this could start getting messy as the test code paths and production code paths could start to deviate. There are other patterns (like #include "file.c" and others suggested by others on this list) that can get around the problem of testing static functions in the file. However, I question the need to do this at all. If we had a need to achieve some kind of code coverage target (in which we have to make sure we test X% of all code paths) then we probably would need some method like this in order to allow this. I think the main problem is the difficultly testing large files (e.g northd.c, ovn-controller.c). If we try to find a way to test these private static functions then we are just covering up the problem that these large files need to be broken up into something more modular. Therefore, I think we should be splitting out logical functionality (like you did with ipam.c), defining a "public" interface, testing against that interface which follows the pattern in OVS. If we can't achieve our goals with this, then maybe we should look at methods like this. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [RFC PATCH ovn v3 3/3] Add ipam unit tests
This adds unit tests for IPAM IPv6 initialization, IPv4 initialization,
and IPv4 address retrieval. It also adds testsuite tests corresponding
to these unit tests.
The IPv6 initialization and IPv4 initialization tests make use of the
new unit test framework. They use ovn-appctl to get access to internal
functions in northd/ipam.c. They require ENABLE_UNIT_TESTS to be
defined, otherwise the internal unit test code will not be compiled in.
The IPv4 address retrieval test makes use of the pre-existing ovstest
utility.
Signed-off-by: Mark Michelson
---
northd/ipam.c | 56 ++
northd/ovn-northd.c | 3 +
tests/automake.mk | 8 +-
tests/ovn-unit-tests.at | 237
tests/testsuite.at | 1 +
5 files changed, 303 insertions(+), 2 deletions(-)
create mode 100644 tests/ovn-unit-tests.at
diff --git a/northd/ipam.c b/northd/ipam.c
index e5383c46f..fb367700b 100644
--- a/northd/ipam.c
+++ b/northd/ipam.c
@@ -326,3 +326,59 @@ ipam_get_unused_mac(ovs_be32 ip)
return mac64;
}
+
+#ifdef ENABLE_UNIT_TESTS
+
+static void
+test_init_ipam_ipv6_prefix(struct ovs_cmdl_context *ctx)
+{
+const char *prefix = ctx->argc > 1 ? ctx->argv[1] : NULL;
+struct ipam_info ipam;
+struct ds *output = ctx->pvt;
+
+init_ipam_ipv6_prefix(prefix, );
+ds_put_format(output, "ipv6_prefix_set: %s\n",
+ ipam.ipv6_prefix_set ? "true" : "false");
+if (ipam.ipv6_prefix_set) {
+char ipv6[INET6_ADDRSTRLEN];
+inet_ntop(AF_INET6, _prefix,
+ ipv6, sizeof ipv6);
+ds_put_format(output, "ipv6_prefix: %s\n", ipv6);
+}
+}
+
+UNIT_TEST_DEFINE(init_ipam_ipv6_prefix, 0, 1, test_init_ipam_ipv6_prefix);
+
+static void
+test_init_ipam_ipv4(struct ovs_cmdl_context *ctx)
+{
+const char *subnet = ctx->argv[1];
+const char *exclude_ips = ctx->argc > 2 ? ctx->argv[2] : NULL;
+struct ipam_info info;
+
+init_ipam_ipv4(subnet, exclude_ips, );
+struct ds *output = ctx->pvt;
+
+ds_put_format(output, "start_ipv4: " IP_FMT "\n",
+ IP_ARGS(htonl(info.start_ipv4)));
+ds_put_format(output, "total_ipv4s: %" PRIuSIZE "\n", info.total_ipv4s);
+
+ds_put_cstr(output, "allocated_ipv4s: ");
+if (info.allocated_ipv4s) {
+int start = 0;
+int end = info.total_ipv4s;
+for (size_t bit = bitmap_scan(info.allocated_ipv4s, true, start, end);
+ bit != end;
+ bit = bitmap_scan(info.allocated_ipv4s, true, bit + 1, end)) {
+ds_put_format(output, IP_FMT " ",
+ IP_ARGS((htonl(info.start_ipv4 + bit;
+}
+}
+ds_chomp(output, ' ');
+ds_put_char(output, '\n');
+destroy_ipam_info();
+}
+
+UNIT_TEST_DEFINE(init_ipam_ipv4, 1, 2, test_init_ipam_ipv4);
+
+#endif /* ENABLE_UNIT_TESTS */
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 0fb7d0969..41c5df49e 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -36,6 +36,7 @@
#include "lib/ovn-nb-idl.h"
#include "lib/ovn-sb-idl.h"
#include "lib/ovn-util.h"
+#include "lib/unit-test.h"
#include "ovn/actions.h"
#include "ovn/logical-fields.h"
#include "packets.h"
@@ -12742,6 +12743,8 @@ main(int argc, char *argv[])
cluster_state_reset_cmd,
_ovnnb_idl_min_index);
+register_unixctl_unit_test();
+
daemonize_complete();
/* We want to detect (almost) all changes to the ovn-nb db. */
diff --git a/tests/automake.mk b/tests/automake.mk
index 26b6d11b4..782af79b9 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -30,7 +30,8 @@ TESTSUITE_AT = \
tests/ovn-controller-vtep.at \
tests/ovn-ic.at \
tests/ovn-macros.at \
- tests/ovn-performance.at
+ tests/ovn-performance.at \
+ tests/ovn-unit-tests.at
SYSTEM_KMOD_TESTSUITE_AT = \
tests/system-common-macros.at \
@@ -200,7 +201,10 @@ noinst_PROGRAMS += tests/ovstest
tests_ovstest_SOURCES = \
tests/ovstest.c \
tests/ovstest.h \
- tests/test-ovn.c
+ tests/test-ovn.c \
+ northd/test-ipam.c \
+ northd/ipam.c \
+ northd/ipmam.h
tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
$(OVS_LIBDIR)/libopenvswitch.la lib/libovn.la
diff --git a/tests/ovn-unit-tests.at b/tests/ovn-unit-tests.at
new file mode 100644
index 0..50fc53745
--- /dev/null
+++ b/tests/ovn-unit-tests.at
@@ -0,0 +1,237 @@
+AT_BANNER([OVN unit tests])
+
+AT_SETUP([ovn -- unit test -- init_ipam_ipv4])
+AT_SKIP_IF([test "$ENABLE_UNIT_TESTS" = no])
+ovn_start
+
+# Valid subnet, no exclude IPs
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4
192.168.0.0/29], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1
+])
+
+# Valid subnet, single exclude IP
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4
192.168.0.0/29 192.168.0.3], [0], [dnl
+start_ipv4: 192.168.0.1
