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

Jakub Hrozek jhrozek at redhat.com
Wed Aug 1 17:54:54 UTC 2012


On Tue, Jul 31, 2012 at 05:35:47PM +0200, Michal Zidek wrote:
> 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
> 

Ack and pushed to master.



More information about the sssd-devel mailing list