[PATCH] nested groups unit test

[Pavel Březina]

Hi,
this is the first unit test for nested groups. It covers only the most 
basic situation when we are trying to resolve one group with no members.

Even though it is only one test, the patch set is quite big. This is 
because it creates the possibility to mock providers related modules. 
Most of the patches are just a preparation for unit testing providers.

Patches 1-5
Moves the code around to reduce number of dependencies. (E.g. you do not 
want to load fail over when you are testing nested groups.)

Patch 6
Mocks basic SDAP interface.

Patch 7
Mocks sysdb objects - currently user and rfc2307bis group. You can 
decide what set of attributes the object should posses. For example, 
creating a user requires only basedn and name parameter, to construct 
originalDN and name attributes. The rest is provided by (attrname, 
value) pairs via variadic function.

E.g.:
mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)

get_attr_type() translates the sysdb attribute name to proper data type. 
This should be extended as needed.

Patch 8
Adds provider tests related common object files and cflags in makefile

Patch 9
New macro sss_will_return_always(fn, value). This can be used to mock 
function data in such way that any call of mock() will return the value.

It was just pushed also to cmocka upstream as will_return_always().

Patch 10-11
Unit test.

Patch 12
Removes a noisy debug message.

I would like to get this reviewed before I continue with more test 
cases, so the framework is tuned enough.

Off topic:
I also created new macro called fail_msg, which will make the test fail 
printing a message. I didn't use this macro in the end, but it made its 
way to cmocka upstream.

[Pavel Březina]

On 07/18/2013 02:38 PM, Pavel Březina wrote:
> Hi,
> this is the first unit test for nested groups. It covers only the most
> basic situation when we are trying to resolve one group with no members.
>
> Even though it is only one test, the patch set is quite big. This is
> because it creates the possibility to mock providers related modules.
> Most of the patches are just a preparation for unit testing providers.
>
> Patches 1-5
> Moves the code around to reduce number of dependencies. (E.g. you do not
> want to load fail over when you are testing nested groups.)
>
> Patch 6
> Mocks basic SDAP interface.
>
> Patch 7
> Mocks sysdb objects - currently user and rfc2307bis group. You can
> decide what set of attributes the object should posses. For example,
> creating a user requires only basedn and name parameter, to construct
> originalDN and name attributes. The rest is provided by (attrname,
> value) pairs via variadic function.
>
> E.g.:
> mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
>
> get_attr_type() translates the sysdb attribute name to proper data type.
> This should be extended as needed.
>
> Patch 8
> Adds provider tests related common object files and cflags in makefile
>
> Patch 9
> New macro sss_will_return_always(fn, value). This can be used to mock
> function data in such way that any call of mock() will return the value.
>
> It was just pushed also to cmocka upstream as will_return_always().
>
> Patch 10-11
> Unit test.
>
> Patch 12
> Removes a noisy debug message.
>
> I would like to get this reviewed before I continue with more test
> cases, so the framework is tuned enough.
>
> Off topic:
> I also created new macro called fail_msg, which will make the test fail
> printing a message. I didn't use this macro in the end, but it made its
> way to cmocka upstream.

Oops, I did some reorganization while writing this mail but sent patches 
generated sooner, so the numbers does not match. Reordered patches are 
attached.

[Lukas Slebodnik]

On (18/07/13 15:45), Pavel Březina wrote:
> On 07/18/2013 02:38 PM, Pavel Březina wrote:
> > Hi,
> > this is the first unit test for nested groups. It covers only the most
> > basic situation when we are trying to resolve one group with no members.
> >
> > Even though it is only one test, the patch set is quite big. This is
> > because it creates the possibility to mock providers related modules.
> > Most of the patches are just a preparation for unit testing providers.
> >
> > Patches 1-5
> > Moves the code around to reduce number of dependencies. (E.g. you do not
> > want to load fail over when you are testing nested groups.)
> >
> > Patch 6
> > Mocks basic SDAP interface.
> >
> > Patch 7
> > Mocks sysdb objects - currently user and rfc2307bis group. You can
> > decide what set of attributes the object should posses. For example,
> > creating a user requires only basedn and name parameter, to construct
> > originalDN and name attributes. The rest is provided by (attrname,
> > value) pairs via variadic function.
> >
> > E.g.:
> > mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
> >
> > get_attr_type() translates the sysdb attribute name to proper data type.
> > This should be extended as needed.
> >
> > Patch 8
> > Adds provider tests related common object files and cflags in makefile
> >
> > Patch 9
> > New macro sss_will_return_always(fn, value). This can be used to mock
> > function data in such way that any call of mock() will return the value.
> >
> > It was just pushed also to cmocka upstream as will_return_always().
> >
> > Patch 10-11
> > Unit test.
> >
> > Patch 12
> > Removes a noisy debug message.
> >
> > I would like to get this reviewed before I continue with more test
> > cases, so the framework is tuned enough.
> >
> > Off topic:
> > I also created new macro called fail_msg, which will make the test fail
> > printing a message. I didn't use this macro in the end, but it made its
> > way to cmocka upstream.
>
> Oops, I did some reorganization while writing this mail but sent
> patches generated sooner, so the numbers does not match. Reordered
> patches are attached.
> From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:51:37 +0200
> Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip when unit
> testing
>
> src/providers/ldap/ldap_options.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/providers/ldap/ldap_options.c b/src/providers/ldap/ldap_options.c
> index 68bf0d280ba1611ba414576ce0289742911d51da..9e8030f08c4f4de77cc78f7c96a42daca283059c 100644
> --- a/src/providers/ldap/ldap_options.c
> +++ b/src/providers/ldap/ldap_options.c
> @@ -429,6 +429,7 @@ int ldap_get_options(TALLOC_CTX *memctx,
>         goto done;
>     }
>
> +#ifndef UNIT_TESTING
>     /* If there is no KDC, try the deprecated krb5_kdcip option, too */
>     /* FIXME - this can be removed in a future version */
>     ret = krb5_try_kdcip(cdb, conf_path, opts->basic, SDAP_KRB5_KDC);
> @@ -436,6 +437,7 @@ int ldap_get_options(TALLOC_CTX *memctx,
>         DEBUG(1, ("sss_krb5_try_kdcip failed.\n"));
>         goto done;
>     }
> +#endif

