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
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.
Thanks Michal
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
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?
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
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.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Michal Židek