Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Nov 13, 2015 at 01:48:41PM +0100, Jakub Hrozek wrote:
> On Fri, Nov 13, 2015 at 12:45:00PM +0100, Petr Cech wrote:
> > >Otherwise ACK.
> > Thanks.
> > 
> > >
> > >CI is still running..
> > 
> > There is new patch set attached.
> 
> These patches look OK to me code-wise, waiting for CI results..

The CI job is stuck, but the previous revisions passed CI fine and I
don't see a reason why this one should not:

pushed to master:
* 16212bbb2aaa55d0587515e72c0018479ae51be9
* 5928fcbb57b92bfd18ad15aaaf4a5e1ab8dabe61
* fe6dd669d1e8606862879127f92c177bb7fdc1bd
* a6a5a08a357d2adbb653b81bacc602ca3543c4c4
* 6ae53d7b54ec2ece9fb51ed92c097f5ba8f9d849
* c4d4fe1603420fe8f3d256a3a446974699563ff3
* b0e8c1802557645e2ff6a88c54c520b0f0ff9ebb
* da79bee1472a06b89be2df903fb0bd8ce600c610
and sssd-1-13:
* 87a8f263e90264b826a0e968693ad3f285039687
* f4c7c9268b120fd3f70493c52a61a3a9857b3bff
* 56c7cb8682ef7ccf8e2571bc3b394fab2659
* a6c3dba7e685a53502e30c76185500224675d608
* b87363f82d941e76389415af55656ba3785b912d
* 3d135f299e5fa8f4a37756930cc4924896387e26
* b6dd72c59f52fd6879091216e07c056ff72908b7
* 73519a95291066464f29bb2ee060f404b279f838
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Sat, Nov 14, 2015 at 01:27:24PM +0100, Jakub Hrozek wrote:
> On Fri, Nov 13, 2015 at 01:48:41PM +0100, Jakub Hrozek wrote:
> > On Fri, Nov 13, 2015 at 12:45:00PM +0100, Petr Cech wrote:
> > > >Otherwise ACK.
> > > Thanks.
> > > 
> > > >
> > > >CI is still running..
> > > 
> > > There is new patch set attached.
> > 
> > These patches look OK to me code-wise, waiting for CI results..
> 
> The CI job is stuck, but the previous revisions passed CI fine and I
> don't see a reason why this one should not:
> 
> pushed to master:
> * 16212bbb2aaa55d0587515e72c0018479ae51be9
> * 5928fcbb57b92bfd18ad15aaaf4a5e1ab8dabe61
> * fe6dd669d1e8606862879127f92c177bb7fdc1bd
> * a6a5a08a357d2adbb653b81bacc602ca3543c4c4
> * 6ae53d7b54ec2ece9fb51ed92c097f5ba8f9d849
> * c4d4fe1603420fe8f3d256a3a446974699563ff3
> * b0e8c1802557645e2ff6a88c54c520b0f0ff9ebb
> * da79bee1472a06b89be2df903fb0bd8ce600c610
> and sssd-1-13:
> * 87a8f263e90264b826a0e968693ad3f285039687
> * f4c7c9268b120fd3f70493c52a61a3a9857b3bff
> * 56c7cb8682ef7ccf8e2571bc3b394fab2659
> * a6c3dba7e685a53502e30c76185500224675d608
> * b87363f82d941e76389415af55656ba3785b912d
> * 3d135f299e5fa8f4a37756930cc4924896387e26
> * b6dd72c59f52fd6879091216e07c056ff72908b7
> * 73519a95291066464f29bb2ee060f404b279f838

