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

[Petr Cech]

On 12/07/2015 03:04 PM, Lukas Slebodnik wrote:
> On (30/11/15 14:44), Petr Cech wrote:
> > Hi,
> >
> > this patch set adds tests on colondb API. You can find more information in
> > headers of patches.
> >
> > Regards
> >
> > Petr
> > From 334a4a807750dc4a149e17bbebe26c17dae267ca Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Tue, 24 Nov 2015 10:34:10 -0500
> > Subject: [PATCH 1/4] COLONDB: Add comment on open function
> >
> > 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.
> >
> > So this patch adds simple lightening comment.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> >
> > src/tools/common/sss_colondb.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/tools/common/sss_colondb.h b/src/tools/common/sss_colondb.h
> > index 6edd99cbe3b9ef5c86a48632ac3fc71e8a3e55fe..b2db21b0db4adb1422f4033063bea4895f591b5c 100644
> > --- a/src/tools/common/sss_colondb.h
> > +++ b/src/tools/common/sss_colondb.h
> > @@ -59,6 +59,7 @@ struct sss_colondb_read_field {
> >      union sss_colondb_read_data data;
> > };
> >
> > +/** The close function is set by talloc_destructor. */
>
> I would prefer to ellaborate here if you decided to add doxygen comment.
> I do not know which close function and what is closed.
> I can only guess

Well, I added doxygen comment to all functions. Close function and 
comment about SSS_COLONDB_SENTINEL are what I missed most. I hope now it 
is more clearer how to use it.
> > struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
> >                                       enum sss_colondb_mode mode,
> >                                       const char *filename);
> > --
> > 2.4.3
> > From d4196abbc6134a79970dd953c7821bbe671b2db7 Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Fri, 27 Nov 2015 06:39:37 -0500
> > Subject: [PATCH 2/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_open
> >
> > There are three functions at API of colondb wrapper:
> > sss_colondb_open()
> > sss_colondb_readline()
> > sss_colondb_writeline()
> >
> > This patch adds tests for sss_colondb_open() function.
> >
> > We test those cases:
> > open nonexisting file for read
> > open nonexisting file for write
> > open existing file for read
> > open existing file for write
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> >
> > Makefile.am                           |  19 +++
> > src/tests/cmocka/test_tools_colondb.c | 246 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 265 insertions(+)
> > create mode 100644 src/tests/cmocka/test_tools_colondb.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index d7a6f29520980d933cf0afddf8d041b2d6f3b2ef..ceafd1bf0afd856859e835717a59dceac688f147 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 \
> >
> > @@ -2556,6 +2557,24 @@ 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) \
> > $(POPT_LIBS) \
> > $(SSSD_LIBS) \
>
> Do you really need all libraries from SSSD_LIBS?
>      $(TALLOC_LIBS) \
>      $(TEVENT_LIBS) \
>      $(POPT_LIBS) \
>      $(LDB_LIBS) \
>      $(DBUS_LIBS) \
>      $(PCRE_LIBS) \
>      $(INI_CONFIG_LIBS) \
>      $(COLLECTION_LIBS) \
>      $(DHASH_LIBS) \
>      $(OPENLDAP_LIBS) \
>      $(TDB_LIBS)
>      $(TALLOC_LIBS) \

Addressed.
> > $(SSSD_INTERNAL_LTLIBS) \
> > libsss_debug.la \
>
> libsss_debug.la is already part of SSSD_INTERNAL_LTLIBS.

