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<pc...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel