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

Jan Zelený jzeleny at redhat.com
Tue Jul 31 11:16:41 UTC 2012


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
-------------- 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/8c56c6e4/attachment.sig>


More information about the sssd-devel mailing list