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