[SSSD] [PATCH] tests: be_ptask

Lukas Slebodnik lslebodn at redhat.com
Thu Nov 27 17:45:11 UTC 2014


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



More information about the sssd-devel mailing list