I also filed http://fedorahosted.org/sssd/ticket/2869 about the memory
leak checks.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Nov 13, 2015 at 12:00:36PM +0100, Lukas Slebodnik wrote:
> On (13/11/15 11:32), Jakub Hrozek wrote:
> >On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:
> >> On 11/13/2015 10:30 AM, Petr Cech wrote:
> >> >On 11/13/2015 10:27 AM, Petr Cech wrote:
> >> >>
> >> >>Patches are rebased now. I hope it will be ok now.
> >> >>
> >> >>Petr
> >> >Sorry, now my local CI tests failed... I will rebase it again.
> >> 
> >> Well, now it is right. Local CI tests passed. There has been patch:
> >> 
> >>   "TESTS: Fix warnings -Wshadow":
> >>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
> >>   Refs: sssd-1_13_1-137-gdf9e9a1
> >>   Author: Lukas Slebodnik 
> >>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
> >>   Commit: Jakub Hrozek 
> >>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
> >> 
> >> There is change
> >> # - time_t time)
> >> # + time_t transaction_time)
> >> in static void prepare_user().
> >> My patches were in conflict with it.
> >> 
> >> Regards
> >> 
> >> Petr
> >
> >> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
> >> From: Petr Cech 
> >> Date: Fri, 2 Oct 2015 07:34:08 -0400
> >> Subject: [PATCH 1/8] 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 
> >> 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153
> >>  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)-1);
> >> +assert_int_equal(ret, EOK);
> >> +
> >> +req_mem_ctx = talloc_new(test_ctx->tctx);
> >> +check_leaks_push(req_mem_ctx);
> >
> >I think the last question is whether we want to use this new context or
> >just call check_leaks_push(test_ctx) recursively. I don't really mind
> >too much, both would work for me.
> >
> >Unless someone opposes, I would push the patch as-is.
> >
> I have a different question. (i haven't read patches yet)
> But I can see that check_leaks_push is called after sysdb_store_user.
> 
> I would like to know why.
> because we shout try to check leaks "caused" in this function.

Wouldn't these leaks be caught by leaks checks that are pushed in
setup() and popped in teardown() ?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/13/2015 11:32 AM, Jakub Hrozek wrote:

On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:

>On 11/13/2015 10:30 AM, Petr Cech wrote:

> >On 11/13/2015 10:27 AM, Petr Cech wrote:

> >>
> >>Patches are rebased now. I hope it will be ok now.
> >>
> >>Petr

> >Sorry, now my local CI tests failed... I will rebase it again.

>
>Well, now it is right. Local CI tests passed. There has been patch:
>
>   "TESTS: Fix warnings -Wshadow":
>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
>   Refs: sssd-1_13_1-137-gdf9e9a1
>   Author: Lukas Slebodnik
>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
>   Commit: Jakub Hrozek
>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
>
>There is change
># - time_t time)
># + time_t transaction_time)
>in static void prepare_user().
>My patches were in conflict with it.
>
>Regards
>
>Petr
> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
>From: Petr Cech
>Date: Fri, 2 Oct 2015 07:34:08 -0400
>Subject: [PATCH 1/8] 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 
85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153 
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)-1);
>+assert_int_equal(ret, EOK);
>+
>+req_mem_ctx = talloc_new(test_ctx->tctx);
>+check_leaks_push(req_mem_ctx);

I think the last question is whether we want to use this new context or
just call check_leaks_push(test_ctx) recursively. I don't really mind
too much, both would work for me.

Unless someone opposes, I would push the patch as-is.

OK.




>+
>+/* Filters always go to DP */
>+will_return(__wrap_sss_dp_get_account_send, test_ctx);
>+mock_account_recv_simple();
>+
>+/* User TEST_USER is created with a DP callback. */
>+req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>+test_ctx->rctx,
>+test_ctx->tctx->dom->name,
>+"test*");
>+assert_non_null(req);
> From df9717ca932f95f55b528024829758dd9b2f2f56 Mon Sep 17 00:00:00 2001
>From: Petr Cech
>Date: Wed, 4 Nov 2015 06:50:33 -0500
>Subject: [PATCH 2/8] TEST: Refactor of test_responder_cache_req.c
>
>This patch only defines constant TEST_USER_FILTER. So code will be more


TEST_USER_PREFIX is defined.

Fixed.


The code is fine.



> From ae448cc95f9ab9fbca3aba5107bb964caf8250ec Mon Sep 17 00:00:00 2001
>From: Petr Cech
>Date: Tue, 27 Oct 2015 03:53:18 -0400
>Subject: [PATCH 3/8] 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.

ACK

Thanks.




> From 943d828ec283284269f954b3044292fb491cf5fa Mon Sep 17 00:00:00 2001
>From: Petr 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/13/2015 12:27 PM, Jakub Hrozek wrote:

+req_mem_ctx = talloc_new(test_ctx->tctx);
> >>+check_leaks_push(req_mem_ctx);

