[SSSD] [PATCHES] ldap: refactor - reuse sss_utc_to_time_t()

Pavel Reichl preichl at redhat.com
Fri Mar 20 12:13:35 UTC 2015


On 03/20/2015 12:11 PM, Jakub Hrozek wrote:
> On Mon, Mar 16, 2015 at 12:10:02PM +0100, Pavel Reichl wrote:
>> Hello,
>>
>> I recently introduced new utility function sss_utc_to_time_t(). This patches should refactor some existing code to reuse it.
>>
>> 1st patch - add unit tests for fuctions to be amended.
>> 2nd patch - amends check_pwexpire_kerberos() and modifies test - to clearly show how has behavior changed.
>> 3rd patch - amends nds_check_expired() - no test changes required
>>
>> ci of this version of patches has passed.
>> http://sssd-ci.duckdns.org/logs/job/10/95/summary.html
>>
>> Thanks!
>>
> The refactoring looks good to me but I don't like the tests :-)
>
>>  From c323db03ff2c72dbfe6496babe2c0920a412118c Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Fri, 6 Mar 2015 04:29:24 -0500
>> Subject: [PATCH 1/3] TESTS: test expiration
>>
>> ---
>>   Makefile.am                           |  44 ++++++++++++
>>   src/providers/ldap/sdap_access.c      |   2 +-
>>   src/tests/cmocka/test_expire_common.c | 130 ++++++++++++++++++++++++++++++++++
>>   src/tests/cmocka/test_expire_common.h |  40 +++++++++++
>>   src/tests/cmocka/test_ldap_auth.c     |  99 ++++++++++++++++++++++++++
>>   src/tests/cmocka/test_sdap_access.c   |  71 +++++++++++++++++++
>>   6 files changed, 385 insertions(+), 1 deletion(-)
>>   create mode 100644 src/tests/cmocka/test_expire_common.c
>>   create mode 100644 src/tests/cmocka/test_expire_common.h
>>   create mode 100644 src/tests/cmocka/test_ldap_auth.c
>>   create mode 100644 src/tests/cmocka/test_sdap_access.c
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index ba24343c3a2be9edc94ec817d3709a0c78599847..bc37a04fc44d0f8f94abc050fb2c9eff15a65ab6 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -212,6 +212,8 @@ if HAVE_CMOCKA
>>           sbus-internal-tests \
>>           sss_sifp-tests \
>>           test_search_bases \
>> +        test_ldap_auth \
>> +        test_sdap_access \
>>           sdap-tests \
>>           test_sysdb_views \
>>           test_be_ptask \
>> @@ -1970,6 +1972,48 @@ test_search_bases_LDADD = \
>>       libsss_krb5_common.la \
>>       libsss_test_common.la
>>   
>> +test_ldap_auth_SOURCES = \
>> +    $(sssd_be_SOURCES) \
>> +    src/tests/cmocka/test_ldap_auth.c \
>> +    src/tests/cmocka/test_expire_common.c \
>> +    $(NULL)
>> +
>> +test_ldap_auth_CFLAGS = \
>> +    $(AM_CFLAGS) \
>> +    -DUNIT_TESTING
>> +test_ldap_auth_LDADD = \
>> +    $(PAM_LIBS) \
>> +    $(CMOCKA_LIBS) \
>> +    $(POPT_LIBS) \
>> +    $(SSSD_LIBS) \
>> +    $(CARES_LIBS) \
>> +    $(KRB5_LIBS) \
>> +    $(SSSD_INTERNAL_LTLIBS) \
>> +    libsss_ldap_common.la \
>> +    libsss_test_common.la \
>> +    $(NULL)
>> +
>> +test_sdap_access_SOURCES = \
>> +    $(sssd_be_SOURCES) \
>> +    src/tests/cmocka/test_sdap_access.c \
>> +    src/tests/cmocka/test_expire_common.c \
>> +    $(NULL)
>> +
>> +test_sdap_access_CFLAGS = \
>> +    $(AM_CFLAGS) \
>> +    -DUNIT_TESTING
>> +test_sdap_access_LDADD = \
>> +    $(PAM_LIBS) \
>> +    $(CMOCKA_LIBS) \
>> +    $(POPT_LIBS) \
>> +    $(SSSD_LIBS) \
>> +    $(CARES_LIBS) \
>> +    $(KRB5_LIBS) \
>> +    $(SSSD_INTERNAL_LTLIBS) \
>> +    libsss_ldap_common.la \
>> +    libsss_test_common.la \
>> +    $(NULL)
> I wonder why didn't you create a single pwexpire test instead? The file
> names are confusing, I would expect tests of authentication and acess
> control.
The functionality I wanted to test was in 2 separate units (ldap_auth,c, 
sdap_access.c), but for those units there were no tests. So I created 2 
new test units (test_ldap_auth,c, test_sdap_access.c) in hope that more 
functions from these units will be tested. (code coverage is currently 
0.0 % for both modules 
(https://jhrozek.fedorapeople.org/sssd-coverage/html/providers/ldap/index.html)

I can merge the tests into one file and just test the functionality that 
the 3rd patch is changing. I guess this would be more consistent with 
the way other tests work.


>
>> +int expire_test_setup(void **state)
>> +{
>> +    struct expire_test_ctx *exp_state;
>> +    TALLOC_CTX *mem_ctx;
>> +    char *past_time;
>> +    char *future_time;
>> +    char *invalid_format;
>> +    char *invalid_longer_format;
>> +
>> +    mem_ctx = talloc_new(NULL);
>> +    assert_non_null(mem_ctx);
>> +
>> +    exp_state = talloc(mem_ctx, struct expire_test_ctx);
>> +    assert_non_null(exp_state);
>> +
>> +    exp_state->mem_ctx = mem_ctx;
> Please don't use a memory context as a structure member, use the
> structure itself.
>
>> +
>> +    *state = exp_state;
>> +
>> +    /* testing data */
>> +    invalid_format = (void*)now_str(mem_ctx, "%Y%m%d%H%M%S", -20);
>> +    assert_non_null(invalid_format);
>> +
>> +    invalid_longer_format = (void*)now_str(mem_ctx, "%Y%m%d%H%M%SZA", -20);
>> +    assert_non_null(invalid_longer_format);
>> +
>> +    past_time = (void*)now_str(mem_ctx, "%Y%m%d%H%M%SZ", -20);
>> +    assert_non_null(past_time);
>> +
>> +    future_time = (void*)now_str(mem_ctx, "%Y%m%d%H%M%SZ", 20);
>> +    assert_non_null(future_time);
>> +
>> +    exp_state->past_time = past_time;
>> +    exp_state->future_time = future_time;
>> +    exp_state->invalid_format = invalid_format;
>> +    exp_state->invalid_longer_format = invalid_longer_format;
>> +
>> +    return 0;
>> +}
>> +
>> +int expire_test_teardown(void **state)
>> +{
>> +    struct expire_test_ctx *test_ctx;
>> +
>> +    test_ctx = talloc_get_type(*state, struct expire_test_ctx);
>> +    assert_non_null(test_ctx);
>> +
>> +    talloc_free(test_ctx->mem_ctx);
> Just free the context here.
>
>
> [...]
>
>> +struct indata {
>> +    enum pwexpire type;
>> +    void *time_fmt;
>> +};
>> +
>> +static void f(void *in, void *_out)
>> +{
>> +    errno_t ret;
>> +    struct indata *data = (struct indata*) in;
>> +
>> +    ret = check_pwexpire_policy(data->type, data->time_fmt, NULL, 0);
>> +    *(errno_t*)_out = ret;
>> +}
>> +
> I know the tests are usually a punk rock area but let's use better names
> than "f" and "indata", please.
>
>>  From ea7a5b02856b852d89efacf33c2bcf2332036901 Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Mon, 16 Mar 2015 06:38:43 -0400
>> Subject: [PATCH 2/3] ldap: refactor check_pwexpire_kerberos to use util func
> ACK
>
>
>>  From 43cf5bcaadbaedea3280462b736c78a0744b3e9e Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Mon, 16 Mar 2015 06:51:37 -0400
>> Subject: [PATCH 3/3] ldap: refactor nds_check_expired to use util func
> ACK
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list