+1, thank you.
> > $(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..a3f66119e7ab887138a1d03811dfd15f19b89b12
> > --- /dev/null
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -0,0 +1,246 @@
> > +/*
> > 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"
> >
> > +#define new_test_with_file(test)                                               \
> > cmocka_unit_test_setup_teardown(test_##test, test_colondb_with_file_setup, \
> >                                test_colondb_teardown)
>
> I do not like macro generated code; especially after debugging nss-pam-ldapd
> Moreover It's not clear from code which setup and teardwn functions are
> used. Test should be crystal clear and not complicate understandind.
> I know that we have such usage in other tests. But it does not mean
> that we need to add new one. Moreover we wil just safe only few lines of code.

That's the problem. I have used other tests and compiled how to write 
the tests. I am afraid that we told about it some months ago. But I 
forget it.
> > +#define new_test_without_file(test)                                            \
> > cmocka_unit_test_setup_teardown(                                           \
> >    test_##test, test_colondb_without_file_setup, test_colondb_teardown)
> >
> > +struct test_colondb_ctx
> > +{
> > bool is_file_created;
> >
> > +};
> >
> > +static int _create_dir(const char *path)
>           ^
>        could you explain why there is a prefix "_"
>
> We already can distinguish between test and helper function.
> Test has prefix "test_" and is also static. Should it be "_test_" ?

I wanted one more level for hiding. It was bad wish.
> > +{
> > errno_t ret;
> > ret = mkdir(path, 0775);
> > if (ret != 0 && errno != EEXIST) {
> >    fprintf(stderr, "Could not create test directory\n");
> >    return 1;
>
> Please use assertions here. instead of return 1
> it's not ordinary code So it does not make a sense to secial case
> helper functions and test. We can use assertions on all places.
> The same applies to _create_file

Addressed.
> > }
> > return 0;
> >
> > +}
> >
> > +static int _create_file(const char *path, const char *name)
> > +{
> > TALLOC_CTX *tmp_ctx = NULL;
> > char *file_name = NULL;
> > FILE *fp = NULL;
> > errno_t ret;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> >    return 1;
> > }
> > ret = _create_dir(path);
> > if (ret != 0) {
> >    ret = 1;
> >    goto done;
> > }
> > file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
> > if (file_name == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Could not construct colondb path\n");
> >    ret = 1;
> >    goto done;
> > }
> > fp = fopen(file_name, "w");
> > if (fp == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Could not create file %s\n", file_name);
> >    ret = 1;
> >    goto done;
> > }
> > fclose(fp);
> >
> > +done:
> > talloc_free(tmp_ctx);
> > return ret;
> >
> > +}
> >
> > +static int test_colondb_setup(void **state, bool create_file)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > assert_true(leak_check_setup());
> > test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> > assert_non_null(test_ctx);
> > if (create_file) {
> >    _create_file(TESTS_PATH, TESTS_FILE);
> >    test_ctx->is_file_created = true;
> > } else {
> >    test_ctx->is_file_created = false;
> > }
> > *state = test_ctx;
> > return 0;
> >
> > +}
> >
> > +static int test_colondb_without_file_setup(void **state)
> > +{
>
> On most places we do not add prefix to setup and teardown functions.
> The name of function is shorter. And IMHO only test function
> should be prefxed with "_test"

Addressed.
> We can discuss cmocka "coding style" on different thread
> if you want. Just start one.

For start it could be sufficient to say what test from our tests is 
written by the preferable way. And then we could write paragraph to SSSD 
coding style.
> > return test_colondb_setup(state, false);
> >
> > +}
> >
> > +static int test_colondb_with_file_setup(void **state)
> > +{
> > return test_colondb_setup(state, true);
> >
> > +}
> >
> > +static int test_colondb_teardown(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > errno_t ret;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > if (test_ctx->is_file_created) {
> >    errno = 0;
> >    ret = unlink(TESTS_PATH "/" TESTS_FILE);
> >    if (ret != 0 && errno != ENOENT) {
> >        ret = errno;
> >        DEBUG(SSSDBG_CRIT_FAILURE, "Could not delete the test config "
> >                                   "ldb file [%d]: (%s)\n",
> >              ret, sss_strerror(ret));
>        I would use assertions here as well.
>        Because if it fails then other test might be affected by this
>        failure. It shoudl not be ignored. and debug message needn't be
>        enough in some cases.

Addressed.
> >    }
> > }
> > talloc_zfree(*state);
> > test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> > assert_true(leak_check_teardown());
> > return 0;
> >
> > +}
> >
> > +void test_open_nonexist_for_read(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > check_leaks_push(test_ctx);
> > db =
> >    sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/" TESTS_FILE);
> > assert_true(check_leaks_pop(test_ctx) == true);
>                                          ^^^^^^^
>                                      This part is redundant
>                                      also on other places

+2 and addressed.
> > assert_null(db);
> >
> > +}
> >
> > +void test_open_nonexist_for_write(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > check_leaks_push(test_ctx);
> > db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> >                      TESTS_PATH "/" TESTS_FILE);
> > test_ctx->is_file_created = true;
> > assert_true(check_leaks_pop(test_ctx) == true);
> > assert_null(db);
> >
> > +}
> >
> > +void test_open_exist_for_read(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db =
> >    sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> >
> > +}
> >
> > +void test_open_exist_for_write(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> >                      TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> >
> > +}
> >
> > +int main(int argc, const char *argv[])
> > +{
> > poptContext pc;
> > int opt;
> > struct poptOption long_options[] = {
> >    POPT_AUTOHELP SSSD_DEBUG_OPTS POPT_TABLEEND};
> > const struct CMUnitTest tests[] = {
> >    new_test_without_file(open_nonexist_for_read),
> >    new_test_without_file(open_nonexist_for_write),
> >    new_test_with_file(open_exist_for_read),
> >    new_test_with_file(open_exist_for_write),
> > };
> > /* Set debug level to invalid value so we can deside if -d 0 was used. */
> > debug_level = SSSDBG_INVALID;
> > pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> > while ((opt = poptGetNextOpt(pc)) != -1) {
> >    switch (opt) {
> >    default:
> >        fprintf(stderr, "\nInvalid option %s: %s\n\n", poptBadOption(pc, 0),
> >                poptStrerror(opt));
> >        poptPrintUsage(pc, stderr, 0);
> >        return 1;
> >    }
> > }
> > poptFreeContext(pc);
> > DEBUG_CLI_INIT(debug_level);
> > /* Even though normally the tests should clean up after themselves
> > * they might not after a failed run. Remove the old db to be sure */
> > tests_set_cwd();
> > test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> > return cmocka_run_group_tests(tests, NULL, NULL);
> > +}
> >
> > 2.4.3
> > From aa78994e2499dcfea1bc5104d6a2a81087a19259 Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Fri, 27 Nov 2015 10:07:55 -0500
> > Subject: [PATCH 3/4] TEST_TOOLS_COLONDB: Test for sss_colondb_writeline
> >
> > There are three functions at API of colondb wrapper:
> > sss_colondb_open()
> > sss_colondb_readline()
> > sss_colondb_writeline()
> >
> > This patch adds tests for sss_colondb_writeline() function.
> >
> > We test those cases:
> > write to empty file
> > write to file with existing records
> >
> > And tests on sss_colondb_open() with existing file were replaced
> > with tests in cases of  existig empty file and existing file
> > with records.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> >
> > src/tests/cmocka/test_tools_colondb.c | 158 ++++++++++++++++++++++++++++++----
> > 1 file changed, 143 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c
> > index a3f66119e7ab887138a1d03811dfd15f19b89b12..51690a3e7bb365f8a224f385d5e114b8a42419e2 100644
> > --- a/src/tests/cmocka/test_tools_colondb.c
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -28,14 +28,26 @@
> > #define TESTS_PATH "tp_" BASE_FILE_STEM
> > #define TESTS_FILE "test_colondb.ldb"
> >
> > -#define new_test_with_file(test)                                               \
> > cmocka_unit_test_setup_teardown(test_##test, test_colondb_with_file_setup, \
> >                                test_colondb_teardown)
>
> Please do not add and remove code in separate patches.
> last 3 test should resolve ticket #2764.
> IMHO one patch is enough.
> Separate patches make sense if you do complicated changes (@see pavel's sudo
> patches.) But if you only add new code It can be in single patch.