> >
> >I think the last question is whether we want to use this new context or
> >just call check_leaks_push(test_ctx) recursively. I don't really mind
> >too much, both would work for me.
> >
> >Unless someone opposes, I would push the patch as-is.
> >

>I have a different question. (i haven't read patches yet)
>But I can see that check_leaks_push is called after sysdb_store_user.
>
>I would like to know why.
>because we shout try to check leaks "caused" in this function.

Wouldn't these leaks be caught by leaks checks that are pushed in
setup() and popped in teardown() ?


I found out that we use only this expression in test code:

req_mem_ctx = talloc_new(global_talloc_context);
check_leaks_push(req_mem_ctx);

So it is possible that I added this check in vain.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Nov 13, 2015 at 08:32:55AM +0100, Petr Cech wrote:
> bump

Hi, patch 003 doesn't apply cleanly for me, can you rebase?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/13/2015 10:09 AM, Jakub Hrozek wrote:

Hi, patch 003 doesn't apply cleanly for me, can you rebase?


Patches are rebased now. I hope it will be ok now.

Petr
>From 3e43417db9b66bdb44d60b5f186156c5ac26ad4b Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Fri, 2 Oct 2015 07:34:08 -0400
Subject: [PATCH 1/8] 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 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153 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)-1);
+assert_int_equal(ret, EOK);
+
+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();
+
+/* User TEST_USER is created with a DP callback. */
+req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
+test_ctx->rctx,
+test_ctx->tctx->dom->name,
+"test*");
+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);
+}
+
+
 void test_users_by_filter_filter_old(void **state)
 {
 struct cache_req_test_ctx *test_ctx = NULL;
@@ -1476,11 +1523,14 @@ int main(int argc, const char *argv[])
 new_multi_domain_test(group_by_id_multiple_domains_found),
 new_multi_domain_test(group_by_id_multiple_domains_notfound),
 
+new_single_domain_test(user_by_recent_filter_valid),
+
 new_single_domain_test(users_by_filter_filter_old),
 new_single_domain_test(users_by_filter_notfound),
 new_multi_domain_test(users_by_filter_multiple_domains_notfound),
 new_single_domain_test(groups_by_filter_notfound),
 new_multi_domain_test(groups_by_filter_multiple_domains_notfound),
+
 };
 
 /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.4.3

>From 94d583476335324c4f4b62e547a74241582f807f Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Wed, 4 Nov 2015 06:50:33 -0500
Subject: [PATCH 2/8] TEST: Refactor of test_responder_cache_req.c

This patch only defines constant TEST_USER_FILTER. So code will be more
redeable.

Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
 src/tests/cmocka/test_responder_cache_req.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
index 14a40ae6e56b2f6d0b18608bac09bc4680245153..5ff6c95681d899e2ae93bd7964b492e52a2d223a 100644
--- 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/13/2015 10:27 AM, Petr Cech wrote:


Patches are rebased now. I hope it will be ok now.

Petr

Sorry, now my local CI tests failed... I will rebase it again.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Lukas Slebodnik]

