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?