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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel