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....@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....@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....@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 <lsleb...@redhat.com> > -- > 1.8.1.4 > Ack to this patch, I will push this one out-of band. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel