On (20/11/14 17:41), Pavel Březina wrote:
On 11/18/2014 06:32 PM, Lukas Slebodnik wrote:
>On (18/11/14 18:26), Lukas Slebodnik wrote:
All above issues where addressed.
>>Very nice unit test.
Thank you.
>>It would be good to add few more tests with corner cases.
>>There aren't test for failed functions (_send, _recv)
>>(Attached is diff with test for error occured in be_ptask_sync_t.)
Thank you for the diff, I have rewritten it slightly. We don't really care
that the tevent_req error was set to the return code of yours - that is
hidden to the caller. What we want to check is that the task will be
correctly rescheduled according to documentation.
This test also revealed a huge bug that we are quite lucky we haven't been
punished for it so far. So cudos. (current patch #4).
I added new tests for corner cases (_recv returns error is already there as
*reschedule_error):
- test_be_ptask_reschedule_null - _send returns NULL
- test_be_ptask_reschedule_timeout - req timed out
- test_be_ptask_sync_reschedule_ok - sync task returns EOK
- test_be_ptask_sync_reschedule_error - sync task returns error
>>There are 6 "time_t" arguments be_ptask_create.
>>perriod and first_delay are tested.
>>enabled_delay,
enabled_delay is tested as well.
random_offset, timeout, max_backoff are tested just with zero.
>>It would be good to add tests at least for timeout, max_backoff.
>>The most lines without code coverage are due to this two variables.
Digging now into the implementation of the backoff feature, I believe it is
wrong. It seems to do what it is supposed to, however it overrides even
first_delay and enabled delay. We should fix that... or was it an intended
behaviour?
>BTW: gcc 4.9 produces a warning in unit test.
>
> CC src/providers/ad_gpo_tests-dp_ptask.o
>src/tests/cmocka/test_be_ptask.c: In function ‘test_be_ptask_reschedule_error’:
>src/tests/cmocka/test_be_ptask.c:558:53: error: ‘now’ may be used uninitialized in
this function [-Werror=maybe-uninitialized]
> assert_true(now + period <= ptask->next_execution);
> ^
>cc1: all warnings being treated as errors
>Makefile:16655: recipe for target
'src/tests/cmocka/test_be_ptask-test_be_ptask.o' failed
>
Thanks, fixed.
>LS
From 150e5559226aa48cf1376b1b0769c7bd092e4ef1 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 10 Nov 2014 13:30:28 +0100
Subject: [PATCH 1/5] be_ptask: create a private header file
This is done so we gain access to the be_ptask structure in unit tests.
---
ACK
From e1ca3edce0cca74097c3a0514a00d686fd936a90 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 10 Nov 2014 13:59:27 +0100
Subject: [PATCH 2/5] be_ptask: handle OFFLINE_DISABLE mode before task
execution
---
ACK
From eb3a9e90972915ce4683ed2628c6a315fbc017f0 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Mon, 10 Nov 2014 14:26:32 +0100
Subject: [PATCH 3/5] be_ptask: add next_execution time to struct be_ptask
For debugging and testing purposes.
---
ACK
From 9bd5d69ab90f66731a467eea4d4917922a28746e Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Thu, 20 Nov 2014 16:53:10 +0100
Subject: [PATCH 4/5] be_ptask: do not store sync ctx to _task
The _task is an output variable of type struct be_ptask * which
is filled by be_ptask_create(). However, we tried to set sync ctx there
as a result of copy and paste error.
---
ACK
From db2a48e3855bae423e9cbf4a9e7736425a1a9569 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Fri, 7 Nov 2014 14:47:12 +0100
Subject: [PATCH 5/5] tests: be_ptask
Resolves:
https://fedorahosted.org/sssd/ticket/1939
---
Tests passed on all distros
http://sssd-ci.duckdns.org/logs/job/4/18/summary.html
The failed test on RHEL7 is unrelated to your patches.
ACK
LS