Addressed.
> > +#define TEST_STRING1 "white"
> > +#define TEST_INT1 12
> >
> > +#define TEST_STRING2 "black"
> > +#define TEST_INT2 34
>
> static constants are more type safe.

Addressed. And this point should be in something like Testing Best Practice.
> > #define new_test_without_file(test)                                            \
> >      cmocka_unit_test_setup_teardown(                                           \
> >          test_##test, test_colondb_without_file_setup, test_colondb_teardown)
> >
> > +#define new_test_with_empty_file(test)                                         \
> > cmocka_unit_test_setup_teardown(test_##test,                               \
> >                                test_colondb_with_empty_file_setup,        \
> >                                test_colondb_teardown)
> >
> > +#define new_test_with_nonempty_file(test)                                      \
> > cmocka_unit_test_setup_teardown(test_##test,                               \
> >                                test_colondb_with_nonempty_file_setup,     \
> >                                test_colondb_teardown)
> >
> > struct test_colondb_ctx
> > {
> >      bool is_file_created;
> > @@ -54,7 +66,7 @@ static int _create_dir(const char *path)
> >      return 0;
> > }
> >
> > -static int _create_file(const char *path, const char *name)
> > +static int _create_empty_file(const char *path, const char *name)
> > {
>
> Please do not add and remove code in separate patches.
> You can do in "wor in progress" patches.
> But final version should not do a magic with renamig and moving
> code here and there.

+1
> >  TALLOC_CTX *tmp_ctx = NULL;
> >  char *file_name = NULL;
> >
> > @@ -93,7 +105,39 @@ done:
> >      return ret;
> > }
> >
> > -static int test_colondb_setup(void **state, bool create_file)
> > +static int _create_nonempty_file(const char *path, const char *name)
> > +{
> > TALLOC_CTX *tmp_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > struct sss_colondb_write_field table[] = {
> >    {SSS_COLONDB_STRING, {.str = TEST_STRING2}},
> >    {SSS_COLONDB_UINT32, {.uint32 = TEST_INT2}},
> >    {SSS_COLONDB_SENTINEL, {0}}};
> > errno_t ret;
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> >    return 1;
> > }
> > ret = _create_empty_file(TESTS_PATH, TESTS_FILE);
> > if (ret != 0) {
> >    ret = 1;
> >    goto done;
> > }
> > db =
> >    sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE, TESTS_PATH "/" TESTS_FILE);
> > ret = sss_colondb_writeline(db, table);
> >
> > +done:
> > talloc_free(tmp_ctx);
> > return ret;
> >
> > +}
> >
> > +static int test_colondb_setup(void **state, int file_state)
> > {
> >      struct test_colondb_ctx *test_ctx = NULL;
> >
> > @@ -102,11 +146,21 @@ static int test_colondb_setup(void **state, bool create_file)
> >      test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> >      assert_non_null(test_ctx);
> > if (create_file) {
> >    _create_file(TESTS_PATH, TESTS_FILE);
> > switch (file_state) {
> > case 0:
> >    test_ctx->is_file_created = false;
> >    break;
> > case 1:
> >    _create_empty_file(TESTS_PATH, TESTS_FILE);
> >    test_ctx->is_file_created = true;
> >    break;
> > case 2:
> >    _create_nonempty_file(TESTS_PATH, TESTS_FILE);
> >    test_ctx->is_file_created = true;
> > } else {
> >    break;
> >
> > default:
> >     test_ctx->is_file_created = false;
> >    break;
> >
> > }
> >
> > *state = test_ctx;
> >
> > @@ -116,12 +170,17 @@ static int test_colondb_setup(void **state, bool create_file)
> >
> > static int test_colondb_without_file_setup(void **state)
> > {
> > return test_colondb_setup(state, false);
> > return test_colondb_setup(state, 0);
> >
> > }
> >
> > -static int test_colondb_with_file_setup(void **state)
> > +static int test_colondb_with_empty_file_setup(void **state)
> > {
> > return test_colondb_setup(state, true);
> > return test_colondb_setup(state, 1);
> >
> > +}
> >
> > +static int test_colondb_with_nonempty_file_setup(void **state)
> > +{
> > return test_colondb_setup(state, 2);
> >
> > }
> >
> > static int test_colondb_teardown(void **state)
> > @@ -151,7 +210,6 @@ static int test_colondb_teardown(void **state)
> >
> > void test_open_nonexist_for_read(void **state)
> > {
> > struct test_colondb_ctx *test_ctx = NULL;
> >    struct sss_colondb *db = NULL;
> >
> > @@ -166,7 +224,6 @@ void test_open_nonexist_for_read(void **state)
> >
> > void test_open_nonexist_for_write(void **state)
> > {
> > struct test_colondb_ctx *test_ctx = NULL;
> >    struct sss_colondb *db = NULL;
> >
> > @@ -206,6 +263,72 @@ void test_open_exist_for_write(void **state)
> >      assert_non_null(db);
> > }
> >
> > +void test_open_nonempty_for_read(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db =
> >    sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> >
> > +}
> >
> > +void test_open_nonempty_for_write(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> >                      TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> >
> > +}
> >
> > +void test_write_to_empty(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > struct sss_colondb_write_field table[] = {
> >    {SSS_COLONDB_STRING, {.str = TEST_STRING1}},
> >    {SSS_COLONDB_UINT32, {.uint32 = TEST_INT1}},
> >    {SSS_COLONDB_SENTINEL, {0}}};
> > errno_t ret;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> >                      TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> > ret = sss_colondb_writeline(db, table);
> > assert_true(ret == 0);
> >
> > +}
> >
> > +void test_write_to_nonempty(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > struct sss_colondb_write_field table[] = {
> >    {SSS_COLONDB_STRING, {.str = TEST_STRING1}},
> >    {SSS_COLONDB_UINT32, {.uint32 = TEST_INT1}},
> >    {SSS_COLONDB_SENTINEL, {0}}};
> > errno_t ret;
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> >                      TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> > ret = sss_colondb_writeline(db, table);
> > assert_true(ret == 0);
> >
> > +}
> >
> > int main(int argc, const char *argv[])
> > {
> >      poptContext pc;
> > @@ -216,8 +339,13 @@ int main(int argc, const char *argv[])
> >      const struct CMUnitTest tests[] = {
> >          new_test_without_file(open_nonexist_for_read),
> >          new_test_without_file(open_nonexist_for_write),
> >    new_test_with_file(open_exist_for_read),
> >    new_test_with_file(open_exist_for_write),
> >    new_test_with_empty_file(open_exist_for_read),
> >    new_test_with_empty_file(open_exist_for_write),
> >    new_test_with_nonempty_file(open_nonempty_for_read),
> >    new_test_with_nonempty_file(open_nonempty_for_write),
> >    new_test_with_empty_file(write_to_empty),
> >    new_test_with_nonempty_file(write_to_nonempty),
> >
> > };
> >
> > /* Set debug level to invalid value so we can deside if -d 0 was used. */
> >
> > --
> > 2.4.3
> > From d3d75d8016f195459d8e4d0734f74e0b39f2d79e Mon Sep 17 00:00:00 2001
> > From: Petr Cech 
> > Date: Mon, 30 Nov 2015 06:48:02 -0500
> > Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Tests for sss_colondb_readline
> >
> > There are three functions at API of colondb wrapper:
> > sss_colondb_open()
> > sss_colondb_readline()
> > sss_colondb_write_line()
> >
> > This patch adds tests for sss_colndb_readline() function.
> >
> > We test cases:
> > read from file with records
> > read from empty file
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> >
> > src/tests/cmocka/test_tools_colondb.c | 49 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c
> > index 51690a3e7bb365f8a224f385d5e114b8a42419e2..cad699b7fcf7f22168dd87146fada67dfc512110 100644
> > --- a/src/tests/cmocka/test_tools_colondb.c
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -329,6 +329,52 @@ void test_write_to_nonempty(void **state)
> >      assert_true(ret == 0);
> > }
> >
> > +void test_read_from_nonempty(void **state)
> > +{
> > struct test_colondb_ctx *test_ctx = NULL;
> > struct sss_colondb *db = NULL;
> > errno_t ret;
> > const char *string;
> > uint32_t number;
> > struct sss_colondb_read_field table[] = {
> >    {SSS_COLONDB_STRING, {.str = &string}},
> >    {SSS_COLONDB_UINT32, {.uint32 = &number}},
> >    {SSS_COLONDB_SENTINEL, {0}}};
> > test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > db =
> >    sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/" TESTS_FILE);
> > assert_non_null(db);
> > ret = sss_colondb_readline(test_ctx, db, table);
> > assert_true(ret == 0);
> > assert_string_equal(string, TEST_STRING2);
> > assert_true(number == TEST_INT2);
>    Please use assert_int_equal, so in case of failure we will see values.
>
> Plase alsoo use appropriate cmocka assertion function also on ther places
> We shoudl not use "==" in assertions.
> It's hard to debug

+2 and addressed.
> LS

Thank you for carefull review.

Petr