On (13/11/15 11:32), Jakub Hrozek wrote:
>On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:
>> On 11/13/2015 10:30 AM, Petr Cech wrote:
>> >On 11/13/2015 10:27 AM, Petr Cech wrote:
>> >>
>> >>Patches are rebased now. I hope it will be ok now.
>> >>
>> >>Petr
>> >Sorry, now my local CI tests failed... I will rebase it again.
>> 
>> Well, now it is right. Local CI tests passed. There has been patch:
>> 
>>   "TESTS: Fix warnings -Wshadow":
>>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
>>   Refs: sssd-1_13_1-137-gdf9e9a1
>>   Author: Lukas Slebodnik 
>>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
>>   Commit: Jakub Hrozek 
>>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
>> 
>> There is change
>> # - time_t time)
>> # + time_t transaction_time)
>> in static void prepare_user().
>> My patches were in conflict with it.
>> 
>> Regards
>> 
>> Petr
>
>> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
>> From: Petr Cech 
>> Date: Fri, 2 Oct 2015 07:34:08 -0400
>> Subject: [PATCH 1/8] 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 
>> 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153
>>  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)-1);
>> +assert_int_equal(ret, EOK);
>> +
>> +req_mem_ctx = talloc_new(test_ctx->tctx);
>> +check_leaks_push(req_mem_ctx);
>
>I think the last question is whether we want to use this new context or
>just call check_leaks_push(test_ctx) recursively. I don't really mind
>too much, both would work for me.
>
>Unless someone opposes, I would push the patch as-is.
>
I have a different question. (i haven't read patches yet)
But I can see that check_leaks_push is called after sysdb_store_user.

I would like to know why.
because we shout try to check leaks "caused" in this function.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Nov 13, 2015 at 10:52:08AM +0100, Petr Cech wrote:
> On 11/13/2015 10:30 AM, Petr Cech wrote:
> >On 11/13/2015 10:27 AM, Petr Cech wrote:
> >>
> >>Patches are rebased now. I hope it will be ok now.
> >>
> >>Petr
> >Sorry, now my local CI tests failed... I will rebase it again.
> 
> Well, now it is right. Local CI tests passed. There has been patch:
> 
>   "TESTS: Fix warnings -Wshadow":
>   commit df9e9a1f9b7dc255eb62c390163c25917b08f5a2
>   Refs: sssd-1_13_1-137-gdf9e9a1
>   Author: Lukas Slebodnik 
>   AuthorDate: Mon Nov 9 10:59:55 2015 +0100
>   Commit: Jakub Hrozek 
>   CommitDate: Tue Nov 10 15:34:41 2015 +0100
> 
> There is change
> # - time_t time)
> # + time_t transaction_time)
> in static void prepare_user().
> My patches were in conflict with it.
> 
> Regards
> 
> Petr

> From 3ce6073dda27fd7a4626f5cbac1c765274ca5fe0 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Fri, 2 Oct 2015 07:34:08 -0400
> Subject: [PATCH 1/8] 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 
> 85d986bd7d159dc238bce4bc770272d18288f2dd..14a40ae6e56b2f6d0b18608bac09bc4680245153
>  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)-1);
> +assert_int_equal(ret, EOK);
> +
> +req_mem_ctx = talloc_new(test_ctx->tctx);
> +check_leaks_push(req_mem_ctx);

I think the last question is whether we want to use this new context or
just call check_leaks_push(test_ctx) recursively. I don't really mind
too much, both would work for me.

Unless someone opposes, I would push the patch as-is.

> +
> +/* Filters always go to DP */
> +will_return(__wrap_sss_dp_get_account_send, test_ctx);
> +mock_account_recv_simple();
> +
> +/* User TEST_USER is created with a DP callback. */
> +req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
> +test_ctx->rctx,
> +test_ctx->tctx->dom->name,
> +"test*");
> +assert_non_null(req);

> From df9717ca932f95f55b528024829758dd9b2f2f56 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Wed, 4 Nov 2015 06:50:33 -0500
> Subject: [PATCH 2/8] TEST: Refactor of test_responder_cache_req.c
> 
> This patch only defines constant TEST_USER_FILTER. So code will be more
   
   TEST_USER_PREFIX is defined.

The code is fine.


> From ae448cc95f9ab9fbca3aba5107bb964caf8250ec Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Tue, 27 Oct 2015 03:53:18 -0400
> Subject: [PATCH 3/8] 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.

ACK

> From 943d828ec283284269f954b3044292fb491cf5fa Mon Sep 17 00:00:00 2001

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Nov 13, 2015 at 12:27:03PM +0100, Jakub Hrozek wrote:
> > I have a different question. (i haven't read patches yet)
> > But I can see that check_leaks_push is called after sysdb_store_user.
> > 
> > I would like to know why.
> > because we shout try to check leaks "caused" in this function.
> 
> Wouldn't these leaks be caught by leaks checks that are pushed in
> setup() and popped in teardown() ?

Ah, but we don't use leak checks in setup() and teardown(), sorry!

Then it's something we definitely need to fix -- I think a leak in
requests (like cached_req) is only possible if we use a long-lived
(responder or event) context by mistake.

So I think we should file another ticket and add leak checks to setup and
teardown in this setup. It's not related to Petr's tests, but I think it
should be done before we switch to the cache_req API in NSS to make sure
we don't regress.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Nov 13, 2015 at 12:45:00PM +0100, Petr Cech wrote:
> >Otherwise ACK.
> Thanks.
> 
> >
> >CI is still running..
> 
> There is new patch set attached.

These patches look OK to me code-wise, waiting for CI results..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

bump
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/10/2015 08:29 AM, Pavel Reichl wrote:



On 11/05/2015 05:29 PM, Petr Cech wrote:

+void test_groups_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 **group_names = NULL;
+const char **ldb_results = NULL;
+const char *ldbname = NULL;
+void *tmp_ctx = NULL;

Could you use TALLOC_CTX?

Yes, I could :-)



