[SSSD] [PATCH] tests: be_ptask

Pavel Březina pbrezina at redhat.com
Thu Nov 20 16:41:00 UTC 2014


On 11/18/2014 06:32 PM, Lukas Slebodnik wrote:
> On (18/11/14 18:26), Lukas Slebodnik wrote:
>> On (10/11/14 14:29), Pavel Březina wrote:
>>> https://fedorahosted.org/sssd/ticket/1939
>>
>> >From 8eb7a424bccfcbb6140fc2d6eed91759d765c1a4 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/4] be_ptask: create a private header file
>>>
>>> This is done so we gain access to the be_ptask structure in unit tests.
>>> ---
>> ACK
>>
>> >From 09dc2fe755b0ac1af21fe888c2dbdd9b6693a3ed 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/4] be_ptask: handle OFFLINE_DISABLE mode before task
>>> execution
>>>
>>> ---
>> ACK
>>
>> >From 30cef74931f242ec5ee7bb8790cc52140671ae86 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/4] be_ptask: add next_execution time to struct be_ptask
>>>
>>> For debugging and testing purposes.
>>> ---
>> ACK
>>
>> >From 4bf4b559a7f1990aa420cd73425af5107f078ba5 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 4/4] tests: be_ptask
>>>
>>> Resolves:
>>> https://fedorahosted.org/sssd/ticket/1939
>>> ---
>>> Makefile.am                      |  16 +
>>> src/tests/cmocka/test_be_ptask.c | 661 +++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 677 insertions(+)
>>> create mode 100644 src/tests/cmocka/test_be_ptask.c
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index 5c30aac82c9aa818d490b909f73c3be2894a3a57..e3057d09bf7662a1d1bf26b9d5ec9294157d24e1 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -219,6 +219,7 @@ if HAVE_CMOCKA
>>>          test_search_bases \
>>>          sdap-tests \
>>>          test_sysdb_views \
>>> +        test_be_ptask \
>>>          $(NULL)
>>>
>>> if BUILD_IFP
>>> @@ -2079,6 +2080,21 @@ test_sysdb_views_LDADD = \
>>>      $(SSSD_INTERNAL_LTLIBS) \
>>>      libsss_test_common.la \
>>>      $(NULL)
>>> +
>>   ^^^^
>>   trailing character '\t'
>>> +test_be_ptask_SOURCES = \
>>> +    src/tests/cmocka/test_be_ptask.c \
>>> +    src/providers/dp_ptask.c \
>>> +    $(NULL)
>>> +test_be_ptask_CFLAGS = \
>>> +    $(AM_CFLAGS) \
>>> +    $(NULL)
>>> +test_be_ptask_LDADD = \
>>> +    $(CMOCKA_LIBS) \
>>> +    $(POPT_LIBS) \
>>> +    $(TALLOC_LIBS) \
>>> +    $(SSSD_INTERNAL_LTLIBS) \
>>> +    libsss_test_common.la \
>>> +    $(NULL)
>>>
>>> endif # HAVE_CMOCKA
>>>
>>> diff --git a/src/tests/cmocka/test_be_ptask.c b/src/tests/cmocka/test_be_ptask.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..00acce2758b9dc7711e9415671f11cdf0bb47ebd
>>> --- /dev/null
>>> +++ b/src/tests/cmocka/test_be_ptask.c
>>> @@ -0,0 +1,661 @@
>>> +/*
>>> +    Authors:
>>> +        Pavel Březina <pbrezina at redhat.com>
>>> +
>>> +    Copyright (C) 2014 Red Hat
>>> +
>>> +    This program is free software; you can redistribute it and/or modify
>>> +    it under the terms of the GNU General Public License as published by
>>> +    the Free Software Foundation; either version 3 of the License, or
>>> +    (at your option) any later version.
>>> +
>>> +    This program is distributed in the hope that it will be useful,
>>> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +    GNU General Public License for more details.
>>> +
>>> +    You should have received a copy of the GNU General Public License
>>> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +*/
>>> +
>>> +#include <talloc.h>
>>> +#include <tevent.h>
>>> +#include <errno.h>
>>> +#include <popt.h>
>>> +#include <time.h>
>>> +
>>> +#include "providers/dp_backend.h"
>>> +#include "providers/dp_ptask_private.h"
>>> +#include "providers/dp_ptask.h"
>>> +#include "tests/cmocka/common_mock.h"
>>> +#include "tests/common.h"
>>> +
>>> +#define new_test(test) \
>>> +    unit_test_setup_teardown(test_ ## test, test_setup, test_teardown)
>>> +
>>> +struct test_ctx {
>>> +    struct be_ctx *be_ctx;
>>> +
>>> +    time_t when;
>>> +    bool done;
>>> +
>>> +    bool add_online_cb_called;
>>> +    bool add_offline_cb_called;
>>> +};
>>> +
>>> +#define mark_online(test_ctx) do { \
>>> +    test_ctx->be_ctx->offstat.went_offline = 0; \
>>> +    test_ctx->be_ctx->offstat.offline = false; \
>>> +} while (0)
>>    The macro mark_online is unused.
>>    Did you plan to use this macro somehere?
>>
>> //snip
>>
>>> +void test_setup(void **state)
>>> +{
>>> +    struct test_ctx *test_ctx = NULL;
>>> +
>>> +    //assert_true(leak_check_setup());
>>       ^^^
>>     Could you remove comment?
>>> +
>>> +    test_ctx = talloc_zero(global_talloc_context, struct test_ctx);
>>> +    assert_non_null(test_ctx);
>>> +
>>> +    /* create be_ctx, only ev and offline field should be used */
>>> +    test_ctx->be_ctx = talloc_zero(test_ctx, struct be_ctx);
>>> +    assert_non_null(test_ctx->be_ctx);
>>> +
>>> +    test_ctx->be_ctx->ev = tevent_context_init(test_ctx->be_ctx);
>>> +    assert_non_null(test_ctx->be_ctx->ev);
>>> +
>>> +    *state = test_ctx;
>>> +}
>>> +
>>> +void test_teardown(void **state)
>>> +{
>>> +    talloc_zfree(*state);
>>> +    //assert_true(leak_check_teardown());
>>       ^^^
>>    Could you remove comment here as well?
>>    I didn't have any problem with allocation leak check assertions.
>>> +}
>>> +
>>> +void test_be_ptask_create_first_delay(void **state)
>>> +{
>>> +    struct test_ctx *test_ctx = (struct test_ctx *)(*state);
>>> +    struct be_ptask *ptask = NULL;
>>> +    time_t delay = 2;
>>       IMHO, it would be better to use constant here (with uppercase)
>>       It would improve readability.
>>       The same applies to period in other tests.
>>
>>> +    time_t now;
>>> +    errno_t ret;
>>> +
>>> +    now = time(NULL);
>>> +    ret = be_ptask_create(test_ctx, test_ctx->be_ctx, 10, delay, 0, 0, 0,
>>> +                          BE_PTASK_OFFLINE_SKIP, 0, test_be_ptask_send,
>>> +                          test_be_ptask_recv, test_ctx, "Test ptask", &ptask);
>>> +    assert_int_equal(ret, ERR_OK);
>>> +    assert_non_null(ptask);
>>> +    assert_non_null(ptask->timer);
>>> +
>>> +}
>>> +
>>> +    out_period = be_ptask_get_period(ptask);
>>> +    assert_true(period == out_period);
>>> +
>>> +    be_ptask_destroy(&ptask);
>>> +    assert_null(ptask);
>>> +}
>>> +
>>> +void test_be_ptask_create_sync(void **state)
>>> +{
>>> +    struct test_ctx *test_ctx = (struct test_ctx *)(*state);
>>> +    struct be_ptask *ptask = NULL;
>>> +    time_t now;
>>> +    errno_t ret;
>>> +
>>> +    now = time(NULL);
>>> +    ret = be_ptask_create_sync(test_ctx, test_ctx->be_ctx, 10, 0, 0, 0, 0,
>>> +                          BE_PTASK_OFFLINE_SKIP, 0, test_be_ptask_sync,
>>> +                          test_ctx, "Test ptask", &ptask);
>>                             ^^^
>>                         Indentation issue.
>>                         (simple diff is attached.)
>>> +    assert_int_equal(ret, ERR_OK);
>>> +    assert_non_null(ptask);
>>> +    assert_non_null(ptask->timer);
>>> +
>>> +    while (!test_ctx->done) {
>>> +        tevent_loop_once(test_ctx->be_ctx->ev);
>>> +    }
>>> +
>>> +    assert_true(now <= ptask->last_execution);
>>> +    assert_true(now <= test_ctx->when);
>>> +    assert_true(ptask->last_execution <= test_ctx->when);
>>> +
>>> +    be_ptask_destroy(&ptask);
>>> +    assert_null(ptask);
>>> +}
>>> +
>>> +int main(int argc, const char *argv[])
>>> +{
>>> +    poptContext pc;
>>> +    int opt;
>>> +    struct poptOption long_options[] = {
>>> +        POPT_AUTOHELP
>>> +        SSSD_DEBUG_OPTS
>>> +        POPT_TABLEEND
>>> +    };
>>> +
>>> +    const UnitTest tests[] = {
>>> +        new_test(be_ptask_create_einval_be),
>>> +        new_test(be_ptask_create_einval_period),
>>> +        new_test(be_ptask_create_einval_send),
>>> +        new_test(be_ptask_create_einval_recv),
>>> +        new_test(be_ptask_create_einval_name),
>>> +        new_test(be_ptask_create_no_delay),
>>> +        new_test(be_ptask_create_first_delay),
>>> +        new_test(be_ptask_disable),
>>> +        new_test(be_ptask_enable),
>>> +        new_test(be_ptask_enable_delay),
>>> +        new_test(be_ptask_offline_skip),
>>> +        new_test(be_ptask_offline_disable),
>>> +        new_test(be_ptask_offline_execute),
>>> +        new_test(be_ptask_reschedule_ok),
>>> +        new_test(be_ptask_reschedule_error),
>>> +        new_test(be_ptask_get_period),
>>> +        new_test(be_ptask_create_sync)

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-be_ptask-create-a-private-header-file.patch
Type: text/x-patch
Size: 4081 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141120/5ad02f17/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-be_ptask-handle-OFFLINE_DISABLE-mode-before-task-exe.patch
Type: text/x-patch
Size: 1212 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141120/5ad02f17/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-be_ptask-add-next_execution-time-to-struct-be_ptask.patch
Type: text/x-patch
Size: 1478 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141120/5ad02f17/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-be_ptask-do-not-store-sync-ctx-to-_task.patch
Type: text/x-patch
Size: 910 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141120/5ad02f17/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-tests-be_ptask.patch
Type: text/x-patch
Size: 27942 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141120/5ad02f17/attachment-0004.bin>


More information about the sssd-devel mailing list