Why do you need conditional build?
I think, that you can mock krb5_try_kdcip and every time return EOK.
> authtok_type = dp_opt_get_string(opts->basic, SDAP_DEFAULT_AUTHTOK_TYPE);
> if (authtok_type != NULL &&
>
> -- 
> 1.7.11.7
> From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:58:54 +0200
> Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
> tests
>
> Makefile.am | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Makefile.am b/Makefile.am
> index 6cfc363cea9eae833a8a4bdff12309286475b89a..9065ee4b6312bc808de0253d81df68e38f5c633e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1209,6 +1209,19 @@ TEST_MOCK_RESP_OBJ = \
>      src/responder/common/responder_cmd.c \
>      src/responder/common/negcache.c \
>      src/responder/common/responder_common.c

^^^^^
trailing whitespaces
> +TEST_MOCK_PROVIDER_OBJ = \
> src/util/sss_ldap.c \
> src/providers/data_provider_opts.c \
> src/providers/ldap/ldap_options.c \
> src/providers/ldap/sdap.c \
> src/providers/ldap/sdap_utils.c \
> src/providers/ldap/sdap_range.c \
> src/tests/cmocka/common_mock_sdap.c \
> src/tests/cmocka/common_mock_sysdb_objects.c

^^^^^
trailing whitespaces
> +TEST_MOCK_PROVIDER_CFLAGS = \
> -DUNIT_TESTING
>
> nss_srv_tests_DEPENDENCIES = \
>      $(ldblib_LTLIBRARIES)
> -- 
> 1.7.11.7

[Jakub Hrozek]

On Thu, Jul 18, 2013 at 03:45:35PM +0200, Pavel Březina wrote:
> On 07/18/2013 02:38 PM, Pavel Březina wrote:
> > Hi,
> > this is the first unit test for nested groups. It covers only the most
> > basic situation when we are trying to resolve one group with no members.
> >
> > Even though it is only one test, the patch set is quite big. This is
> > because it creates the possibility to mock providers related modules.
> > Most of the patches are just a preparation for unit testing providers.
> >
> > Patches 1-5
> > Moves the code around to reduce number of dependencies. (E.g. you do not
> > want to load fail over when you are testing nested groups.)
> >
> > Patch 6
> > Mocks basic SDAP interface.
> >
> > Patch 7
> > Mocks sysdb objects - currently user and rfc2307bis group. You can
> > decide what set of attributes the object should posses. For example,
> > creating a user requires only basedn and name parameter, to construct
> > originalDN and name attributes. The rest is provided by (attrname,
> > value) pairs via variadic function.
> >
> > E.g.:
> > mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
> >
> > get_attr_type() translates the sysdb attribute name to proper data type.
> > This should be extended as needed.
> >
> > Patch 8
> > Adds provider tests related common object files and cflags in makefile
> >
> > Patch 9
> > New macro sss_will_return_always(fn, value). This can be used to mock
> > function data in such way that any call of mock() will return the value.
> >
> > It was just pushed also to cmocka upstream as will_return_always().
> >
> > Patch 10-11
> > Unit test.
> >
> > Patch 12
> > Removes a noisy debug message.
> >
> > I would like to get this reviewed before I continue with more test
> > cases, so the framework is tuned enough.
> >
> > Off topic:
> > I also created new macro called fail_msg, which will make the test fail
> > printing a message. I didn't use this macro in the end, but it made its
> > way to cmocka upstream.
>
> Oops, I did some reorganization while writing this mail but sent
> patches generated sooner, so the numbers does not match. Reordered
> patches are attached.

The patches no longer apply (not suprising after such a long time on the
list..)

I will add some quick comments inline without trying out the patches.
> From 9b370ed83ec5c85819781145de92c39977fd19eb Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 16 Jul 2013 11:21:02 +0200
> Subject: [PATCH 01/12] sdap: move non async functions from sdap_async.c to
>  sdap_utils.c
>
> Makefile.am                     |   1 +
>  src/providers/ldap/sdap_async.c | 107 --------------------------------
>  src/providers/ldap/sdap_utils.c | 131 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 132 insertions(+), 107 deletions(-)
>  create mode 100644 src/providers/ldap/sdap_utils.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 515843420834bbbd0e5b4c86c114294c89afa752..6cfc363cea9eae833a8a4bdff12309286475b89a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1450,6 +1450,7 @@ libsss_ldap_common_la_SOURCES = \
>      src/providers/ldap/sdap_reinit.c \
>      src/providers/ldap/sdap_dyndns.c \
>      src/providers/ldap/sdap_refresh.c \
> src/providers/ldap/sdap_utils.c \
> src/providers/ldap/sdap.c
>
> libsss_ldap_common_la_LDFLAGS = \
>      -avoid-version
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index bdd3c3c8b2ed4cc7246c008ddbfe531b82d880bc..6d29c1a566f82c2ce0dead2ca8073ace15afbf56 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -23,26 +23,8 @@
>  #include "util/util.h"
>  #include "providers/ldap/sdap_async_private.h"
>
> -#define REALM_SEPARATOR '@'
>  #define REPLY_REALLOC_INCREMENT 10
>
> -void make_realm_upper_case(const char *upn)

This function seems to be called from a single location only. Can we
either ditch it in favor of get_uppercase_realm() from usertools.c or
make it static?
> -{
> char *c;
> c = strchr(upn, REALM_SEPARATOR);
> if (c == NULL) {
>    DEBUG(9, ("No realm delimiter found in upn [%s].\n", upn));
>    return;
> }
> while(*(++c) != '\0') {
>    c[0] = toupper(*c);
> }
> return;
>
> -}
> From 4f76600299c31489c98f960d162ab7c1d2a29126 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:20:43 +0200
> Subject: [PATCH 02/12] sdap: move non async functions from
>  sdap_async_connection.c to sdap_utils.c

Looks good to me.
> From dab6d9221bafe0e18f599b88de10b7cc57a57b8e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 14:28:26 +0200
> Subject: [PATCH 03/12] sdap: move sdap_get_id_specific_filter() to
>  sdap_utils.c

I think this makes sense.

Since we started talking about LDAP provider refactoring on IRC
recently, I would like to bring up the ldap_*.c vs. sdap_*.c confusion.

In my opinion, we should separate libsss_ldap.la as discussed on #sssd
the other day and keep its sources in sdap*.c. Only code specific to the
initialization of the LDAP provider should be kept in ldap*.c
> From a97267f35a598e7f944cf993b37cd6b43db7a572 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:50:34 +0200
> Subject: [PATCH 04/12] ldap: move options related content from ldap_common.c
>  to ldap_options.c
>
> src/providers/ldap/ldap_common.c                   | 886 --------------------
>  .../ldap/{ldap_common.c => ldap_options.c}         | 921 ---------------------
>  2 files changed, 1807 deletions(-)
>  copy src/providers/ldap/{ldap_common.c => ldap_options.c} (53%)
>
> diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
> index 5782c4615ddd3778d16e52986eae065c3c61b8e5..7a8bf93d2366a517e545f1faecdc3aefa2be36e7 100644
> --- a/src/providers/ldap/ldap_common.c
> +++ b/src/providers/ldap/ldap_common.c
> @@ -39,892 +39,6 @@
>  /* a fd the child process would log into */
>  int ldap_child_debug_fd = -1;
>
> -int
> -sdap_domain_destructor(void *mem)
> -{
> struct sdap_domain *dom =
>        talloc_get_type(mem, struct sdap_domain);
> DLIST_REMOVE(*(dom->head), dom);
> return 0;
>
> -}
>
> -struct sdap_domain *
> -sdap_domain_get(struct sdap_options *opts,
>            struct sss_domain_info *dom)
>
> -{
> struct sdap_domain *sditer = NULL;
> DLIST_FOR_EACH(sditer, opts->sdom) {
>    if (sditer->dom == dom) {
>        break;
>    }
> }
> return sditer;
>
> -}
>
> -errno_t
> -sdap_domain_add(struct sdap_options *opts,
>            struct sss_domain_info *dom,
>            struct sdap_domain **_sdom)
>
> -{
> struct sdap_domain *sdom;
> sdom = talloc_zero(opts, struct sdap_domain);
> if (sdom == NULL) {
>    return ENOMEM;
> }
> sdom->dom = dom;
> sdom->head = &opts->sdom;
> if (opts->sdom) {
>    /* Only allow subdomains of the parent domain */
>    if (dom->parent == NULL ||
>        dom->parent != opts->sdom->dom) {
>        DEBUG(SSSDBG_OP_FAILURE, ("Domain %s is not a subdomain of %s\n",
>              dom->name, opts->sdom->dom->name));
>        return EINVAL;
>    }
> }
> talloc_set_destructor((TALLOC_CTX *)sdom, sdap_domain_destructor);
> DLIST_ADD_END(opts->sdom, sdom, struct sdap_domain *);
> if (_sdom) *_sdom = sdom;
> return EOK;
>
> -}
>
> -errno_t
> -sdap_domain_subdom_add(struct sdap_id_ctx *sdap_id_ctx,
>                   struct sdap_domain *sdom_list,
>                   struct sss_domain_info *parent)
>
> -{
> struct sss_domain_info *dom;
> struct sdap_domain *sdom, *sditer;
> char *basedn;
> errno_t ret;
> for (dom = get_next_domain(parent, true);
>     dom && IS_SUBDOMAIN(dom); /* if we get back to a parent, stop */
>     dom = get_next_domain(dom, false)) {
>    DLIST_FOR_EACH(sditer, sdom_list) {
>        if (sditer->dom == dom) {
>            break;
>        }
>    }
>    if (sditer == NULL) {
>        /* New sdap domain */
>        DEBUG(SSSDBG_TRACE_FUNC, ("subdomain %s is a new one, will "
>              "create a new sdap domain object\n", dom->name));
>        ret = sdap_domain_add(sdap_id_ctx->opts, dom, &sdom);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_OP_FAILURE,
>                ("Cannot add new sdap domain for domain %s [%d]: %s\n",
>                parent->name, ret, strerror(ret)));
>            return ret;
>        }
>    } else {
>        sdom = sditer;
>    }
>    /* Convert the domain name into search base */
>    ret = domain_to_basedn(sdom, sdom->dom->name, &basedn);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE,
>            ("Cannot convert domain name [%s] to base DN [%d]: %s\n",
>            dom->name, ret, strerror(ret)));
>        talloc_free(basedn);
>        return ret;
>    }
>    /* Update search bases */
>    talloc_zfree(sdom->search_bases);
>    sdom->search_bases = talloc_array(sdom, struct sdap_search_base *, 2);
>    if (sdom->search_bases == NULL) {
>        return ret;
>    }
>    sdom->search_bases[1] = NULL;
>    ret = sdap_create_search_base(sdom, basedn, LDAP_SCOPE_SUBTREE, NULL,
>                                  &sdom->search_bases[0]);
>    talloc_free(basedn);
>    if (ret) {
>        DEBUG(SSSDBG_OP_FAILURE, ("Cannot create new sdap search base\n"));
>        return ret;
>    }
>    sdom->user_search_bases = sdom->search_bases;
>    sdom->group_search_bases = sdom->search_bases;
>    sdom->netgroup_search_bases = sdom->search_bases;
>    sdom->sudo_search_bases = sdom->search_bases;
>    sdom->service_search_bases = sdom->search_bases;
>    sdom->autofs_search_bases = sdom->search_bases;
> }
> return EOK;
>
> -}
>
> -void
> -sdap_domain_remove(struct sdap_options *opts,
>               struct sss_domain_info *dom)
>
> -{
> struct sdap_domain *sdom;
> sdom = sdap_domain_get(opts, dom);
> if (sdom == NULL) return;
> DLIST_REMOVE(*(sdom->head), sdom);
>
> -}