+errno_t ret;
+
+test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+test_ctx->create_group1 = true;
+test_ctx->create_group2 = true;
+
+ret = sysdb_store_group(test_ctx->tctx->dom, TEST_GROUP_NAME2,
+1001, NULL, 1001, time(NULL));
+assert_int_equal(ret, EOK);
+
+sleep(1);
+
+req_mem_ctx = talloc_new(global_talloc_context);
+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();
+
+/* Group TEST_GROUP1 and TEST_GROUP2 are created with a DP
callback. */
+req = cache_req_group_by_filter_send(req_mem_ctx,
test_ctx->tctx->ev,
+ test_ctx->rctx,
+ test_ctx->tctx->dom->name,
+ TEST_USER_PREFIX);
+assert_non_null(req);
+
+tevent_req_set_callback(req, cache_req_group_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, 2);
+
+tmp_ctx = talloc_zero(NULL, void *);


Why not to use talloc_new(parent_ctx)?


+
+group_names = talloc_array(tmp_ctx, const char *, 2);
+assert_non_null(group_names);
+group_names[0] = TEST_GROUP_NAME;
+group_names[1] = TEST_GROUP_NAME2;
+
+ldb_results = talloc_array(tmp_ctx, const char *, 2);
+assert_non_null(ldb_results);
+for (int i = 0; i < 2; ++i) {
+ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[i],
+  SYSDB_NAME, NULL);
+assert_non_null(ldbname);
+ldb_results[i] = ldbname;
+}
+
+assert_string_not_equal(ldb_results[0], ldb_results[1]);
+
+assert_true(are_values_in_ldb_result(ldb_results, group_names));
+
+talloc_zfree(tmp_ctx);
+}


Thanks!


Your comments will be addressed in nex patchset.

Petr
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/10/2015 08:37 AM, Lukas Slebodnik wrote:

On (10/11/15 08:29), Pavel Reichl wrote:



On 11/05/2015 05:29 PM, Petr Cech wrote:

+void test_groups_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 **group_names = NULL;
+const char **ldb_results = NULL;
+const char *ldbname = NULL;
+void *tmp_ctx = NULL;

Could you use TALLOC_CTX?


Why do we need two different talloc context in a test?
"TALLOC_CTX *req_mem_ctx", "void *tmp_ctx"

If we properly release resources we can use single talloc context.
It's the best way how to catch memory leaks.

LS


Right, I will change void *tmp_ctx to TALLOC_CTX *tmp_ctx and I will 
create it under req_mem_ctx. I feel it will be more clear and readable.


Petr
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 11/09/2015 04:28 PM, Jakub Hrozek wrote:

On Thu, Nov 05, 2015 at 05:29:25PM +0100, Petr Cech wrote:

>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
> >>>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
> >>>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 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Thu, Nov 05, 2015 at 05:29:25PM +0100, Petr Cech wrote:
> 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
> >>>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
> >>>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 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Tue, Nov 10, 2015 at 08:37:13AM +0100, Lukas Slebodnik wrote:
> On (10/11/15 08:29), Pavel Reichl wrote:
> >
> >
> >On 11/05/2015 05:29 PM, Petr Cech wrote:
> >>+void test_groups_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 **group_names = NULL;
> >>+const char **ldb_results = NULL;
> >>+const char *ldbname = NULL;
> >>+void *tmp_ctx = NULL;
> >Could you use TALLOC_CTX?
> >
> Why do we need two different talloc context in a test?
> "TALLOC_CTX *req_mem_ctx", "void *tmp_ctx"
> 
> If we properly release resources we can use single talloc context.
> It's the best way how to catch memory leaks.

As long as we can push right before the _send() and pop right after the
_recv() we probably don't..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Lukas Slebodnik]

On (10/11/15 08:29), Pavel Reichl wrote:
>
>
>On 11/05/2015 05:29 PM, Petr Cech wrote:
>>+void test_groups_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 **group_names = NULL;
>>+const char **ldb_results = NULL;
>>+const char *ldbname = NULL;
>>+void *tmp_ctx = NULL;
>Could you use TALLOC_CTX?
>
Why do we need two different talloc context in a test?
"TALLOC_CTX *req_mem_ctx", "void *tmp_ctx"

If we properly release resources we can use single talloc context.
It's the best way how to catch memory leaks.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Pavel Reichl]

On 11/05/2015 05:29 PM, Petr Cech wrote:

+void test_groups_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 **group_names = NULL;
+const char **ldb_results = NULL;
+const char *ldbname = NULL;
+void *tmp_ctx = NULL;

Could you use TALLOC_CTX?


+errno_t ret;
+
+test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
+test_ctx->create_group1 = true;
+test_ctx->create_group2 = true;
+
+ret = sysdb_store_group(test_ctx->tctx->dom, TEST_GROUP_NAME2,
+1001, NULL, 1001, time(NULL));
+assert_int_equal(ret, EOK);
+
+sleep(1);
+
+req_mem_ctx = talloc_new(global_talloc_context);
+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();
+
+/* Group TEST_GROUP1 and TEST_GROUP2 are created with a DP callback. */
+req = cache_req_group_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
+ test_ctx->rctx,
+ test_ctx->tctx->dom->name,
+ TEST_USER_PREFIX);
+assert_non_null(req);
+
+tevent_req_set_callback(req, cache_req_group_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, 2);
+
+tmp_ctx = talloc_zero(NULL, void *);


Why not to use talloc_new(parent_ctx)?


+
+group_names = talloc_array(tmp_ctx, const char *, 2);
+assert_non_null(group_names);
+group_names[0] = TEST_GROUP_NAME;
+group_names[1] = TEST_GROUP_NAME2;
+
+ldb_results = talloc_array(tmp_ctx, const char *, 2);
+assert_non_null(ldb_results);
+for (int i = 0; i < 2; ++i) {
+ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[i],
+  SYSDB_NAME, NULL);
+assert_non_null(ldbname);
+ldb_results[i] = ldbname;
+}
+
+assert_string_not_equal(ldb_results[0], ldb_results[1]);
+
+assert_true(are_values_in_ldb_result(ldb_results, group_names));
+
+talloc_zfree(tmp_ctx);
+}


Thanks!
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

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
>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
>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
>+++ 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Tue, Oct 27, 2015 at 09:42:20AM +0100, Petr Cech wrote:
> On 10/23/2015 12:57 PM, Jakub Hrozek wrote:
> >Thank you, I think your approach is correct. Your test essentially tests
> >that testuser2 was on the server but was removed, so only testuser1 is
> >returned.
> >
> >It's correct, but because the interface is able to return more users, I
> >would prefer if we tested that as well.
> >
> >I have one more minor remark inline, but in general, please go
> >ahead and add back the other tests..
> 
> Hello Jakub and everyone!
> 
> The first patch set is attached.
> 
> The removed tests were:
>  * users_by_filter_valid
>  * users_by_filter_multiple_domains_valid
>  * groups_by_filter_valid
>  * groups_by_filter_multiple_domains_valid
> 
> This patch set covers users_by_filter_valid by two new tests:
>  * user_by_recent_filter_valid
>  * users_by_recent_filter_valid
> 
> The first of them tests the recent filter. The seconds tests interface
> ability to return more users.
> 
> Regards,
> 
> Petr

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 
> 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 = 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 10/27/2015 09:42 AM, Petr Cech wrote:

The removed tests were:
  * users_by_filter_valid
  * users_by_filter_multiple_domains_valid
  * groups_by_filter_valid
  * groups_by_filter_multiple_domains_valid


Hello,

another patch set is attached.

This patch set covers groups_by_filter_valid by two new tests:
 * group_by_recent_filter_valid
 * groups_by_recent_filter_valid

The first of them tests the recent filter. The second tests interface 
ability to return more groups.



