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.
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.
From bc8fd57de30abad8c2b5e74def22927bb2870830 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 17:50:45 +0200 Subject: [PATCH 1/6] TESTS: Add confdb domain base DN to sss_test_ctx
Creation of the path to the domain's confdb entry was duplicated in the tests. Rather than adding yet another duplication, I added the path as another field of the sss_test_ctx structure.
From patch, it is clean that it was not used on many places.
It will be usefull in future.
ACK
From 532cee578213b3d0d9d13229c72adefeb126f8cf Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 17:52:03 +0200 Subject: [PATCH 2/6] TESTS: Use the right confdb path
The nested group test only worked by accident. Its confdb settings were not applied because a wrong confdb path was used.
ACK
From 09ca16557b3eb556f38a959d1749160c42a33578 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 18:07:51 +0200 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?
From 0cd01ebe4f7f61e830cb06c907b04ff07200dbd1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 19:06:52 +0200 Subject: [PATCH 4/6] TESTS: Do not require replies from mocked sdap_get_generic_recv to be talloc contexts
While it's beneficial for the real implementation of sdap_get_generic_recv() to move memory around, the mocked implementation can just pass around pointer.
make sense.
ACK
From 67a524e5ce1b7c48b99104a8726b18a090550ae4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 19:07:41 +0200 Subject: [PATCH 5/6] TESTS: Change how mock_sysdb_user() is implemented
For the purpose of unit tests, it's better to create a user object with a UID and a name.
ACK
From 3907236536cd4157ab068311b76f0b0babde2681 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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?
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?
struct nested_groups_test_ctx { struct sss_test_ctx *tctx; @@ -114,11 +115,136 @@ static void nested_groups_test_one_group_no_members(void **state) assert_true(rootgroup == test_ctx->groups[0]); }
+static void nested_groups_test_one_group_unique_members(void **state) +{
- struct nested_groups_test_ctx *test_ctx = NULL;
- struct sysdb_attrs *rootgroup = NULL;
- struct tevent_req *req = NULL;
- TALLOC_CTX *req_mem_ctx = NULL;
- errno_t ret;
- const char *name;
- const char *users[] = { "cn=user1,"USER_BASE_DN,
"cn=user2,"USER_BASE_DN,
NULL };
- const struct sysdb_attrs *user1_reply[2] = { NULL };
- const struct sysdb_attrs *user2_reply[2] = { NULL };
- test_ctx = talloc_get_type_abort(*state, struct nested_groups_test_ctx);
- /* mock return values */
- rootgroup = mock_sysdb_group_rfc2307bis(test_ctx, GROUP_BASE_DN, 1000,
"rootgroup", users);
- user1_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2001, "user1");
- assert_non_null(user1_reply[0]);
- will_return(sdap_get_generic_recv, 1);
- will_return(sdap_get_generic_recv, user1_reply);
- user2_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2002, "user2");
- assert_non_null(user2_reply[0]);
- will_return(sdap_get_generic_recv, 1);
- will_return(sdap_get_generic_recv, user2_reply);
- sss_will_return_always(sdap_has_deref_support, false);
- /* run test, check for memory leaks */
- req_mem_ctx = talloc_new(global_talloc_context);
- assert_non_null(req_mem_ctx);
- check_leaks_push(req_mem_ctx);
- req = sdap_nested_group_send(req_mem_ctx, test_ctx->tctx->ev,
test_ctx->sdap_domain, test_ctx->sdap_opts,
test_ctx->sdap_handle, rootgroup);
- assert_non_null(req);
- tevent_req_set_callback(req, nested_groups_test_done, test_ctx);
- ret = test_ev_loop(test_ctx->tctx);
- assert_true(check_leaks_pop(req_mem_ctx) == true);
- talloc_zfree(req_mem_ctx);
- /* check return code */
- assert_int_equal(ret, ERR_OK);
- /* Check the users */
- assert_int_equal(test_ctx->num_users, 2);
- 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);
- assert_string_equal(name, "user1");
- ret = sysdb_attrs_get_string(test_ctx->users[1], SYSDB_NAME, &name);
- assert_int_equal(ret, EOK);
- assert_string_equal(name, "user2");
+}
+static void nested_groups_test_one_group_dup_users(void **state) +{
- struct nested_groups_test_ctx *test_ctx = NULL;
- struct sysdb_attrs *rootgroup = NULL;
- struct tevent_req *req = NULL;
- TALLOC_CTX *req_mem_ctx = NULL;
- errno_t ret;
- const char *name;
- const char *users[] = { "cn=user1,"USER_BASE_DN,
"cn=user1,"USER_BASE_DN,
NULL };
- const struct sysdb_attrs *user1_reply[2] = { NULL };
- const struct sysdb_attrs *user2_reply[2] = { NULL };
- test_ctx = talloc_get_type_abort(*state, struct nested_groups_test_ctx);
- /* mock return values */
- rootgroup = mock_sysdb_group_rfc2307bis(test_ctx, GROUP_BASE_DN, 1000,
"rootgroup", users);
- user1_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2001, "user1");
- assert_non_null(user1_reply[0]);
- will_return(sdap_get_generic_recv, 1);
- will_return(sdap_get_generic_recv, user1_reply);
- user2_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2001, "user1");
- assert_non_null(user2_reply[0]);
- will_return(sdap_get_generic_recv, 1);
- will_return(sdap_get_generic_recv, user2_reply);
- sss_will_return_always(sdap_has_deref_support, false);
- /* run test, check for memory leaks */
- req_mem_ctx = talloc_new(global_talloc_context);
- assert_non_null(req_mem_ctx);
- check_leaks_push(req_mem_ctx);
- req = sdap_nested_group_send(req_mem_ctx, test_ctx->tctx->ev,
test_ctx->sdap_domain, test_ctx->sdap_opts,
test_ctx->sdap_handle, rootgroup);
- assert_non_null(req);
- tevent_req_set_callback(req, nested_groups_test_done, test_ctx);
- ret = test_ev_loop(test_ctx->tctx);
- assert_true(check_leaks_pop(req_mem_ctx) == true);
- talloc_zfree(req_mem_ctx);
- /* 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.
- 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?
LS
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.
From bc8fd57de30abad8c2b5e74def22927bb2870830 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 17:50:45 +0200 Subject: [PATCH 1/6] TESTS: Add confdb domain base DN to sss_test_ctx
Creation of the path to the domain's confdb entry was duplicated in the tests. Rather than adding yet another duplication, I added the path as another field of the sss_test_ctx structure.
From patch, it is clean that it was not used on many places. It will be usefull in future.
ACK
From 532cee578213b3d0d9d13229c72adefeb126f8cf Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 17:52:03 +0200 Subject: [PATCH 2/6] TESTS: Use the right confdb path
The nested group test only worked by accident. Its confdb settings were not applied because a wrong confdb path was used.
ACK
From 09ca16557b3eb556f38a959d1749160c42a33578 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 18:07:51 +0200 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.
From 0cd01ebe4f7f61e830cb06c907b04ff07200dbd1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 19:06:52 +0200 Subject: [PATCH 4/6] TESTS: Do not require replies from mocked sdap_get_generic_recv to be talloc contexts
While it's beneficial for the real implementation of sdap_get_generic_recv() to move memory around, the mocked implementation can just pass around pointer.
make sense.
ACK
From 67a524e5ce1b7c48b99104a8726b18a090550ae4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 23 Jun 2014 19:07:41 +0200 Subject: [PATCH 5/6] TESTS: Change how mock_sysdb_user() is implemented
For the purpose of unit tests, it's better to create a user object with a UID and a name.
ACK
From 3907236536cd4157ab068311b76f0b0babde2681 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@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.
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.
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@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@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
On Fri, Jun 27, 2014 at 08:55:02PM +0200, Lukas Slebodnik wrote:
On (27/06/14 18:39), Jakub Hrozek wrote:
On Wed, Jun 25, 2014 at 06:40:26PM +0200, Lukas Slebodnik wrote:
Almost ACK.
Thank you, I did a git commit -am and didn't realize the extra hunk.
new patches are attached.
ACK
LS
master: 6815772cb5c1c65de6c93090f4fd191ee02527f1 b0fb84bb4a98d3086b78550c737ef20f63cbac26 132d6228d981436c5e1ca3ddf486f95ac77e46b6 3b28d617f9af28e002b1d0a63faa90ef9e23e2ef 9339e19aa015a7a32791cb94f92d1656361c6662 919cfb623e9b725eaa654c6e1b513d1305057314
sssd-devel@lists.fedorahosted.org