Would it make sense to separate sdap_domain_* functions to an sdap
module instead?

Moving ldap_get_$provider_options makes sense to me.
> From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:51:37 +0200
> Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip when unit
>  testing

I agree with Lukas, I would prefer not to add ifdefs to the code unless
needed.
> From d1e5b04f0cc7ece9bf7fccd564626e488d532951 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 26 Mar 2013 15:07:17 +0100
> Subject: [PATCH 06/12] tests: mock SDAP

Looks good to me.
> From e64bce82249f1b8be7bd5eb8bd4fd01d7a5fb5dd Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 26 Mar 2013 15:18:21 +0100
> Subject: [PATCH 07/12] tests: mock sysdb users and groups

I think the approach is fine for unit tests. I don't see us change the
types of sysdb attributes frequently.
> From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:58:54 +0200
> Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
>  tests

Looks good to me.
> From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Thu, 18 Jul 2013 13:24:26 +0200
> Subject: [PATCH 09/12] tests: new macro sss_will_return_always

Is this feature available in all cmocka releases (IIRC you wrote it) ?
> From 65da81b7fe5418f44ec743fc11520c5e68a85435 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 26 Mar 2013 15:06:44 +0100
> Subject: [PATCH 10/12] tests: add confdb_path to sss_test_ctx

Looks good to me.
> From 03e318c1319f8b3545195d7deee3d55793fc941a Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Wed, 17 Jul 2013 13:59:36 +0200
> Subject: [PATCH 11/12] tests: nested groups unit test

I only scrolled through this one, but in general looks good.
> From 53ff2f0812b6ae81642513aee693284ad134ba47 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Thu, 18 Jul 2013 14:05:05 +0200
> Subject: [PATCH 12/12] tests: don't print debug message when test dir does
>  not exist

Looks good to me.

[Pavel Březina]

