[SSSD] Re: [PATCH SET] TEST_TOOLS_COLONDB: Add tests for sss_colondb_* public API

[Lukas Slebodnik]

On (29/01/16 14:15), Lukas Slebodnik wrote:
> On (06/01/16 13:47), Petr Cech wrote:
> //snip
> > Lukas, thanks for careful review.
> >
> > There are fixed patches attached. I hope that I check memory leaks in proper
> > way.
> > I am little confused if we need "struct test_colondb_ctx" because it is empty
> > now.
> >
> > Regards
> >
> > -- 
> > Petr^4 Cech
> > From 435b1c44f7b7c351129847f67e7db04898711e9e Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Tue, 24 Nov 2015 10:34:10 -0500
> > Subject: [PATCH 1/2] COLONDB: Add comments on functions
> >
> > The colondb API provides three function:
> > sss_colondb_open()
> > sss_colondb_write_field()
> > sss_colondb_read_field()
> >
> > It is not obvious that sss_colondb_open() add destructor on talloc
> > context which close the colondb during free context. And there is
> > expectation that SSS_COLONDB_SENTINEL is type of last item in line.
> >
> > So this patch adds simple lightening comments in doxygen style.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
>
> LGTM
> > From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Fri, 27 Nov 2015 06:39:37 -0500
> > Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
> >
> > There are three functions at API of colondb wrapper:
> > sss_colondb_open()
> > sss_colondb_readline()
> > sss_colondb_writeline()
> >
> > This patch adds tests for all of them.
> >
> > We test those cases:
> > open nonexisting file for read
> > open nonexisting file for write
> > open existing empty file for read
> > open existing file with records for read
> > open existing empty file for write
> > open existing file with records for write
> > write to empty file
> > write to file with existing records
> > sss_colondb_open()
> > sss_colondb_readline()
> > sss_colondb_write_line()
> > write to empty file and read it
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> >
> > Makefile.am                           |  16 ++
> > src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 439 insertions(+)
> > create mode 100644 src/tests/cmocka/test_tools_colondb.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -241,6 +241,7 @@ if HAVE_CMOCKA
> >         pam-srv-tests \
> >         test_ipa_subdom_util \
> >         test_ipa_subdom_server \
> >    test_tools_colondb \
> >   test_krb5_wait_queue \
> >   test_cert_utils \
> >   test_ldap_id_cleanup \
> >
> > @@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \
> >     libdlopen_test_providers.la \
> >     $(NULL)
> >
> > +test_tools_colondb_SOURCES = \
> > src/tests/cmocka/test_tools_colondb.c \
> > src/tools/common/sss_colondb.c \
> > $(NULL)
> >
> > +test_tools_colondb_CFLAGS = \
> > $(AM_CFLAGS) \
> > $(NULL)
> >
> > +test_tools_colondb_LDFLAGS = \
> > $(NULL)
> >
> > +test_tools_colondb_LDADD = \
> > $(CMOCKA_LIBS) \
> > $(SSSD_INTERNAL_LTLIBS) \
> > libsss_test_common.la \
> > $(NULL)
> >
> > test_krb5_wait_queue_SOURCES = \
> >     src/tests/cmocka/common_mock_be.c \
> >     src/tests/cmocka/test_krb5_wait_queue.c \
> > diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773
> > --- /dev/null
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -0,0 +1,423 @@
> > +/*
> > Authors:
> >    Petr Čech <[e-mail]>
> > Copyright (C) 2015 Red Hat
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> > the Free Software Foundation; either version 3 of the License, or
> > (at your option) any later version.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program.  If not, see http://www.gnu.org/licenses/.
> >
> > +*/
> >
> > +#include <talloc.h>
> > +#include <errno.h>
> > +#include <popt.h>
> >
> > +#include "tests/cmocka/common_mock.h"
> > +#include "src/tools/common/sss_colondb.h"
> >
> > +#define TESTS_PATH "tp_" BASE_FILE_STEM
> > +#define TESTS_FILE "test_colondb.ldb"
> >
> > +const char *TEST_STRING1 = "white";
> > +const int TEST_INT1 = 12;
> >
> > +const char *TEST_STRING2 = "black";
> > +const int TEST_INT2 = 34;
> >
> > +struct test_colondb_ctx {
> > ;
> >
> > +};
>
> "test context" is usually used for passing
> values from setup funtion to tests.
>
> If you plan to reuse in future in extended test
> then it might be easier to directly use TALLOC_CTX.
> > +static void create_dir(const char *path)
> > +{
> > errno_t ret;
> > errno = 0;
> > ret = mkdir(path, 0775);
> > assert_int_equal(ret, 0);
> >
> > +}
> >
> > +static void create_empty_file(const char *path, const char *name)
> > +{
> > TALLOC_CTX *tmp_ctx = NULL;
> > char *file_name = NULL;
> > FILE *fp = NULL;
> > tmp_ctx = talloc_new(NULL);
>  Please do not allocate anything on NULL context.
>  It will disable leak checking leak_check_setup, leak_check_teardown
>  Try to look into leak checking in test
>  src/tests/cmocka/test_child_common.c.
>  And it's not important whether allocation is in test
>  or in setup function. It's better to be consistent,
>
> Otherwise patch LGTM.

There are few issues it the patch set.
http://sssd-ci.duckdns.org/logs/job/36/89/summary.html

Linking error on debian.
/usr/bin/ld: src/tests/cmocka/test_tools_colondb-test_tools_colondb.o: undefined reference to symbol 'poptFreeContext@@LIBPOPT_0'
//lib/x86_64-linux-gnu/libpopt.so.0: error adding symbols: DSO missing from command line

just add $(POPT_LIBS) to test_tools_colondb_LDADD in Makefile.am

and also memory leak
http://sssd-ci.duckdns.org/logs/job/36/89/rhel7/ci-build-debug/test_tools_co...

LS