Re: [Openvpn-devel] [PATCH 1/7] put argv_* functions into own file, add unit tests
Hi, On 08-11-16 09:44, Steffan Karger wrote: > I didn't look at the code itself (David already did), but two very minor > nitpicks I ran into while rebasing my src/openvpn/* unit tests on this > patch: > > [..] Apart from this, I noticed that builds that depend on OPENSSL_CFLAGS/MBEDTLS_CFLAGS (or LIBS) would fail to compile the testdriver. Attached a proposed fixup that fixes both me previous nitpicks as well as the test driver build. Feel free to squash into the original patch if you agree. -Steffan From f2214b435cc4a63cf7a58c78915f445feb9ae014 Mon Sep 17 00:00:00 2001 From: Steffan Karger Date: Tue, 8 Nov 2016 14:42:24 +0100 Subject: [PATCH] argv_test fixups --- configure.ac | 4 ++-- tests/unit_tests/Makefile.am | 2 +- tests/unit_tests/openvpn/Makefile.am | 6 -- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/configure.ac b/configure.ac index a42257a..bd6797c 100644 --- a/configure.ac +++ b/configure.ac @@ -1270,10 +1270,10 @@ AC_CONFIG_FILES([ src/plugins/down-root/Makefile tests/Makefile tests/unit_tests/Makefile -tests/unit_tests/plugins/Makefile -tests/unit_tests/plugins/auth-pam/Makefile tests/unit_tests/example_test/Makefile tests/unit_tests/openvpn/Makefile +tests/unit_tests/plugins/Makefile +tests/unit_tests/plugins/auth-pam/Makefile vendor/Makefile sample/Makefile doc/Makefile diff --git a/tests/unit_tests/Makefile.am b/tests/unit_tests/Makefile.am index 44ab26b..31d37b8 100644 --- a/tests/unit_tests/Makefile.am +++ b/tests/unit_tests/Makefile.am @@ -1,5 +1,5 @@ AUTOMAKE_OPTIONS = foreign if CMOCKA_INITIALIZED -SUBDIRS = example_test plugins openvpn +SUBDIRS = example_test openvpn plugins endif diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index af7f12f..b706fae 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -7,8 +7,10 @@ TESTS = $(check_PROGRAMS) openvpn_srcdir = $(top_srcdir)/src/openvpn compat_srcdir = $(top_srcdir)/src/compat -argv_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir) -argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line +argv_testdriver_CFLAGS = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir) \ + $(OPTIONAL_CRYPTO_CFLAGS) +argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line \ + $(OPTIONAL_CRYPTO_LIBS) argv_testdriver_SOURCES = test_argv.c \ $(openvpn_srcdir)/platform.c \ $(openvpn_srcdir)/buffer.c \ -- 2.7.4 signature.asc Description: OpenPGP digital signature -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/7] put argv_* functions into own file, add unit tests
Hi, I didn't look at the code itself (David already did), but two very minor nitpicks I ran into while rebasing my src/openvpn/* unit tests on this patch: On 28-10-16 18:42, Heiko Hund wrote: > --- a/configure.ac > +++ b/configure.ac > @@ -1273,6 +1273,7 @@ AC_CONFIG_FILES([ > tests/unit_tests/plugins/Makefile > tests/unit_tests/plugins/auth-pam/Makefile > tests/unit_tests/example_test/Makefile > +tests/unit_tests/openvpn/Makefile > vendor/Makefile Could we keep this ordering alphabetical? (And fix the example_test occurrence as we go?) > diff --git a/tests/unit_tests/Makefile.am b/tests/unit_tests/Makefile.am > index 8868c1c..44ab26b 100644 > --- a/tests/unit_tests/Makefile.am > +++ b/tests/unit_tests/Makefile.am > @@ -1,5 +1,5 @@ > AUTOMAKE_OPTIONS = foreign > > if CMOCKA_INITIALIZED > -SUBDIRS = example_test plugins > +SUBDIRS = example_test plugins openvpn > endif Here too alphabetical ordering would be a little nicer. Maybe David can tackle this when applying? -Steffan signature.asc Description: OpenPGP digital signature -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/7] put argv_* functions into own file, add unit tests
Hi, On 08-11-16 00:28, David Sommerseth wrote: > I will wait with applying this patch to the git tree until all the other > patches in this series have been reviewed and ACKed. Actually, this patch does not need the follow-up patches, right? In that case, I would strongly prefer if we didn't wait with applying it. I say this because my --tls-crypt patch set also includes src/openvpn/* unit tests, and it would be a lot better if I could rebase that patch set on this patch before sending it to the list. (Less work for the maintainers, and makes my patch set slightly easier to review.) -Steffan signature.asc Description: OpenPGP digital signature -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/7] put argv_* functions into own file, add unit tests
On 28/10/16 18:42, Heiko Hund wrote: > misc.c is too crowded with different things to perform any > sane unit testing due to its dependencies. So, in order to re-write > the #ifdef'ed tests for the argv_* family of functions into unit > tests I moved them into a dedicated file. > > Signed-off-by: Heiko Hund > --- > configure.ac | 1 + > src/openvpn/Makefile.am | 1 + > src/openvpn/argv.c | 412 +++ > src/openvpn/argv.h | 76 ++ > src/openvpn/misc.c | 459 > --- > src/openvpn/misc.h | 48 +--- > tests/unit_tests/Makefile.am | 2 +- > tests/unit_tests/openvpn/Makefile.am | 15 ++ > tests/unit_tests/openvpn/test_argv.c | 194 +++ > 9 files changed, 701 insertions(+), 507 deletions(-) > create mode 100644 src/openvpn/argv.c > create mode 100644 src/openvpn/argv.h > create mode 100644 tests/unit_tests/openvpn/Makefile.am > create mode 100644 tests/unit_tests/openvpn/test_argv.c > ACK. Reviewed that all argv_* functions and the related code is just moved from misc.[ch] to argv.[ch]. One minor change was detected, but it makes sense: The argv_clone() function is not declared static in argv.c. This makes sense as it is only used in this scope. Good to see that we have proper unit testing on argv.c too! These tests doesn't test all functions explicitly, but the majority of them will be tested indirectly through the other calls. The exception is argv_reset(), which I've understood will be changed further later on in this process. I will wait with applying this patch to the git tree until all the other patches in this series have been reviewed and ACKed. -- kind regards, David Sommerseth OpenVPN Technologies, Inc signature.asc Description: OpenPGP digital signature -- Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