On 11/05/2013 12:22 PM, Jakub Hrozek wrote:
> On Thu, Jul 18, 2013 at 03:45:35PM +0200, Pavel Březina wrote:
> > On 07/18/2013 02:38 PM, Pavel Březina wrote:
> > > Hi,
> > > this is the first unit test for nested groups. It covers only the most
> > > basic situation when we are trying to resolve one group with no members.
> > >
> > > Even though it is only one test, the patch set is quite big. This is
> > > because it creates the possibility to mock providers related modules.
> > > Most of the patches are just a preparation for unit testing providers.
> > >
> > > Patches 1-5
> > > Moves the code around to reduce number of dependencies. (E.g. you do not
> > > want to load fail over when you are testing nested groups.)
> > >
> > > Patch 6
> > > Mocks basic SDAP interface.
> > >
> > > Patch 7
> > > Mocks sysdb objects - currently user and rfc2307bis group. You can
> > > decide what set of attributes the object should posses. For example,
> > > creating a user requires only basedn and name parameter, to construct
> > > originalDN and name attributes. The rest is provided by (attrname,
> > > value) pairs via variadic function.
> > >
> > > E.g.:
> > > mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
> > >
> > > get_attr_type() translates the sysdb attribute name to proper data type.
> > > This should be extended as needed.
> > >
> > > Patch 8
> > > Adds provider tests related common object files and cflags in makefile
> > >
> > > Patch 9
> > > New macro sss_will_return_always(fn, value). This can be used to mock
> > > function data in such way that any call of mock() will return the value.
> > >
> > > It was just pushed also to cmocka upstream as will_return_always().
> > >
> > > Patch 10-11
> > > Unit test.
> > >
> > > Patch 12
> > > Removes a noisy debug message.
> > >
> > > I would like to get this reviewed before I continue with more test
> > > cases, so the framework is tuned enough.
> > >
> > > Off topic:
> > > I also created new macro called fail_msg, which will make the test fail
> > > printing a message. I didn't use this macro in the end, but it made its
> > > way to cmocka upstream.
> >
> > Oops, I did some reorganization while writing this mail but sent
> > patches generated sooner, so the numbers does not match. Reordered
> > patches are attached.
>
> The patches no longer apply (not suprising after such a long time on the
> list..)
>
> I will add some quick comments inline without trying out the patches.
> > From 9b370ed83ec5c85819781145de92c39977fd19eb Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 16 Jul 2013 11:21:02 +0200
> > Subject: [PATCH 01/12] sdap: move non async functions from sdap_async.c to
> >   sdap_utils.c
> >
> > Makefile.am                     |   1 +
> >   src/providers/ldap/sdap_async.c | 107 --------------------------------
> >   src/providers/ldap/sdap_utils.c | 131 ++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 132 insertions(+), 107 deletions(-)
> >   create mode 100644 src/providers/ldap/sdap_utils.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 515843420834bbbd0e5b4c86c114294c89afa752..6cfc363cea9eae833a8a4bdff12309286475b89a 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -1450,6 +1450,7 @@ libsss_ldap_common_la_SOURCES = \
> >       src/providers/ldap/sdap_reinit.c \
> >       src/providers/ldap/sdap_dyndns.c \
> >       src/providers/ldap/sdap_refresh.c \
> > src/providers/ldap/sdap_utils.c \
> >  src/providers/ldap/sdap.c
> > libsss_ldap_common_la_LDFLAGS = \
> >  -avoid-version
> >
> > diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> > index bdd3c3c8b2ed4cc7246c008ddbfe531b82d880bc..6d29c1a566f82c2ce0dead2ca8073ace15afbf56 100644
> > --- a/src/providers/ldap/sdap_async.c
> > +++ b/src/providers/ldap/sdap_async.c
> > @@ -23,26 +23,8 @@
> >   #include "util/util.h"
> >   #include "providers/ldap/sdap_async_private.h"
> >
> > -#define REALM_SEPARATOR '@'
> >   #define REPLY_REALLOC_INCREMENT 10
> >
> > -void make_realm_upper_case(const char *upn)
>
> This function seems to be called from a single location only. Can we
> either ditch it in favor of get_uppercase_realm() from usertools.c or
> make it static?
> > -{
> > char *c;
> > c = strchr(upn, REALM_SEPARATOR);
> > if (c == NULL) {
> >    DEBUG(9, ("No realm delimiter found in upn [%s].\n", upn));
> >    return;
> > }
> > while(*(++c) != '\0') {
> >    c[0] = toupper(*c);
> > }
> > return;
> >
> > -}
> > From 4f76600299c31489c98f960d162ab7c1d2a29126 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 17 Jul 2013 13:20:43 +0200
> > Subject: [PATCH 02/12] sdap: move non async functions from
> >   sdap_async_connection.c to sdap_utils.c
>
> Looks good to me.
> > From dab6d9221bafe0e18f599b88de10b7cc57a57b8e Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 17 Jul 2013 14:28:26 +0200
> > Subject: [PATCH 03/12] sdap: move sdap_get_id_specific_filter() to
> >   sdap_utils.c
>
> I think this makes sense.
>
> Since we started talking about LDAP provider refactoring on IRC
> recently, I would like to bring up the ldap_*.c vs. sdap_*.c confusion.
>
> In my opinion, we should separate libsss_ldap.la as discussed on #sssd
> the other day and keep its sources in sdap*.c. Only code specific to the
> initialization of the LDAP provider should be kept in ldap*.c
> > From a97267f35a598e7f944cf993b37cd6b43db7a572 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 17 Jul 2013 13:50:34 +0200
> > Subject: [PATCH 04/12] ldap: move options related content from ldap_common.c
> >   to ldap_options.c
> >
> > src/providers/ldap/ldap_common.c                   | 886 --------------------
> >   .../ldap/{ldap_common.c => ldap_options.c}         | 921 ---------------------
> >   2 files changed, 1807 deletions(-)
> >   copy src/providers/ldap/{ldap_common.c => ldap_options.c} (53%)
> >
> > diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
> > index 5782c4615ddd3778d16e52986eae065c3c61b8e5..7a8bf93d2366a517e545f1faecdc3aefa2be36e7 100644
> > --- a/src/providers/ldap/ldap_common.c
> > +++ b/src/providers/ldap/ldap_common.c
> > @@ -39,892 +39,6 @@
> >   /* a fd the child process would log into */
> >   int ldap_child_debug_fd = -1;
> >
> > -int
> > -sdap_domain_destructor(void *mem)
> > -{
> > struct sdap_domain *dom =
> >        talloc_get_type(mem, struct sdap_domain);
> > DLIST_REMOVE(*(dom->head), dom);
> > return 0;
> >
> > -}
> >
> > -struct sdap_domain *
> > -sdap_domain_get(struct sdap_options *opts,
> >            struct sss_domain_info *dom)
> >
> > -{
> > struct sdap_domain *sditer = NULL;
> > DLIST_FOR_EACH(sditer, opts->sdom) {
> >    if (sditer->dom == dom) {
> >        break;
> >    }
> > }
> > return sditer;
> >
> > -}
> >
> > -errno_t
> > -sdap_domain_add(struct sdap_options *opts,
> >            struct sss_domain_info *dom,
> >            struct sdap_domain **_sdom)
> >
> > -{
> > struct sdap_domain *sdom;
> > sdom = talloc_zero(opts, struct sdap_domain);
> > if (sdom == NULL) {
> >    return ENOMEM;
> > }
> > sdom->dom = dom;
> > sdom->head = &opts->sdom;
> > if (opts->sdom) {
> >    /* Only allow subdomains of the parent domain */
> >    if (dom->parent == NULL ||
> >        dom->parent != opts->sdom->dom) {
> >        DEBUG(SSSDBG_OP_FAILURE, ("Domain %s is not a subdomain of %s\n",
> >              dom->name, opts->sdom->dom->name));
> >        return EINVAL;
> >    }
> > }
> > talloc_set_destructor((TALLOC_CTX *)sdom, sdap_domain_destructor);
> > DLIST_ADD_END(opts->sdom, sdom, struct sdap_domain *);
> > if (_sdom) *_sdom = sdom;
> > return EOK;
> >
> > -}
> >
> > -errno_t
> > -sdap_domain_subdom_add(struct sdap_id_ctx *sdap_id_ctx,
> >                   struct sdap_domain *sdom_list,
> >                   struct sss_domain_info *parent)
> >
> > -{
> > struct sss_domain_info *dom;
> > struct sdap_domain *sdom, *sditer;
> > char *basedn;
> > errno_t ret;
> > for (dom = get_next_domain(parent, true);
> >     dom && IS_SUBDOMAIN(dom); /* if we get back to a parent, stop */
> >     dom = get_next_domain(dom, false)) {
> >    DLIST_FOR_EACH(sditer, sdom_list) {
> >        if (sditer->dom == dom) {
> >            break;
> >        }
> >    }
> >    if (sditer == NULL) {
> >        /* New sdap domain */
> >        DEBUG(SSSDBG_TRACE_FUNC, ("subdomain %s is a new one, will "
> >              "create a new sdap domain object\n", dom->name));
> >        ret = sdap_domain_add(sdap_id_ctx->opts, dom, &sdom);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_OP_FAILURE,
> >                ("Cannot add new sdap domain for domain %s [%d]: %s\n",
> >                parent->name, ret, strerror(ret)));
> >            return ret;
> >        }
> >    } else {
> >        sdom = sditer;
> >    }
> >    /* Convert the domain name into search base */
> >    ret = domain_to_basedn(sdom, sdom->dom->name, &basedn);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_OP_FAILURE,
> >            ("Cannot convert domain name [%s] to base DN [%d]: %s\n",
> >            dom->name, ret, strerror(ret)));
> >        talloc_free(basedn);
> >        return ret;
> >    }
> >    /* Update search bases */
> >    talloc_zfree(sdom->search_bases);
> >    sdom->search_bases = talloc_array(sdom, struct sdap_search_base *, 2);
> >    if (sdom->search_bases == NULL) {
> >        return ret;
> >    }
> >    sdom->search_bases[1] = NULL;
> >    ret = sdap_create_search_base(sdom, basedn, LDAP_SCOPE_SUBTREE, NULL,
> >                                  &sdom->search_bases[0]);
> >    talloc_free(basedn);
> >    if (ret) {
> >        DEBUG(SSSDBG_OP_FAILURE, ("Cannot create new sdap search base\n"));
> >        return ret;
> >    }
> >    sdom->user_search_bases = sdom->search_bases;
> >    sdom->group_search_bases = sdom->search_bases;
> >    sdom->netgroup_search_bases = sdom->search_bases;
> >    sdom->sudo_search_bases = sdom->search_bases;
> >    sdom->service_search_bases = sdom->search_bases;
> >    sdom->autofs_search_bases = sdom->search_bases;
> > }
> > return EOK;
> >
> > -}
> >
> > -void
> > -sdap_domain_remove(struct sdap_options *opts,
> >               struct sss_domain_info *dom)
> >
> > -{
> > struct sdap_domain *sdom;
> > sdom = sdap_domain_get(opts, dom);
> > if (sdom == NULL) return;
> > DLIST_REMOVE(*(sdom->head), sdom);
> >
> > -}
>
> Would it make sense to separate sdap_domain_* functions to an sdap
> module instead?
>
> Moving ldap_get_$provider_options makes sense to me.
> > From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 17 Jul 2013 13:51:37 +0200
> > Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip when unit
> >   testing
>
> I agree with Lukas, I would prefer not to add ifdefs to the code unless
> needed.
> > From d1e5b04f0cc7ece9bf7fccd564626e488d532951 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 26 Mar 2013 15:07:17 +0100
> > Subject: [PATCH 06/12] tests: mock SDAP
>
> Looks good to me.
> > From e64bce82249f1b8be7bd5eb8bd4fd01d7a5fb5dd Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 26 Mar 2013 15:18:21 +0100
> > Subject: [PATCH 07/12] tests: mock sysdb users and groups
>
> I think the approach is fine for unit tests. I don't see us change the
> types of sysdb attributes frequently.
> > From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 17 Jul 2013 13:58:54 +0200
> > Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
> >   tests
>
> Looks good to me.
> > From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Thu, 18 Jul 2013 13:24:26 +0200
> > Subject: [PATCH 09/12] tests: new macro sss_will_return_always
>
> Is this feature available in all cmocka releases (IIRC you wrote it) ?
> > From 65da81b7fe5418f44ec743fc11520c5e68a85435 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Tue, 26 Mar 2013 15:06:44 +0100
> > Subject: [PATCH 10/12] tests: add confdb_path to sss_test_ctx
>
> Looks good to me.
> > From 03e318c1319f8b3545195d7deee3d55793fc941a Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Wed, 17 Jul 2013 13:59:36 +0200
> > Subject: [PATCH 11/12] tests: nested groups unit test
>
> I only scrolled through this one, but in general looks good.
> > From 53ff2f0812b6ae81642513aee693284ad134ba47 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Thu, 18 Jul 2013 14:05:05 +0200
> > Subject: [PATCH 12/12] tests: don't print debug message when test dir does
> >   not exist
>
> Looks good to me.

Rebased version with all issues addressed is attached.

[Pavel Březina]

On 01/31/2014 02:37 PM, Pavel Březina wrote:
> On 11/05/2013 12:22 PM, Jakub Hrozek wrote:
> > On Thu, Jul 18, 2013 at 03:45:35PM +0200, Pavel Březina wrote:
> > > On 07/18/2013 02:38 PM, Pavel Březina wrote:
> > > > Hi,
> > > > this is the first unit test for nested groups. It covers only the most
> > > > basic situation when we are trying to resolve one group with no
> > > > members.
> > > >
> > > > Even though it is only one test, the patch set is quite big. This is
> > > > because it creates the possibility to mock providers related modules.
> > > > Most of the patches are just a preparation for unit testing providers.
> > > >
> > > > Patches 1-5
> > > > Moves the code around to reduce number of dependencies. (E.g. you do
> > > > not
> > > > want to load fail over when you are testing nested groups.)
> > > >
> > > > Patch 6
> > > > Mocks basic SDAP interface.
> > > >
> > > > Patch 7
> > > > Mocks sysdb objects - currently user and rfc2307bis group. You can
> > > > decide what set of attributes the object should posses. For example,
> > > > creating a user requires only basedn and name parameter, to construct
> > > > originalDN and name attributes. The rest is provided by (attrname,
> > > > value) pairs via variadic function.
> > > >
> > > > E.g.:
> > > > mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
> > > >
> > > > get_attr_type() translates the sysdb attribute name to proper data
> > > > type.
> > > > This should be extended as needed.
> > > >
> > > > Patch 8
> > > > Adds provider tests related common object files and cflags in makefile
> > > >
> > > > Patch 9
> > > > New macro sss_will_return_always(fn, value). This can be used to mock
> > > > function data in such way that any call of mock() will return the
> > > > value.
> > > >
> > > > It was just pushed also to cmocka upstream as will_return_always().
> > > >
> > > > Patch 10-11
> > > > Unit test.
> > > >
> > > > Patch 12
> > > > Removes a noisy debug message.
> > > >
> > > > I would like to get this reviewed before I continue with more test
> > > > cases, so the framework is tuned enough.
> > > >
> > > > Off topic:
> > > > I also created new macro called fail_msg, which will make the test fail
> > > > printing a message. I didn't use this macro in the end, but it made its
> > > > way to cmocka upstream.
> > >
> > > Oops, I did some reorganization while writing this mail but sent
> > > patches generated sooner, so the numbers does not match. Reordered
> > > patches are attached.
> >
> > The patches no longer apply (not suprising after such a long time on the
> > list..)
> >
> > I will add some quick comments inline without trying out the patches.
> > > From 9b370ed83ec5c85819781145de92c39977fd19eb Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 16 Jul 2013 11:21:02 +0200
> > > Subject: [PATCH 01/12] sdap: move non async functions from
> > > sdap_async.c to
> > >   sdap_utils.c
> > >
> > > Makefile.am                     |   1 +
> > >   src/providers/ldap/sdap_async.c | 107 --------------------------------
> > >   src/providers/ldap/sdap_utils.c | 131
> > > ++++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 132 insertions(+), 107 deletions(-)
> > >   create mode 100644 src/providers/ldap/sdap_utils.c
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index
> > > 515843420834bbbd0e5b4c86c114294c89afa752..6cfc363cea9eae833a8a4bdff12309286475b89a
> > > 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -1450,6 +1450,7 @@ libsss_ldap_common_la_SOURCES = \
> > >       src/providers/ldap/sdap_reinit.c \
> > >       src/providers/ldap/sdap_dyndns.c \
> > >       src/providers/ldap/sdap_refresh.c \
> > > src/providers/ldap/sdap_utils.c \
> > >  src/providers/ldap/sdap.c
> > > libsss_ldap_common_la_LDFLAGS = \
> > >  -avoid-version
> > >
> > > diff --git a/src/providers/ldap/sdap_async.c
> > > b/src/providers/ldap/sdap_async.c
> > > index
> > > bdd3c3c8b2ed4cc7246c008ddbfe531b82d880bc..6d29c1a566f82c2ce0dead2ca8073ace15afbf56
> > > 100644
> > > --- a/src/providers/ldap/sdap_async.c
> > > +++ b/src/providers/ldap/sdap_async.c
> > > @@ -23,26 +23,8 @@
> > >   #include "util/util.h"
> > >   #include "providers/ldap/sdap_async_private.h"
> > >
> > > -#define REALM_SEPARATOR '@'
> > >   #define REPLY_REALLOC_INCREMENT 10
> > >
> > > -void make_realm_upper_case(const char *upn)
> >
> > This function seems to be called from a single location only. Can we
> > either ditch it in favor of get_uppercase_realm() from usertools.c or
> > make it static?
> > > -{
> > > char *c;
> > > c = strchr(upn, REALM_SEPARATOR);
> > > if (c == NULL) {
> > >    DEBUG(9, ("No realm delimiter found in upn [%s].\n", upn));
> > >    return;
> > > }
> > > while(*(++c) != '\0') {
> > >    c[0] = toupper(*c);
> > > }
> > > return;
> > >
> > > -}
> > > From 4f76600299c31489c98f960d162ab7c1d2a29126 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 17 Jul 2013 13:20:43 +0200
> > > Subject: [PATCH 02/12] sdap: move non async functions from
> > >   sdap_async_connection.c to sdap_utils.c
> >
> > Looks good to me.
> > > From dab6d9221bafe0e18f599b88de10b7cc57a57b8e Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 17 Jul 2013 14:28:26 +0200
> > > Subject: [PATCH 03/12] sdap: move sdap_get_id_specific_filter() to
> > >   sdap_utils.c
> >
> > I think this makes sense.
> >
> > Since we started talking about LDAP provider refactoring on IRC
> > recently, I would like to bring up the ldap_*.c vs. sdap_*.c confusion.
> >
> > In my opinion, we should separate libsss_ldap.la as discussed on #sssd
> > the other day and keep its sources in sdap*.c. Only code specific to the
> > initialization of the LDAP provider should be kept in ldap*.c
> > > From a97267f35a598e7f944cf993b37cd6b43db7a572 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 17 Jul 2013 13:50:34 +0200
> > > Subject: [PATCH 04/12] ldap: move options related content from
> > > ldap_common.c
> > >   to ldap_options.c
> > > src/providers/ldap/ldap_common.c                   | 886
> > > .../ldap/{ldap_common.c => ldap_options.c}         | 921
> > >
> > > 2 files changed, 1807 deletions(-)
> > >   copy src/providers/ldap/{ldap_common.c => ldap_options.c} (53%)
> > >
> > > diff --git a/src/providers/ldap/ldap_common.c
> > > b/src/providers/ldap/ldap_common.c
> > > index
> > > 5782c4615ddd3778d16e52986eae065c3c61b8e5..7a8bf93d2366a517e545f1faecdc3aefa2be36e7
> > > 100644
> > > --- a/src/providers/ldap/ldap_common.c
> > > +++ b/src/providers/ldap/ldap_common.c
> > > @@ -39,892 +39,6 @@
> > >   /* a fd the child process would log into */
> > >   int ldap_child_debug_fd = -1;
> > >
> > > -int
> > > -sdap_domain_destructor(void *mem)
> > > -{
> > > struct sdap_domain *dom =
> > >        talloc_get_type(mem, struct sdap_domain);
> > > DLIST_REMOVE(*(dom->head), dom);
> > > return 0;
> > >
> > > -}
> > >
> > > -struct sdap_domain *
> > > -sdap_domain_get(struct sdap_options *opts,
> > >            struct sss_domain_info *dom)
> > >
> > > -{
> > > struct sdap_domain *sditer = NULL;
> > > DLIST_FOR_EACH(sditer, opts->sdom) {
> > >    if (sditer->dom == dom) {
> > >        break;
> > >    }
> > > }
> > > return sditer;
> > >
> > > -}
> > >
> > > -errno_t
> > > -sdap_domain_add(struct sdap_options *opts,
> > >            struct sss_domain_info *dom,
> > >            struct sdap_domain **_sdom)
> > >
> > > -{
> > > struct sdap_domain *sdom;
> > > sdom = talloc_zero(opts, struct sdap_domain);
> > > if (sdom == NULL) {
> > >    return ENOMEM;
> > > }
> > > sdom->dom = dom;
> > > sdom->head = &opts->sdom;
> > > if (opts->sdom) {
> > >    /* Only allow subdomains of the parent domain */
> > >    if (dom->parent == NULL ||
> > >        dom->parent != opts->sdom->dom) {
> > >        DEBUG(SSSDBG_OP_FAILURE, ("Domain %s is not a subdomain
> > >
> > > of %s\n",
> > >              dom->name, opts->sdom->dom->name));
> > >        return EINVAL;
> > >    }
> > > }
> > > talloc_set_destructor((TALLOC_CTX *)sdom, sdap_domain_destructor);
> > > DLIST_ADD_END(opts->sdom, sdom, struct sdap_domain *);
> > > if (_sdom) *_sdom = sdom;
> > > return EOK;
> > >
> > > -}
> > >
> > > -errno_t
> > > -sdap_domain_subdom_add(struct sdap_id_ctx *sdap_id_ctx,
> > >                   struct sdap_domain *sdom_list,
> > >                   struct sss_domain_info *parent)
> > >
> > > -{
> > > struct sss_domain_info *dom;
> > > struct sdap_domain *sdom, *sditer;
> > > char *basedn;
> > > errno_t ret;
> > > for (dom = get_next_domain(parent, true);
> > >     dom && IS_SUBDOMAIN(dom); /* if we get back to a parent,
> > >
> > > stop */
> > >     dom = get_next_domain(dom, false)) {
> > >    DLIST_FOR_EACH(sditer, sdom_list) {
> > >        if (sditer->dom == dom) {
> > >            break;
> > >        }
> > >    }
> > >    if (sditer == NULL) {
> > >        /* New sdap domain */
> > >        DEBUG(SSSDBG_TRACE_FUNC, ("subdomain %s is a new one,
> > >
> > > will "
> > >              "create a new sdap domain object\n", dom->name));
> > >        ret = sdap_domain_add(sdap_id_ctx->opts, dom, &sdom);
> > >        if (ret != EOK) {
> > >            DEBUG(SSSDBG_OP_FAILURE,
> > >                ("Cannot add new sdap domain for domain %s [%d]:
> > >
> > > %s\n",
> > >                parent->name, ret, strerror(ret)));
> > >            return ret;
> > >        }
> > >    } else {
> > >        sdom = sditer;
> > >    }
> > >    /* Convert the domain name into search base */
> > >    ret = domain_to_basedn(sdom, sdom->dom->name, &basedn);
> > >    if (ret != EOK) {
> > >        DEBUG(SSSDBG_OP_FAILURE,
> > >            ("Cannot convert domain name [%s] to base DN [%d]:
> > >
> > > %s\n",
> > >            dom->name, ret, strerror(ret)));
> > >        talloc_free(basedn);
> > >        return ret;
> > >    }
> > >    /* Update search bases */
> > >    talloc_zfree(sdom->search_bases);
> > >    sdom->search_bases = talloc_array(sdom, struct
> > >
> > > sdap_search_base *, 2);
> > >    if (sdom->search_bases == NULL) {
> > >        return ret;
> > >    }
> > >    sdom->search_bases[1] = NULL;
> > >    ret = sdap_create_search_base(sdom, basedn,
> > >
> > > LDAP_SCOPE_SUBTREE, NULL,
> > >                                  &sdom->search_bases[0]);
> > >    talloc_free(basedn);
> > >    if (ret) {
> > >        DEBUG(SSSDBG_OP_FAILURE, ("Cannot create new sdap search
> > >
> > > base\n"));
> > >        return ret;
> > >    }
> > >    sdom->user_search_bases = sdom->search_bases;
> > >    sdom->group_search_bases = sdom->search_bases;
> > >    sdom->netgroup_search_bases = sdom->search_bases;
> > >    sdom->sudo_search_bases = sdom->search_bases;
> > >    sdom->service_search_bases = sdom->search_bases;
> > >    sdom->autofs_search_bases = sdom->search_bases;
> > > }
> > > return EOK;
> > >
> > > -}
> > >
> > > -void
> > > -sdap_domain_remove(struct sdap_options *opts,
> > >               struct sss_domain_info *dom)
> > >
> > > -{
> > > struct sdap_domain *sdom;
> > > sdom = sdap_domain_get(opts, dom);
> > > if (sdom == NULL) return;
> > > DLIST_REMOVE(*(sdom->head), sdom);
> > >
> > > -}
> >
> > Would it make sense to separate sdap_domain_* functions to an sdap
> > module instead?
> >
> > Moving ldap_get_$provider_options makes sense to me.
> > > From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 17 Jul 2013 13:51:37 +0200
> > > Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip
> > > when unit
> > >   testing
> >
> > I agree with Lukas, I would prefer not to add ifdefs to the code unless
> > needed.
> > > From d1e5b04f0cc7ece9bf7fccd564626e488d532951 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 26 Mar 2013 15:07:17 +0100
> > > Subject: [PATCH 06/12] tests: mock SDAP
> >
> > Looks good to me.
> > > From e64bce82249f1b8be7bd5eb8bd4fd01d7a5fb5dd Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 26 Mar 2013 15:18:21 +0100
> > > Subject: [PATCH 07/12] tests: mock sysdb users and groups
> >
> > I think the approach is fine for unit tests. I don't see us change the
> > types of sysdb attributes frequently.
> > > From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 17 Jul 2013 13:58:54 +0200
> > > Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
> > >   tests
> >
> > Looks good to me.
> > > From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Thu, 18 Jul 2013 13:24:26 +0200
> > > Subject: [PATCH 09/12] tests: new macro sss_will_return_always
> >
> > Is this feature available in all cmocka releases (IIRC you wrote it) ?
> > > From 65da81b7fe5418f44ec743fc11520c5e68a85435 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Tue, 26 Mar 2013 15:06:44 +0100
> > > Subject: [PATCH 10/12] tests: add confdb_path to sss_test_ctx
> >
> > Looks good to me.
> > > From 03e318c1319f8b3545195d7deee3d55793fc941a Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Wed, 17 Jul 2013 13:59:36 +0200
> > > Subject: [PATCH 11/12] tests: nested groups unit test
> >
> > I only scrolled through this one, but in general looks good.
> > > From 53ff2f0812b6ae81642513aee693284ad134ba47 Mon Sep 17 00:00:00 2001
> > > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > > Date: Thu, 18 Jul 2013 14:05:05 +0200
> > > Subject: [PATCH 12/12] tests: don't print debug message when test dir
> > > does
> > >   not exist
> >
> > Looks good to me.
>
> Rebased version with all issues addressed is attached.

Rebased again atop Nikolai's DEBUG patches. I hope it is the last time :-)

[Jakub Hrozek]

On Tue, Feb 18, 2014 at 01:46:14PM +0100, Pavel Březina wrote:
> Rebased again atop Nikolai's DEBUG patches. I hope it is the last time :-)

[PATCH 01/13] sdap: move non async functions from sdap_async.c to sdap_utils.c
ACK

[PATCH 02/13] sdap: move non async functions from sdap_async_connection.c to sdap_utils.c
ACK, just can we napespace deref_string_to_val in the future?

[PATCH 03/13] sdap: move sdap_get_id_specific_filter() to sdap_utils.c
ACK

[PATCH 04/13] ldap: move options related content from ldap_common.c to ldap_options.c
ACK

[PATCH 05/13] ldap: move domain related content from ldap_common.c to sdap_domain.c
ACK

[PATCH 06/13] make make_realm_upper_case() static
ACK

[PATCH 07/13] tests: add confdb_path to sss_test_ctx
ACK

[PATCH 08/13] tests: mock SDAP
Shouldn't the file be called mock_sdap_async.c given that most if
not all functions the module mocks come from sdap_async.c ? But ACK to
the content of the files.

You need to add common_mock_sdap.h to noinst_HEADERS to avoid breaking
distcheck.

[PATCH 09/13] tests: mock sysdb users and groups
I think the shortcut is acceptable in tests as it makes the unit test
itself easier to read. Also the ldb values are ultimately just strings, 
so I don't think the danger here is too big.

You need to add common_mock_sysdb_objects.h to noinst_HEADERS to
avoid breaking distcheck.

mock_sysdb_user() is unused, do you plan on using it in future?

[PATCH 10/13] tests: prepare makefile for provider related unit
ACK. I just verified that SSSD builds fine when this patch is the tip of
the tree to make sure we don't break bisect.

[PATCH 11/13] tests: new macro sss_will_return_always
ACK, same simple test as above

[PATCH 12/13] tests: nested groups unit test
ACK. Do you plan on adding more tests in the future with different group
nestings etc? Then even mock_sysdb_user could be useful..

[PATCH 13/13] tests: don't print debug message when test dir does
ACK

[Pavel Březina]

On 02/18/2014 04:14 PM, Jakub Hrozek wrote:
> On Tue, Feb 18, 2014 at 01:46:14PM +0100, Pavel Březina wrote:
> > Rebased again atop Nikolai's DEBUG patches. I hope it is the last time :-)
>
> [PATCH 01/13] sdap: move non async functions from sdap_async.c to sdap_utils.c
> ACK
>
> [PATCH 02/13] sdap: move non async functions from sdap_async_connection.c to sdap_utils.c
> ACK, just can we napespace deref_string_to_val in the future?
>
> [PATCH 03/13] sdap: move sdap_get_id_specific_filter() to sdap_utils.c
> ACK
>
> [PATCH 04/13] ldap: move options related content from ldap_common.c to ldap_options.c
> ACK
>
> [PATCH 05/13] ldap: move domain related content from ldap_common.c to sdap_domain.c
> ACK
>
> [PATCH 06/13] make make_realm_upper_case() static
> ACK
>
> [PATCH 07/13] tests: add confdb_path to sss_test_ctx
> ACK
>
> [PATCH 08/13] tests: mock SDAP
> Shouldn't the file be called mock_sdap_async.c given that most if
> not all functions the module mocks come from sdap_async.c ? But ACK to
> the content of the files.

It contains everything that is necessary to test sdap modules - all 
asynchronous functions (at this moment only those that are needed for 
this test), options, sdap handle. So I think current name is more 
describing, but I don't really oppose. Tell me if you want me to rename it.
> You need to add common_mock_sdap.h to noinst_HEADERS to avoid breaking
> distcheck.

Done.
> [PATCH 09/13] tests: mock sysdb users and groups
> I think the shortcut is acceptable in tests as it makes the unit test
> itself easier to read. Also the ldb values are ultimately just strings,
> so I don't think the danger here is too big.
>
> You need to add common_mock_sysdb_objects.h to noinst_HEADERS to
> avoid breaking distcheck.
>
> mock_sysdb_user() is unused, do you plan on using it in future?

Yes, it will be used in other nested groups tests.
> [PATCH 10/13] tests: prepare makefile for provider related unit
> ACK. I just verified that SSSD builds fine when this patch is the tip of
> the tree to make sure we don't break bisect.
>
> [PATCH 11/13] tests: new macro sss_will_return_always
> ACK, same simple test as above
>
> [PATCH 12/13] tests: nested groups unit test
> ACK. Do you plan on adding more tests in the future with different group
> nestings etc? Then even mock_sysdb_user could be useful..

Of course. As it is mentioned in the first e-mail, I wanted to make sure 
that we agree on this framework before I spend more time on writing tests.
> [PATCH 13/13] tests: don't print debug message when test dir does
> ACK

Thank you for the review.

[Jakub Hrozek]

On Tue, Feb 18, 2014 at 04:55:54PM +0100, Pavel Březina wrote:
> > [PATCH 01/13] sdap: move non async functions from sdap_async.c to sdap_utils.c
> > ACK
> >
> > [PATCH 02/13] sdap: move non async functions from sdap_async_connection.c to sdap_utils.c
> > ACK, just can we napespace deref_string_to_val in the future?
> >
> > [PATCH 03/13] sdap: move sdap_get_id_specific_filter() to sdap_utils.c
> > ACK
> >
> > [PATCH 04/13] ldap: move options related content from ldap_common.c to ldap_options.c
> > ACK
> >
> > [PATCH 05/13] ldap: move domain related content from ldap_common.c to sdap_domain.c
> > ACK
> >
> > [PATCH 06/13] make make_realm_upper_case() static
> > ACK
> >
> > [PATCH 07/13] tests: add confdb_path to sss_test_ctx
> > ACK
> >
> > [PATCH 08/13] tests: mock SDAP
> > Shouldn't the file be called mock_sdap_async.c given that most if
> > not all functions the module mocks come from sdap_async.c ? But ACK to
> > the content of the files.
>
> It contains everything that is necessary to test sdap modules - all
> asynchronous functions (at this moment only those that are needed
> for this test), options, sdap handle. So I think current name is
> more describing, but I don't really oppose. Tell me if you want me
> to rename it.

Fair enough, the name is not that important after all, we can change it
later if needed.
> > You need to add common_mock_sdap.h to noinst_HEADERS to avoid breaking
> > distcheck.
>
> Done.

ACK
> > [PATCH 09/13] tests: mock sysdb users and groups
> > I think the shortcut is acceptable in tests as it makes the unit test
> > itself easier to read. Also the ldb values are ultimately just strings,
> > so I don't think the danger here is too big.
> >
> > You need to add common_mock_sysdb_objects.h to noinst_HEADERS to
> > avoid breaking distcheck.
> >
> > mock_sysdb_user() is unused, do you plan on using it in future?
>
> Yes, it will be used in other nested groups tests.

OK, let's keep it, then,

ACK
> > [PATCH 10/13] tests: prepare makefile for provider related unit
> > ACK. I just verified that SSSD builds fine when this patch is the tip of
> > the tree to make sure we don't break bisect.
> >
> > [PATCH 11/13] tests: new macro sss_will_return_always
> > ACK, same simple test as above
> >
> > [PATCH 12/13] tests: nested groups unit test
> > ACK. Do you plan on adding more tests in the future with different group
> > nestings etc? Then even mock_sysdb_user could be useful..
>
> Of course. As it is mentioned in the first e-mail, I wanted to make
> sure that we agree on this framework before I spend more time on
> writing tests.

Great. I admit I don't know what was in the first e-mail anymore since
the patches were idling on the list for so long.

ACK
> > [PATCH 13/13] tests: don't print debug message when test dir does
> > ACK
>
> Thank you for the review.

All patches look good now. I'll push the whole batch to master. Thank
you for the patience and the time you took to write unit tests.

[Jakub Hrozek]

On Tue, Feb 18, 2014 at 10:49:49PM +0100, Jakub Hrozek wrote:
> All patches look good now. I'll push the whole batch to master. Thank
> you for the patience and the time you took to write unit tests.

Pushed to master.

[Pavel Březina]

On 11/05/2013 12:22 PM, Jakub Hrozek wrote:
> > From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> > Date: Thu, 18 Jul 2013 13:24:26 +0200
> > Subject: [PATCH 09/12] tests: new macro sss_will_return_always
>
> Is this feature available in all cmocka releases (IIRC you wrote it) ?

It is present since cmocka 0.3.2.