On Sat, Mar 23, 2013 at 01:49:26AM +0530, Abhishek Singh wrote:
patches attached.
Hello Abhishek,
in general the tests look good to me, thank you! I only have a couple of
nitpicks now. See inline.
From 2f3281a7b0af9f4a5f53dec0b911b024251ad828 Mon Sep 17 00:00:00
2001
From: Abhishek Singh <abhishekkumarsingh.cse(a)gmail.com>
Date: Sat, 23 Mar 2013 01:42:48 +0530
Subject: [PATCH] cmocka unittest for io added
---
Makefile.am | 13 +++-
src/tests/cmocka/test_io.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 160 insertions(+), 1 deletion(-)
create mode 100644 src/tests/cmocka/test_io.c
diff --git a/Makefile.am b/Makefile.am
index aa79ae6fbd637c7b1301f37130e729013602ff0d..fa906e91542764403e926200d880970e8125b8d7
100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -147,7 +147,8 @@ endif
if HAVE_CMOCKA
non_interactive_cmocka_based_tests = \
nss-srv-tests \
- test-find-uid
+ test-find-uid \
+ test-io
endif
check_PROGRAMS = \
@@ -1232,6 +1233,16 @@ test_find_uid_LDADD = \
$(DHASH_LIBS) \
$(CMOCKA_LIBS) \
libsss_util.la
+
+test_io_DEPENDENCIES = \
+ $(ldblib_LTLIBRARIES)
I don't think you need to link against ldb here, you don't use the cache
in your test. ldb is the library that we use as an on-disk cache for the
SSSD.
+test_io_SOURCES = \
+ src/tests/cmocka/test_io.c \
+ src/util/io.c
+test_io_CFLAGS = \
+ $(AM_CFLAGS)
+test_io_LDADD = \
+ $(CMOCKA_LIBS)
endif
noinst_PROGRAMS = pam_test_client
diff --git a/src/tests/cmocka/test_io.c b/src/tests/cmocka/test_io.c
new file mode 100644
index 0000000000000000000000000000000000000000..bed7ef196e73e7e93879b8b659721369e6c0cbb4
--- /dev/null
+++ b/src/tests/cmocka/test_io.c
@@ -0,0 +1,148 @@
+/*
+ SSSD
+
+ find_uid - Utilities tests
+
+ Authors:
+ Abhishek Singh <abhishekkumarsingh.cse(a)gmail.com>
+
+ Copyright (C) 2013 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 <stdio.h>
+#include <fcntl.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+#include <dirent.h>
+#include <unistd.h>
+
+#include "util/io.h"
+
+#define FILE_PATH "/tmp/test.in"
Instead of hardcoding the file in /tmp I would prefer if the test used
TEST_DIR (see its usage elsewhere in the SSSD) as a base directory for
the test file. FILE_PATH could then be defined as (not tested, just an
idea):
#define FILE_PATH TEST_DIR"/test.in".
Maybe also instead of using test.in you could use mkstemp to cover the
odd case there would be several testcases running concurrently.
+#define NON_EX_PATH "non-existent-path"
+
+static void setup_all(void)
+{
+ FILE *fp;
+ fp = fopen(FILE_PATH, "w");
+ if (fp != NULL)
We prefer to use brackets even for single-line statements. In other
words, use:
if (fp != NULL) {
fclose(fp);
}
+ fclose(fp);
+ else
+ printf("Error! file test.in can't be created");
I would prefer fprintf(stderr) here.
+}
+
+static void teardown_all(void)
+{
+ remove(FILE_PATH);
+}
+
+static int get_dirfd(void)
+{
+ int dir_fd;
+ DIR *tmp = opendir("/tmp");
+ if (tmp != NULL)
+ dir_fd = dirfd(tmp);
+
This seems like a bad indentation to me. Also the directory could be a
parameter instead of hardcoding it. The caller could then hardcode it to
TEST_DIR, but still, I would prefer a parameter.
+ return dir_fd;
+}
+
+void test_sss_open_cloexec_success(void **state)
+{
+ int fd;
+ int ret;
+ int ret_flag;
+ int expec_flag;
+ int flags = O_RDWR;
+
+ fd = sss_open_cloexec(FILE_PATH, flags, &ret);
+
+ ret_flag = fcntl(fd, F_GETFD, 0);
+ expec_flag = FD_CLOEXEC;
+
+ assert_true(fd != -1);
I think you should first test for fd validity and only then call GETFD
and then test for the flag.
+ assert_true(ret_flag & expec_flag);
+
+ close(fd);
+}
+
+void test_sss_open_cloexec_fail(void **state)
+{
+ int fd;
+ int ret;
+ int flags = O_RDWR;
+
+ fd = sss_open_cloexec(NON_EX_PATH, flags, &ret);
+
+ assert_true(fd == -1);
+ assert_int_not_equal(ret, 0);
+
+ close(fd);
+}
+
+void test_sss_openat_cloexec_success(void **state)
+{
+ int fd;
+ int ret;
+ int ret_flag;
+ int expec_flag;
+ int dir_fd;
+ int flags = O_RDWR;
+
+ dir_fd = get_dirfd();
+ fd = sss_openat_cloexec(dir_fd, "test.in", flags, &ret);
+
+ ret_flag = fcntl(fd, F_GETFD, 0);
+ expec_flag = FD_CLOEXEC;
+
+ assert_true(fd != -1);
+ assert_true(ret_flag & expec_flag);
+
+ close(fd);
+}
+
+void test_sss_openat_cloexec_fail(void **state)
+{
+ int fd;
+ int ret;
+ int dir_fd;
+ int flags = O_RDWR;
+
+ dir_fd = get_dirfd();
+ fd = sss_openat_cloexec(dir_fd, NON_EX_PATH, flags, &ret);
+
+ assert_true(fd == -1);
+ assert_int_not_equal(ret, 0);
+
+ close(fd);
+}
+
+int main(void)
+{
+ const UnitTest tests[] = {
+ unit_test_setup_teardown(test_sss_open_cloexec_success, setup_all,
+ teardown_all),
+ unit_test_setup_teardown(test_sss_open_cloexec_fail, setup_all,
+ teardown_all),
+ unit_test_setup_teardown(test_sss_openat_cloexec_success, setup_all,
+ teardown_all),
+ unit_test_setup_teardown(test_sss_openat_cloexec_fail, setup_all,
+ teardown_all)
+ };
+
+ return run_tests(tests);
+}
--
1.8.1.4
The rest looks good to me.
From b9f90d1313b91fb7213b006b4efaaf6c50568e26 Mon Sep 17 00:00:00
2001
From: Abhishek Singh <abhishekkumarsingh.cse(a)gmail.com>
Date: Sat, 23 Mar 2013 01:47:23 +0530
Subject: [PATCH] filename in comment is corrected
---
src/util/io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/io.c b/src/util/io.c
index 4d1f2e74a1607bf7f6a8977ca97455185eba7403..7f979c941d11e78d409f6a9c0f06dd98a29be6e4
100644
--- a/src/util/io.c
+++ b/src/util/io.c
@@ -1,7 +1,7 @@
/*
SSSD
- compact.c
+ io.c
Authors:
Lukas Slebodnik <lslebodn(a)redhat.com>
--
1.8.1.4
Ack to this patch, I will push this one out-of band.