I looked at multiple_domains tests too. But I am afraid I misunderstood 
their purpose. Because users/groups are set with the same domains. I 
will look at it once again.


Regards,

Petr
>From 9c2cf658b62734df71650b568bd1c6be6c4c6e43 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Sun, 1 Nov 2015 07:09:28 -0500
Subject: [PATCH 4/6] TEST: Add test_group_by_recent_filter_valid

Test groups_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).

groups_by_filter_valid() --> group_by_recent_filter_valid()
 grous_by_recent_filter_valid()

The first of new tests, group_by_recent_filter_valid(), counts with two
groups. One is stored before filter request creation and the second
group is stored after filter request creation. So filter returns only
one group.

The second of new tests, groups_by_recent_filter_valid(), counts with
three users. One is stored before filter request creation and two
groups are stored after filter request creation. So filter returns two
groups.

This patch adds group_by_recent_filter_valid().

Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
 src/tests/cmocka/test_responder_cache_req.c | 45 +
 1 file changed, 45 insertions(+)

diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
index e4fccdab883f267cced1cf2e9995bd9828242690..77bdde40917b576b2b97d92d9dc23900085a11ae 100644
--- a/src/tests/cmocka/test_responder_cache_req.c
+++ b/src/tests/cmocka/test_responder_cache_req.c
@@ -1495,6 +1495,50 @@ static void cache_req_group_by_filter_test_done(struct tevent_req *req)
 ctx->tctx->done = true;
 }
 
+void test_group_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_group = true;
+
+ret = sysdb_store_group(test_ctx->tctx->dom, TEST_GROUP_NAME2,
+1001, NULL, 1001, time(NULL));
+assert_int_equal(ret, EOK);
+
+sleep(1);
+
+req_mem_ctx = talloc_new(global_talloc_context);
+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();
+
+req = cache_req_group_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
+ test_ctx->rctx,
+ test_ctx->tctx->dom->name,
+ "test*");
+assert_non_null(req);
+tevent_req_set_callback(req, cache_req_group_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_GROUP_NAME);
+}
+
 void test_groups_by_filter_notfound(void **state)
 {
 struct cache_req_test_ctx *test_ctx = NULL;
@@ -1615,6 +1659,7 @@ int main(int argc, const char *argv[])
 
 new_single_domain_test(user_by_recent_filter_valid),
 new_single_domain_test(users_by_recent_filter_valid),
+new_single_domain_test(group_by_recent_filter_valid),
 
 new_single_domain_test(users_by_filter_filter_old),
 new_single_domain_test(users_by_filter_notfound),
-- 
2.4.3

>From 4efa3966f20791d65c439ff450b473c6d9419eff Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Sun, 1 Nov 2015 07:21:18 -0500
Subject: [PATCH 5/6] TEST: Refactor of test_responder_cache_req.c

We need little more in backroung of responder_cache_req tests. There
will be tests which will use three test groups. This patch add support
for it.

Resolves:
https://fedorahosted.org/sssd/ticket/2730
---
 src/tests/cmocka/test_responder_cache_req.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/tests/cmocka/test_responder_cache_req.c 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

On 10/23/2015 12:57 PM, Jakub Hrozek wrote:

Thank you, I think your approach is correct. Your test essentially tests
that testuser2 was on the server but was removed, so only testuser1 is
returned.

It's correct, but because the interface is able to return more users, I
would prefer if we tested that as well.

I have one more minor remark inline, but in general, please go
ahead and add back the other tests..


Hello Jakub and everyone!

The first patch set is attached.

The removed tests were:
 * users_by_filter_valid
 * users_by_filter_multiple_domains_valid
 * groups_by_filter_valid
 * groups_by_filter_multiple_domains_valid

This patch set covers users_by_filter_valid by two new tests:
 * user_by_recent_filter_valid
 * users_by_recent_filter_valid

The first of them tests the recent filter. The seconds tests interface 
ability to return more users.


Regards,

Petr
>From e3dd543eec09f6e4386bfe6f1505538575fe5356 Mon Sep 17 00:00:00 2001
From: Petr Cech 
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);
+
+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();
+
+req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
+test_ctx->rctx,
+test_ctx->tctx->dom->name,
+"test*");
+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);
+}
+
+
 void test_users_by_filter_filter_old(void **state)
 {
 struct cache_req_test_ctx *test_ctx = NULL;
@@ -1476,11 +1523,14 @@ int main(int argc, const char *argv[])
 new_multi_domain_test(group_by_id_multiple_domains_found),
 new_multi_domain_test(group_by_id_multiple_domains_notfound),
 
+new_single_domain_test(user_by_recent_filter_valid),
+
 new_single_domain_test(users_by_filter_filter_old),
 new_single_domain_test(users_by_filter_notfound),
 new_multi_domain_test(users_by_filter_multiple_domains_notfound),
 new_single_domain_test(groups_by_filter_notfound),
 new_multi_domain_test(groups_by_filter_multiple_domains_notfound),
+
 };
 
 /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
