[SSSD] [PATCH] TEST: recent_valid filter testing
Petr Cech
pcech at redhat.com
Thu Nov 5 16:29:25 UTC 2015
On 11/04/2015 11:11 AM, Jakub Hrozek wrote:
> Hi,
>
> Sorry it took so long to get back to the review. I only have some minor
> comments, see inline..
>
> Because the group patches are more or less equivalent, I'll just comment
> here. If you agree with the comments, please also change the group tests
> and resend in a single set.
>
> Thanks for the tests!
>
>> > From e3dd543eec09f6e4386bfe6f1505538575fe5356 Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Fri, 2 Oct 2015 07:34:08 -0400
>> >Subject: [PATCH 1/3] TEST: Add test_user_by_recent_filter_valid
>> >
>> >Test users_by_filter_valid() was removed in past. We will add two new
>> >tests instead of it. Logic of those tests is connected to RECENT
>> >filter. It returns only records which have been wrote or updated after
>> >filter was created (or another given time).
>> >
>> >users_by_filter_valid() --> user_by_recent_filter_valid()
>> > users_by_recent_filter_valid()
>> >
>> >The first of new tests, user_by_recent_filter_valid(), counts with two
>> >users. One is stored before filter request creation and the second user
>> >is stored after filter request creation. So filter returns only one
>> >user.
>> >
>> >The second of new tests, users_by_recent_filter_valid(), counts with
>> >three users. One is stored before filter request creation and two users
>> >are stored after filter request creation. So filter returns two users.
>> >
>> >This patch adds user_by_recent_filter_valid().
>> >
>> >Resolves:
>> >https://fedorahosted.org/sssd/ticket/2730
>> >---
>> > src/tests/cmocka/test_responder_cache_req.c | 50 +++++++++++++++++++++++++++++
>> > 1 file changed, 50 insertions(+)
>> >
>> >diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
>> >index 744c8f4a8f7aa4e08f82aca5aea003438b5b59da..3379b17f7feea521966d6c8646afd9859a3c5255 100644
>> >--- a/src/tests/cmocka/test_responder_cache_req.c
>> >+++ b/src/tests/cmocka/test_responder_cache_req.c
>> >@@ -1239,6 +1239,53 @@ static void cache_req_user_by_filter_test_done(struct tevent_req *req)
>> > ctx->tctx->done = true;
>> > }
>> >
>> >+void test_user_by_recent_filter_valid(void **state)
>> >+{
>> >+ struct cache_req_test_ctx *test_ctx = NULL;
>> >+ TALLOC_CTX *req_mem_ctx = NULL;
>> >+ struct tevent_req *req = NULL;
>> >+ const char *ldbname = NULL;
>> >+ errno_t ret;
>> >+
>> >+ test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
>> >+ test_ctx->create_user = true;
>> >+
>> >+ ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2, "pwd", 1001, 1001,
>> >+ NULL, NULL, NULL, "cn="TEST_USER_NAME2",dc=test", NULL,
>> >+ NULL, 1000, time(NULL));
>> >+ assert_int_equal(ret, EOK);
>> >+
>> >+ sleep(1);
> The purpose of the sleep() here is just to make sure the entry was
> created in the past, right? Would it be equally safe to create the user
> with timestamp time(NULL)-1 to make the test faster?
>
>> >+
>> >+ req_mem_ctx = talloc_new(test_ctx->tctx);
>> >+ check_leaks_push(req_mem_ctx);
>> >+
>> >+ /* Filters always go to DP */
>> >+ will_return(__wrap_sss_dp_get_account_send, test_ctx);
>> >+ mock_account_recv_simple();
> Can you add a comment that the TEST_USER is created with a DP callback
> here?
>
>> >+
>> >+ req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>> >+ test_ctx->rctx,
>> >+ test_ctx->tctx->dom->name,
>> >+ "test*");
> It would read nicer if we had a constant TEST_USER_PREFIX "test_user" #defined,
> or even TEST_USER_FILTER with the asterist.
>
>> >+ assert_non_null(req);
>> >+
>> >+ tevent_req_set_callback(req, cache_req_user_by_filter_test_done, test_ctx);
>> >+
>> >+ ret = test_ev_loop(test_ctx->tctx);
>> >+ assert_int_equal(ret, ERR_OK);
>> >+ assert_true(check_leaks_pop(req_mem_ctx));
>> >+
>> >+ assert_non_null(test_ctx->result);
>> >+ assert_int_equal(test_ctx->result->count, 1);
>> >+
>> >+ ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
>> >+ SYSDB_NAME, NULL);
>> >+ assert_non_null(ldbname);
>> >+ assert_string_equal(ldbname, TEST_USER_NAME);
>> >+}
>> > From c2e87544dfbc0667e1b935394d697322b34dddeb Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Tue, 27 Oct 2015 03:53:18 -0400
>> >Subject: [PATCH 2/3] TEST: Refactor of test_responder_cache_req.c
>> >
>> >We need little more in background of responder_cache_req tests. There
>> >will be tests which will use three test users. This patch add support
>> >for it.
>> >
>> >Resolves:
>> >https://fedorahosted.org/sssd/ticket/2730
>> >---
>> > src/tests/cmocka/test_responder_cache_req.c | 59 +++++++++++++++++++++--------
>> > 1 file changed, 44 insertions(+), 15 deletions(-)
>> >
>> >diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
>> >index 3379b17f7feea521966d6c8646afd9859a3c5255..a457d5f277314b75cd73b1306995665456df7f9d 100644
>> >--- a/src/tests/cmocka/test_responder_cache_req.c
>> >+++ b/src/tests/cmocka/test_responder_cache_req.c
>> >@@ -39,8 +39,15 @@
>> > #define TEST_GROUP_NAME "test-group"
>> > #define TEST_GROUP_ID 1000
>> >
>> >-#define TEST_USER_NAME2 "test-user2"
>> >-#define TEST_GROUP_NAME2 "test-group2"
>> >+#define TEST_USER_ID2 1001
>> >+#define TEST_USER_NAME2 "test_user2"
>> >+#define TEST_GROUP_NAME2 "test_group2"
>> >+#define TEST_GROUP_ID2 1001
>> >+
>> >+#define TEST_USER_ID3 1002
>> >+#define TEST_USER_NAME3 "test_user3"
>> >+#define TEST_GROUP_NAME3 "test_group3"
>> >+#define TEST_GROUP_ID3 1002
>> >
>> > #define new_single_domain_test(test) \
>> > cmocka_unit_test_setup_teardown(test_ ## test, \
>> >@@ -82,7 +89,8 @@ struct cache_req_test_ctx {
>> > struct sss_domain_info *domain;
>> > char *name;
>> > bool dp_called;
>> >- bool create_user;
>> >+ bool create_user1;
>> >+ bool create_user2;
> With just two booleans this might be OK, but if we end up adding more
> users, then additional booleans might get clunky and maybe something
> like a bitshift where the DP callback would test for a particular bit
> set might be more generic? Can you add a comment so that the
> next person touching the code won't add another boolean?
>
>> > From 99e1932b876186e702fcb20b6d0bdd5c7e07e1ba Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Tue, 27 Oct 2015 04:02:46 -0400
>> >Subject: [PATCH 3/3] TEST: Add test_users_by_recent_filter_valid
> [...]
>
>> >+ assert_non_null(test_ctx->result);
>> >+ assert_int_equal(test_ctx->result->count, 2);
>> >+
>> >+ ldbname1 = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
>> >+ SYSDB_NAME, NULL);
>> >+ assert_non_null(ldbname1);
>> >+
>> >+ ldbname2 = ldb_msg_find_attr_as_string(test_ctx->result->msgs[1],
>> >+ SYSDB_NAME, NULL);
>> >+ assert_non_null(ldbname2);
>> >+
>> >+ assert_string_not_equal(ldbname1, ldbname2);
>> >+ comparison1 = strcmp(ldbname1, TEST_USER_NAME) == 0 ? true : false;
>> >+ comparison2 = strcmp(ldbname1, TEST_USER_NAME2) == 0 ? true : false;
>> >+ comparison3 = strcmp(ldbname2, TEST_USER_NAME) == 0 ? true : false;
>> >+ comparison4 = strcmp(ldbname2, TEST_USER_NAME2) == 0 ? true : false;
>> >+ assert_true((comparison1 || comparison2) && (comparison3 || comparison4));
>> >+}
> Can you extract this part into a common function that would accept a
> ldb_result pointer and two usernames and would make sure they are in the
> ldb_messages? The same pattern is used in the group tests.
>
> (I wonder if that could also be written in a nice and generic way for N
> usernames in a string array, but it's not important at the moment)
Hello Jakub,
I addresed all hints. There are fixed patch set attached.
Regards,
Petr
PS: I am still not sure what was the purpose of
* users_by_filter_multiple_domains_valid
* groups_by_filter_multiple_domains_valid
tests. How I wrote earlier: "I am afraid I misunderstood their purpose.
Because users/groups are set with the same domains." Please, do you
remember their purpose?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TEST-Add-test_user_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4136 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-TEST-Refactor-of-test_responder_cache_req.c.patch
Type: text/x-patch
Size: 2046 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-TEST-Refactor-of-test_responder_cache_req.c.patch
Type: text/x-patch
Size: 5916 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-TEST-Add-test_users_by_recent_filter_valid.patch
Type: text/x-patch
Size: 5744 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-TEST-Add-test_group_by_recent_filter_valid.patch
Type: text/x-patch
Size: 3760 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-TEST-Refactor-of-test_responder_cache_req.c.patch
Type: text/x-patch
Size: 3120 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-TEST-Add-test_groups_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4521 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0013.bin>
More information about the sssd-devel
mailing list