On (10/11/14 14:29), Pavel Březina wrote:
From 8eb7a424bccfcbb6140fc2d6eed91759d765c1a4 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/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(a)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(a)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(a)redhat.com>
>Date: Fri, 7 Nov 2014 14:47:12 +0100
>Subject: [PATCH 4/4] tests: be_ptask
>
>Resolves:
>---
> 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(a)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)
Very nice unit test.
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.)
There are 6 "time_t" arguments be_ptask_create.
perriod and first_delay are tested.
enabled_delay, 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.
LS