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

Lukas Slebodnik lslebodn at redhat.com
Mon Aug 17 08:37:31 UTC 2015


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.

>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



More information about the sssd-devel mailing list