[SSSD] [WIP] [TEST]: Observation patch

Petr Cech pcech at redhat.com
Mon Aug 17 08:48:14 UTC 2015


On 08/17/2015 10:37 AM, Lukas Slebodnik wrote:
> On (17/08/15 10:17), Petr Cech wrote:
>> On 08/17/2015 08:52 AM, Lukas Slebodnik wrote:
>>> >From c871c97862997df4e724647f1a0ce7297f2f059b Mon Sep 17 00:00:00 2001
>>>>> From: Petr Cech<pcech at redhat.com>
>>>>> Date: Fri, 14 Aug 2015 13:17:22 +0200
>>>>> Subject: [PATCH] TEST: Fix for responder_cache_req-tests
>>>>>
>>>>> Tests, that do not pass, have a problem with time. Time for writing
>>>>> records into database varied from time of creating a request,  that is
>>>>> used for filtering records internally.
>>>>>
>>>>> The patch modifies the time of creation record (adds one second to
>>>>> now()), so it should not be different times there.
>>>>>
>>>>> Resolves:
>>>>> https://fedorahosted.org/sssd/ticket/2730
>>>>> ---
>>>>> src/tests/cmocka/test_responder_cache_req.c |   18 ++++++++++++------
>>>>> 1 files changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
>>>>> index 032fe429ac88b8cc9113976329ea04837f287276..4f77fe767e016496652a97c7a73fc9e29ba7faf0 100644
>>>>> --- a/src/tests/cmocka/test_responder_cache_req.c
>>>>> +++ b/src/tests/cmocka/test_responder_cache_req.c
>>>>> @@ -1721,9 +1721,10 @@ void test_users_by_filter_valid(void **state)
>>>>>      test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
>>>>>      test_ctx->create_user = true;
>>>>>
>>>>> +    /* set (time+1) to avoid failure request time filter */
>>>>>      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));
>>>>> +                           NULL, 1000, time(NULL)+1);
>>>>>      assert_int_equal(ret, EOK);
>>> Although, this patch fix intermitent failures
>>> there are few problems.
>>>
>>> The protopype of function sysdb_store_user is:
>>> /* this function does not check that all user members are actually present */
>>>
>>> /* if one of the basic attributes is empty ("") as opposed to NULL,
>>>   * this will just remove it */
>>>
>>> int sysdb_store_user(struct sss_domain_info *domain,
>>>                       const char *name,
>>>                       const char *pwd,
>>>                       uid_t uid, gid_t gid,
>>>                       const char *gecos,
>>>                       const char *homedir,
>>>                       const char *shell,
>>>                       const char *orig_dn,
>>>                       struct sysdb_attrs *attrs,
>>>                       char **remove_attrs,
>>>                       uint64_t cache_timeout,
>>>                       time_t now);
>>>
>>>
>>> and if now is 0 then we will get the current time.
>>> 1912     /* get transaction timestamp */
>>> 1913     if (!now) {
>>> 1914         now = time(NULL);
>>> 1915     }
>>>
>>> I do not understand why we shoudl set current time (now)
>>> to future "time(NULL)+1". I didn't check it properly, but
>>> if now is used as transaction timestamp (according to comment)
>>> it should not be from futire.
>>>
>>> LS
>> The initial value was time(now) and it could be simply 0, I agree with that.
>> (I've tried time(now) -> 0, but unfortunately it was not enough. The problem
>> is elsewhere.)
>>
>> The problem is reading the data. There is a filter from a certain time,
>> internally used time is set to time of creating request for reading data. But
>> this request is creating after inserting data. Therefore, you can insert a
>> timestamp data and timestamp of request creation vary, especially if the
>> machine is busy.
>>
> But busy machine can be also in production.
> Can this situation occur in real deployment?
>
> The unit test should be considered as additional documentation.
> Your solution would mean we can/should use "time(NULL)+1" even in production.
> Which does not seems to be a correct solution.
OK, I understand. I will try to rewrite test in right way.

Now we have:
1. insert data (timestamp A)
2.create request for reading (timestamp B)
3. reading with condition time >= timestamp B

But it should by
0. timestamp 0
1. insert data (timestamp A)
2.create request for reading (timestamp B)
3. reading with condition time >= timestamp 0

Petr
>> Completely correct solution (meaning clear) would be create a request to read
>> data in the beginning of the test, then insert data and then try to read it.
>> I tried this, I had complication with mock then.
>>
> If you have troubles with mock/cmocka
> then please at least try to bisect which patch broke it
> and ask an author for help.
>
> BTW I cannot see it in coding guidelines but it's better to add spaces
> around operators. "time(NULL)+1" -> "time(NULL) + 1"
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list