2.4.3

>From 

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Jakub Hrozek]

On Fri, Oct 02, 2015 at 01:55:29PM +0200, Petr Cech wrote:
> Hi,
> 
> there is WiP attached. I removed some tests like this one some time ago.
> They fail really often and we decided that the test logic was corrupted. Now
> I am trying get it back to the codebase.
> 
> There is some kind of cmocka magic around data provider. I think it creates
> test_user_1 during creation of filter.
> 
> In case of this type of tests, we need two users, one stored before filter
> request and one stored after filter request. There is a special type of
> filter which has time parameter which it search from. So the filter returns
> only one user.
> 
> If this concept is right, I will send whole patch.
> 
> Regards
> 
> Petr
> 
> PS: I applied my patch after 000*-cache_req_*. Those patches are on list.

> From aa0b0ab7c0a95ff47d5003907730c5432ff7bb85 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Fri, 2 Oct 2015 07:34:08 -0400
> Subject: [PATCH] TEST: recent_valid filter testing
> 
> Some tests were removed in past. This is only WiP, not regular patch.
> I rewrote one of the removed test. Is it this right way?
> 
> We speak about RECENT filter. It returns only records which
> have been wrote or updated after filter was created (or another given
> time). Some notes are written in comments of this patch.

Thank you, I think your approach is correct. Your test essentially tests
that testuser2 was on the server but was removed, so only testuser1 is
returned.

It's correct, but because the interface is able to return more users, I
would prefer if we tested that as well.

I have one more minor remark inline, but in general, please go
ahead and add back the other tests..

> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2730
> ---
>  src/tests/cmocka/test_responder_cache_req.c | 60 
> -
>  1 file changed, 58 insertions(+), 2 deletions(-)
> 
> diff --git a/src/tests/cmocka/test_responder_cache_req.c 
> b/src/tests/cmocka/test_responder_cache_req.c
> index 
> bb79fd10eefd7186f17a1f9306b57ddca2e3279f..c01d92fd9f3f078d853da1642e63cdbc3a1aed7b
>  100644
> --- a/src/tests/cmocka/test_responder_cache_req.c
> +++ b/src/tests/cmocka/test_responder_cache_req.c
> @@ -1239,6 +1239,58 @@ static void cache_req_user_by_filter_test_done(struct 
> tevent_req *req)
>  ctx->tctx->done = true;
>  }
>  
> +/* NOTE better name is filter_recent_valid */
> +void test_users_by_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;
> +
> +/* NOTE This user (#2) is stored before filter creation. */
> +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);
> +
> +/* NOTE To make sure that the times of user/filter creation will vary.*/
> +sleep(1);
> +
> +req_mem_ctx = talloc_new(global_talloc_context);

Please either allocate the context on test_ctx or just use test_ctx..

> +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();
> +
> +/* NOTE During this call the TEST_USER_NAME (#1) will be stored. */
> +req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
> +test_ctx->rctx,
> +test_ctx->tctx->dom->name,
> +"test*");
> +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));
> +
> +/* NOTE We receive only user #1, because #2 was stored before filter was 
> created. */
> +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);
> +}
> +
> +
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

ping
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] TEST: recent_valid filter testing

[Petr Cech]

There is previous commit to this ticket, so you can see what tests were 
removed.


https://git.fedorahosted.org/cgit/sssd.git/commit/?id=bdf422fde0fd6b40b3412bad3b200f8fd7ea8693
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel