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

Jan Zelený jzeleny at redhat.com
Tue Jul 31 12:15:27 UTC 2012


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120731/fedbe530/attachment.sig>


More information about the sssd-devel mailing list