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

Jakub Hrozek jhrozek at redhat.com
Tue Jul 31 15:01:02 UTC 2012


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?



More information about the sssd-devel mailing list