[SSSD] SSSD Test Suite Coverage

Jakub Hrozek jhrozek at redhat.com
Tue Mar 26 21:39:42 UTC 2013


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 at 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 at 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 at 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 at redhat.com>
> -- 
> 1.8.1.4
> 

Ack to this patch, I will push this one out-of band.



More information about the sssd-devel mailing list