[SSSD] [PATCH] Added unit test for sysdb_ssh.c

Michal Zidek mzidek at redhat.com
Tue Jul 31 15:35:47 UTC 2012


On 07/31/2012 05:01 PM, Jakub Hrozek wrote:
> On Tue, Jul 31, 2012 at 02:15:27PM +0200, Jan Zelený wrote:
>> Dne úterý 31 července 2012 14:06:57, Michal Zidek napsal(a):
>>> On 07/31/2012 01:16 PM, Jan Zelený wrote:
>>>> Adding sssd-devel list back to CC
>>>>
>>>> Dne čtvrtek 26 července 2012 18:14:40, Michal Zidek napsal(a):
>>>>> On 07/26/2012 02:25 PM, Jan Zelený wrote:
>>>>>> Dne středa 25 července 2012 13:21:01, Michal Zidek napsal(a):
>>>>>>> Unit test for src/db/sysdb_ssh.c
>>>>>> Nack,
>>>>>> you are cleaning the directory TESTS_PATH but what if the directory
>>>>>> existed
>>>>>> (and maybe wasn't empty) before? The least you should do is to exclude
>>>>>> the
>>>>>> return code from ret variable.
>>>>> The TESTS_PATH directory is created only for this unit test needs (it is
>>>>> actually tests_sysdb_ssh
>>>>> directory). If this directory existed before (unlikely, but possible)
>>>>> and contains files other than
>>>>> tests_conf.ldb and sssd.ldb, then those will remain untouched (rmdir
>>>>> syscall only removes empty
>>>>> directories). However, if the file existed before and contained files
>>>>> with the same names, then these
>>>>> will be overwritten, but I don't think this is a big problem because
>>>>> such files are probably only garbage
>>>>> from previous unfinished tests where the clean_up function was not
>>>>> called (because of segfault or
>>>>> something). And unlink and rmdir only returns 0 or -1, so  I think it is
>>>>> OK to test them the way I did (it
>>>>> saves some lines of code).
>>>> Ok, I have no big problem with this.
>>>>
>>>>>> Please use the SSSD_MAIN_OPTS macro in the variable long_options instead
>>>>>> defining the debug-level manually there?
>>>>>>
>>>>>> An important code-style comment: please declare all variables at the
>>>>>> beginning of functions. There are only a handful exceptions of this rule
>>>>>> in the code.
>>>>>>
>>>>>> Nitpick: you are missing space before SYSDB_OBJECTCLASS in
>>>>>> store_one_host_test()
>>>>>>
>>>>>> Consider checking results of all functions (for example function like
>>>>>> talloc_strdup(), talloc_zero(), sysdb_attrs_add_string(), ...) or at
>>>>>> least
>>>>>> those on which something else is depending (like allocation of
>>>>>> structures)
>>>>>> so we don't get segfaults. I'm not stating this as a hard requirement
>>>>>> sice we don't have it in most of places in sysdb test suite, but it
>>>>>> would
>>>>>> be nice to make the coding style a little bit more defensive.
>>>>>>
>>>>>> In delete_nonexisting_host_test():
>>>>>> nonexisting -> nonexistent
>>>>>> Deleting -> Deletion
>>>>>>
>>>>>>
>>>>>> Other than that the patch is ok.
>>>>>>
>>>>>> Thanks
>>>>>> Jan
>>>>> Thanks. New patch attached.
>>>> Just two more minor things:
>>>>
>>>> src/tests/sysdb_ssh-tests.c: In function ‘main’:
>>>> src/tests/sysdb_ssh-tests.c:409:9: warning: unused variable ‘debug’
>>>> [-Wunused- variable]
>>>>
>>>> Bad indentation level on line 285 of the patch.
>>>>
>>>> Thanks
>>>> Jan
>>> Fixed. New patch attached.
>> Ack
>>
>> Jan
> Can you also modify the Makefile.am hunk so that it only included the
> neccessary files?
Done. New patch attached.

Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-unit-test-for-sysdb_ssh.c.patch
Type: text/x-patch
Size: 13207 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/3643a7ee/attachment.bin>


More information about the sssd-devel mailing list