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<lslebodn(a)redhat.com>
> > AuthorDate: Mon Nov 9 10:59:55 2015 +0100
> > Commit: Jakub Hrozek<jhrozek(a)redhat.com>
> > 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<pcech(a)redhat.com>
> >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<pcech(a)redhat.com>
> >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<pcech(a)redhat.com>
> >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 Cech<pcech(a)redhat.com>
> >Date: Tue, 10 Nov 2015 06:04:45 -0500
> >Subject: [PATCH 4/8] TEST: Add common function are_values_in_array()
> >
> >This patch adds function are_values_in_array() to common test code. And
> >there is tc_are_values_in_array macro defined which is usefull for
> >talloc allocated values and arrays.
Thanks, ACK
Thanks.
> > From 5e6274406deeb64e0c8a125d6409300b3bda6d61 Mon Sep 17 00:00:00 2001
> >From: Petr Cech<pcech(a)redhat.com>
> >Date: Tue, 27 Oct 2015 04:02:46 -0400
> >Subject: [PATCH 5/8] TEST: Add test_users_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 users_by_recent_filter_valid().
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2730
Two remarks about memory contexts
> >---
> > src/tests/cmocka/test_responder_cache_req.c | 68 ++++++++++++++++++++++++++++-
> > 1 file changed, 67 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/tests/cmocka/test_responder_cache_req.c
b/src/tests/cmocka/test_responder_cache_req.c
> >index
439084c85c97c8405227c2599621d89c3dc85fe4..b680bd8f24ede5b149d0f814fbf0a2aa9cbfa040 100644
> >--- a/src/tests/cmocka/test_responder_cache_req.c
> >+++ b/src/tests/cmocka/test_responder_cache_req.c
> >@@ -318,7 +318,6 @@ static void check_group(struct cache_req_test_ctx *test_ctx,
> > assert_string_equal(exp_dom->name, test_ctx->domain->name);
> > }
> >
> >-
> > struct tevent_req *
> > __wrap_sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
> > struct resp_ctx *rctx,
> >@@ -1317,6 +1316,72 @@ void test_user_by_recent_filter_valid(void **state)
> > assert_string_equal(ldbname, TEST_USER_NAME);
> > }
> >
> >+void test_users_by_recent_filter_valid(void **state)
> >+{
> >+ struct cache_req_test_ctx *test_ctx = NULL;
> >+ TALLOC_CTX *req_mem_ctx = NULL;
> >+ TALLOC_CTX *tmp_ctx = NULL;
[...]
> >+ tmp_ctx = talloc_new(req_mem_ctx);
I don't see the need for tmp_ctx really, you can just use test_ctx..
Fixed.
> >+
> >+ user_names = talloc_array(tmp_ctx, const char *, 2);
> >+ assert_non_null(user_names);
> >+ user_names[0] = TEST_USER_NAME;
> >+ user_names[1] = TEST_USER_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(tc_are_values_in_array(user_names, ldb_results));
> >+
> >+ talloc_zfree(tmp_ctx);
Please either free both tmp_ctx and req_mem_ctx or none of them. I think
freeing none of them and relying on talloc to free them when test_ctx is
gone is fine.
Fixed.
> > From 8a38a1c3d03f9c89725f9b9e686696882eed305e Mon Sep 17 00:00:00 2001
> >From: Petr Cech<pcech(a)redhat.com>
> >Date: Sun, 1 Nov 2015 07:09:28 -0500
> >Subject: [PATCH 6/8] TEST: Add test_group_by_recent_filter_valid
ACK
Thanks.
> > From 71347406a22bc0a9a6931abe83dcb65b8f3eca8a Mon Sep 17 00:00:00 2001
> >From: Petr Cech<pcech(a)redhat.com>
> >Date: Sun, 1 Nov 2015 07:21:18 -0500
> >Subject: [PATCH 7/8] 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
ACK
Thanks.
> > From 331a7b0f995aadb69dfa15a89aa46875e67d4377 Mon Sep 17 00:00:00 2001
> >From: Petr Cech<pcech(a)redhat.com>
> >Date: Sun, 1 Nov 2015 07:45:56 -0500
> >Subject: [PATCH 8/8] TEST: Add test_groups_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 groups_by_recent_filter_valid().
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2730
> >---
> > src/tests/cmocka/test_responder_cache_req.c | 68 +++++++++++++++++++++++++++++
> > 1 file changed, 68 insertions(+)
> >
> >diff --git a/src/tests/cmocka/test_responder_cache_req.c
b/src/tests/cmocka/test_responder_cache_req.c
> >index
5030d3e3fcee9223e855309c70403e4f7e25a749..7725c76651e2caf545b5b26488bc8b6417b9b2e5 100644
> >--- a/src/tests/cmocka/test_responder_cache_req.c
> >+++ b/src/tests/cmocka/test_responder_cache_req.c
> >@@ -1557,6 +1557,73 @@ void test_group_by_recent_filter_valid(void **state)
> > assert_string_equal(ldbname, TEST_GROUP_NAME);
> > }
> >
> >+void test_groups_by_recent_filter_valid(void **state)
> >+{
> >+ struct cache_req_test_ctx *test_ctx = NULL;
> >+ TALLOC_CTX *req_mem_ctx = NULL;
> >+ TALLOC_CTX *tmp_ctx = NULL;
> >+ struct tevent_req *req = NULL;
> >+ const char **group_names = NULL;
> >+ const char **ldb_results = NULL;
> >+ const char *ldbname = NULL;
> >+ 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));
Can we use time(NULL)-1 and drop the sleep(1) ?
Yes, of course. I fixed it.
Otherwise ACK.
Thanks.
CI is still running..
There is new patch set attached.
Petr