On (25/06/14 15:57), Jakub Hrozek wrote:
On Tue, Jun 24, 2014 at 10:49:34AM +0200, Lukas Slebodnik wrote:
> On (23/06/14 19:36), Jakub Hrozek wrote:
> >Hi,
> >
> >When I was reviewing PavelB's duplicate members patch, I realized our
> >nested group unit tests could use some improvements. Please see the
> >attached patches and their commit messages.
>
> >Subject: [PATCH 3/6] TESTS: Fix group search base
> >
> >After fixing the confdb initialization I realized the group DN couldn't
> >be parsed. This patch fixes that.
> >---
> > src/tests/cmocka/test_nested_groups.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/src/tests/cmocka/test_nested_groups.c
b/src/tests/cmocka/test_nested_groups.c
> >index
9f38e3dd388dcafcdaa61cb39136c30fa58b399a..bf678c2b2025e54e6ed50b8b33fe41a78ffec007 100644
> >--- a/src/tests/cmocka/test_nested_groups.c
> >+++ b/src/tests/cmocka/test_nested_groups.c
> >@@ -43,7 +43,7 @@
> > /* put users and groups under the same container so we can easily run the
> > * same tests cases for several search base scenarios */
> > #define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
> >-#define GROUP_BASE_DN "cn=groups" OBJECT_BASE_DN
> >+#define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
> Could you used constant from SYSDB_GROUPS_CONTAINER from sysdb.h?
It wouldn't be the right thing to do, SYSDB_GROUPS_CONTAINER is a cache
base DN, we are simulating the original LDAP DN with this test.
Sorry, My fault.
I should have noticed OBJECT_BASE_DN is not from sysdb.
> >From 3907236536cd4157ab068311b76f0b0babde2681 Mon Sep 17
00:00:00 2001
> >From: Jakub Hrozek <jhrozek(a)redhat.com>
> >Date: Mon, 23 Jun 2014 19:07:59 +0200
> >Subject: [PATCH 6/6] TESTS: Add more tests for nested groups processing
> >
> >Adds unit test for basic group retrieval functionality as well as for
> >testing duplicate members in the LDAP group entry.
> Could you add reference (git hash) to patch with PB's fix?
OK.
Thank you
>
> >---
> > src/tests/cmocka/test_nested_groups.c | 130 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 129 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/tests/cmocka/test_nested_groups.c
b/src/tests/cmocka/test_nested_groups.c
> >index
bf678c2b2025e54e6ed50b8b33fe41a78ffec007..693d0fb33837643566d7a9ab0dc02bd5cff9c849 100644
> >--- a/src/tests/cmocka/test_nested_groups.c
> >+++ b/src/tests/cmocka/test_nested_groups.c
> >@@ -44,6 +44,7 @@
> > * same tests cases for several search base scenarios */
> > #define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
> > #define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
> >+#define USER_BASE_DN "cn=users," OBJECT_BASE_DN
> Could you used constant from SYSDB_USERS_CONTAINER from sysdb.h?
See above.
> >+ /* check return code */
> >+ assert_int_equal(ret, ERR_OK);
> ^^^^^^
> >+
> >+ /* Check the users */
> >+ assert_int_equal(test_ctx->num_users, 1);
> >+ assert_int_equal(test_ctx->num_groups, 1);
> >+
> >+ ret = sysdb_attrs_get_string(test_ctx->users[0], SYSDB_NAME, &name);
> >+ assert_int_equal(ret, EOK);
> ^^^
> You mixed EOK and ERR_OK. According to git history, ERR_OK is newer.
> We should decide and use just one in next patches.
OK, converted to ERR_OK to maintain the same code in the test.
>
>
>
> >+ assert_string_equal(name, "user1");
> >+}
> >+
> > void nested_groups_test_setup(void **state)
> > {
> > struct nested_groups_test_ctx *test_ctx = NULL;
> > static struct sss_test_conf_param params[] = {
> > { "ldap_schema", "rfc2307bis" }, /* enable nested
groups */
> >+ { "ldap_search_base", OBJECT_BASE_DN },
> >+ { "ldap_user_search_base", USER_BASE_DN },
> >+ { "ldap_group_search_base", GROUP_BASE_DN },
> > { NULL, NULL }
> > };
> >
> >@@ -163,7 +289,9 @@ int main(int argc, const char *argv[])
> > };
> >
> > const UnitTest tests[] = {
> >- new_test(one_group_no_members)
> >+ new_test(one_group_no_members),
> >+ new_test(one_group_unique_members),
> >+ new_test(one_group_dup_users)
> > };
> >
> > /* Set debug level to invalid value so we can deside if -d 0 was used. */
>
> I tested with reverted aptch "nested groups: do not fail if we get one entry
> twice" and unit test fail. It's good.
>
> PB modified function sdap_nested_group_single_step_process on two places
> 1) in case SDAP_NESTED_GROUP_DN_USER
> 2) in case SDAP_NESTED_GROUP_DN_GROUP
>
> Unit test covers only the first part. Do we want ot also test the 2nd one?
Yes, I added two more test cases.
Both cases are covered now.
Almost ACK.
From 8479a0a88e80c7025b411d6390276b6c97f3488f Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 19:07:59 +0200
Subject: [PATCH 6/6] TESTS: Add more tests for nested groups processing
Adds unit test for basic group retrieval functionality as well as for
testing duplicate members in the LDAP group entry.
These unit tests exercise code added in patch
a47cb2e08e4004179d2a6b5f9a9340200270fbd0
---
src/responder/ifp/ifp_iface_generated.c | 460 ++++++++++++++++----------------
This
file should not be in this patch.
It if a know bug in code generation #2344
src/tests/cmocka/test_nested_groups.c | 266 +++++++++++++++++-
2 files changed, 495 insertions(+), 231 deletions(-)
LS