Hi All,
This is Abhishek Kumar Singh. I want to apply for Gsoc this year. I have sound Knowledge of C, C++, Python, bash script, pytest, unit testing framework, code coverage. Currently am learning cmocka unit testing
I will start with the idea "SSSD Test Suite Coverage" as a part of my Gsoc 2013 Proposal, which includes writing unit tests for SSSD using cmocka library. I had a discussion with Jakub Hrozek(jhrozek) who is my mentor. Please guide me through your valuable suggestions.
Thanks, Abhishek
On Thu, Feb 28, 2013 at 01:17:41AM +0530, Abhishek Singh wrote:
Hi All,
This is Abhishek Kumar Singh. I want to apply for Gsoc this year. I have sound Knowledge of C, C++, Python, bash script, pytest, unit testing framework, code coverage. Currently am learning cmocka unit testing
I will start with the idea "SSSD Test Suite Coverage" as a part of my Gsoc 2013 Proposal, which includes writing unit tests for SSSD using cmocka library. I had a discussion with Jakub Hrozek(jhrozek) who is my mentor. Please guide me through your valuable suggestions.
Thanks, Abhishek
Hi Abhishek!
We've talked with Abhishek on #sssd on IRC and drafted a rough plan on what the GSoC work might look like: https://fedorahosted.org/sssd/wiki/DesignDocs/TestCoverage
Currently Abhishek is looking at cmocka and started looking at SSSD from user point of view - how to set it up etc.
The result of his work should be a better test coverage.
Welcome aboard Abhishek and good luck!
Hi Jackub,
May be you can include the attached file here http://jhrozek.fedorapeople.org/cmocka/ It's an example showing complete mocking of rect.c interface.
thanks, Abhishek
On Thu, Feb 28, 2013 at 3:10 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Thu, Feb 28, 2013 at 01:17:41AM +0530, Abhishek Singh wrote:
Hi All,
This is Abhishek Kumar Singh. I want to apply for Gsoc this year. I have sound Knowledge of C, C++, Python, bash script, pytest, unit testing framework, code coverage. Currently am learning cmocka unit testing
I will start with the idea "SSSD Test Suite Coverage" as a part of my
Gsoc
2013 Proposal, which includes writing unit tests for SSSD using cmocka library. I had a discussion with Jakub Hrozek(jhrozek) who is my mentor. Please guide me through your valuable suggestions.
Thanks, Abhishek
Hi Abhishek!
We've talked with Abhishek on #sssd on IRC and drafted a rough plan on what the GSoC work might look like: https://fedorahosted.org/sssd/wiki/DesignDocs/TestCoverage
Currently Abhishek is looking at cmocka and started looking at SSSD from user point of view - how to set it up etc.
The result of his work should be a better test coverage.
Welcome aboard Abhishek and good luck! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jackub,
I have attached cmocka test for 'find_uid.c' and coverage report. Kindly review it.
thanks, Abhishek
On Wed, Mar 6, 2013 at 1:36 AM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
Hi Jackub,
May be you can include the attached file here http://jhrozek.fedorapeople.org/cmocka/ It's an example showing complete mocking of rect.c interface.
thanks, Abhishek
On Thu, Feb 28, 2013 at 3:10 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Thu, Feb 28, 2013 at 01:17:41AM +0530, Abhishek Singh wrote:
Hi All,
This is Abhishek Kumar Singh. I want to apply for Gsoc this year. I have sound Knowledge of C, C++, Python, bash script, pytest, unit testing framework, code coverage. Currently am learning cmocka unit testing
I will start with the idea "SSSD Test Suite Coverage" as a part of my
Gsoc
2013 Proposal, which includes writing unit tests for SSSD using cmocka library. I had a discussion with Jakub Hrozek(jhrozek) who is my mentor. Please guide me through your valuable suggestions.
Thanks, Abhishek
Hi Abhishek!
We've talked with Abhishek on #sssd on IRC and drafted a rough plan on what the GSoC work might look like: https://fedorahosted.org/sssd/wiki/DesignDocs/TestCoverage
Currently Abhishek is looking at cmocka and started looking at SSSD from user point of view - how to set it up etc.
The result of his work should be a better test coverage.
Welcome aboard Abhishek and good luck! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi Jackub,
I have attached a patch. Kindly check it.
thanks,
On Mon, Mar 18, 2013 at 6:23 PM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
Hi Jackub,
I have attached cmocka test for 'find_uid.c' and coverage report. Kindly review it.
thanks, Abhishek
On Wed, Mar 6, 2013 at 1:36 AM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
Hi Jackub,
May be you can include the attached file here http://jhrozek.fedorapeople.org/cmocka/ It's an example showing complete mocking of rect.c interface.
thanks, Abhishek
On Thu, Feb 28, 2013 at 3:10 PM, Jakub Hrozek jhrozek@redhat.com wrote:
On Thu, Feb 28, 2013 at 01:17:41AM +0530, Abhishek Singh wrote:
Hi All,
This is Abhishek Kumar Singh. I want to apply for Gsoc this year. I
have
sound Knowledge of C, C++, Python, bash script, pytest, unit testing framework, code coverage. Currently am learning cmocka unit testing
I will start with the idea "SSSD Test Suite Coverage" as a part of my
Gsoc
2013 Proposal, which includes writing unit tests for SSSD using cmocka library. I had a discussion with Jakub Hrozek(jhrozek) who is my
mentor.
Please guide me through your valuable suggestions.
Thanks, Abhishek
Hi Abhishek!
We've talked with Abhishek on #sssd on IRC and drafted a rough plan on what the GSoC work might look like: https://fedorahosted.org/sssd/wiki/DesignDocs/TestCoverage
Currently Abhishek is looking at cmocka and started looking at SSSD from user point of view - how to set it up etc.
The result of his work should be a better test coverage.
Welcome aboard Abhishek and good luck! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/18/2013 09:17 AM, Abhishek Singh wrote:
Hi Jackub,
I have attached a patch. Kindly check it.
thanks,
An excellent start, Abhishek!
I have a few comments (mostly style):
Please make sure to maintain the same whitespace that was used in the original code you modify. For example, you added tabs in the HAVE_CMOCKA section of Makefile.am where the other code is indented with spaces. Also, you have trailing whitespace after --coverage in the _CFLAGS section, as well as two lines in test_check_if_uid_is_active_fail() that consist of nothing but whitespace. Many of us use editors that warn of such things with red overlays. If you're using VIM, I can recommend a .vimrc setting that will display them to you as well.
Also, in main(), when you're creating the list of tests, it would be more readable in the form:
const UnitTest tests[] = { unit_test(test_check_if_uid_is_active_success), unit_test(test_check_if_uid_is_active_fail), unit_test(test_get_uid_table) };
When I first glanced at it, I didn't notice the _fail test because it was on the same line as the _success test.
All tests pass when run and it builds without warnings.
Changes done.
My public repo of sssd : https://github.com/AbhishekKumarSingh/sssd
On Mon, Mar 18, 2013 at 7:02 PM, Stephen Gallagher sgallagh@redhat.comwrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/18/2013 09:17 AM, Abhishek Singh wrote:
Hi Jackub,
I have attached a patch. Kindly check it.
thanks,
An excellent start, Abhishek!
I have a few comments (mostly style):
Please make sure to maintain the same whitespace that was used in the original code you modify. For example, you added tabs in the HAVE_CMOCKA section of Makefile.am where the other code is indented with spaces. Also, you have trailing whitespace after --coverage in the _CFLAGS section, as well as two lines in test_check_if_uid_is_active_fail() that consist of nothing but whitespace. Many of us use editors that warn of such things with red overlays. If you're using VIM, I can recommend a .vimrc setting that will display them to you as well.
Also, in main(), when you're creating the list of tests, it would be more readable in the form:
const UnitTest tests[] = { unit_test(test_check_if_uid_is_active_success), unit_test(test_check_if_uid_is_active_fail), unit_test(test_get_uid_table) };
When I first glanced at it, I didn't notice the _fail test because it was on the same line as the _success test.
All tests pass when run and it builds without warnings. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlFHF28ACgkQeiVVYja6o6MSsQCgm6rpXpiKTCrf/Y4JYLQfClTY kKkAnRdupgXGNusltLWZkVII2nVaOvxk =ZKm1 -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, 2013-03-19 at 00:31 +0530, Abhishek Singh wrote:
Changes done.
My public repo of sssd : https://github.com/AbhishekKumarSingh/sssd
Thanks Abhishek, in the futurte can you send the patch attached as type text/plain, apparently you are sending it as application/octet-stream and this break our patchwork instance as it is not able to recognize your patches.
I will try to see if I can fix this issue in patchwork, but in the meanwhile it would be appreciated if you can turn off sending html and send the attachment as text/plain.
Simo.
On Mon, Mar 18, 2013 at 7:02 PM, Stephen Gallagher sgallagh@redhat.com wrote: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/18/2013 09:17 AM, Abhishek Singh wrote: > Hi Jackub, > > I have attached a patch. Kindly check it. > > thanks, > An excellent start, Abhishek! I have a few comments (mostly style): Please make sure to maintain the same whitespace that was used in the original code you modify. For example, you added tabs in the HAVE_CMOCKA section of Makefile.am where the other code is indented with spaces. Also, you have trailing whitespace after --coverage in the _CFLAGS section, as well as two lines in test_check_if_uid_is_active_fail() that consist of nothing but whitespace. Many of us use editors that warn of such things with red overlays. If you're using VIM, I can recommend a .vimrc setting that will display them to you as well. Also, in main(), when you're creating the list of tests, it would be more readable in the form: const UnitTest tests[] = { unit_test(test_check_if_uid_is_active_success), unit_test(test_check_if_uid_is_active_fail), unit_test(test_get_uid_table) }; When I first glanced at it, I didn't notice the _fail test because it was on the same line as the _success test. All tests pass when run and it builds without warnings. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlFHF28ACgkQeiVVYja6o6MSsQCgm6rpXpiKTCrf/Y4JYLQfClTY kKkAnRdupgXGNusltLWZkVII2nVaOvxk =ZKm1 -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Mar 19, 2013 at 12:31:09AM +0530, Abhishek Singh wrote:
Changes done.
My public repo of sssd : https://github.com/AbhishekKumarSingh/sssd
Hi Abhishek,
this is much better, thank you!
I only have two minor style nitpicks now: 1) void test_check_if_uid_is_active_success(void ** state) We usually don't put a space after the asterisk that denotes a pointer and the variable name. In other words, we would have used: "void **state"
2) In main(), I think it would be more readable to put a tab (in my .vimrc a tab expands to 4 spaces) before names of the tests. So this block: 96 int main(void) 97 { 98 const UnitTest tests[] = { 99 unit_test(test_check_if_uid_is_active_success), 100 unit_test(test_check_if_uid_is_active_fail), 101 unit_test(test_get_uid_table) 102 };
Would become this: 96 int main(void) 97 { 98 const UnitTest tests[] = { 99 unit_test(test_check_if_uid_is_active_success), 100 unit_test(test_check_if_uid_is_active_fail), 101 unit_test(test_get_uid_table) 102 };
Otherwise the test looks very good to me. Compiles and runs just fine. There's no need to send the patches to the list if you fix these, just push the changes to your local repo and I'll check them out.
For the record, on IRC we agreed that Abhishek's next step might be to take a look at one of the modules in the src/utils subdirectory that are lacking tests. The advantage is that those modules are just utility functions that are fully synchronous so he wouldn't have to worry about tevent (yet :-)). Two candidates that come to mind are src/util/backup_file.c and src/util/io.c
Great work so far Abhishek!
Hi,
I have attached a patch that contains cmocka unittest for io.c . Kindly review it.
thanks
On Tue, Mar 19, 2013 at 12:52 AM, Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Mar 19, 2013 at 12:31:09AM +0530, Abhishek Singh wrote:
Changes done.
My public repo of sssd : https://github.com/AbhishekKumarSingh/sssd
Hi Abhishek,
this is much better, thank you!
I only have two minor style nitpicks now:
- void test_check_if_uid_is_active_success(void ** state)
We usually don't put a space after the asterisk that denotes a pointer and the variable name. In other words, we would have used: "void **state"
- In main(), I think it would be more readable to put a tab (in my
.vimrc a tab expands to 4 spaces) before names of the tests. So this block: 96 int main(void) 97 { 98 const UnitTest tests[] = { 99 unit_test(test_check_if_uid_is_active_success), 100 unit_test(test_check_if_uid_is_active_fail), 101 unit_test(test_get_uid_table) 102 };
Would become this: 96 int main(void) 97 { 98 const UnitTest tests[] = { 99 unit_test(test_check_if_uid_is_active_success), 100 unit_test(test_check_if_uid_is_active_fail), 101 unit_test(test_get_uid_table) 102 };
Otherwise the test looks very good to me. Compiles and runs just fine. There's no need to send the patches to the list if you fix these, just push the changes to your local repo and I'll check them out.
For the record, on IRC we agreed that Abhishek's next step might be to take a look at one of the modules in the src/utils subdirectory that are lacking tests. The advantage is that those modules are just utility functions that are fully synchronous so he wouldn't have to worry about tevent (yet :-)). Two candidates that come to mind are src/util/backup_file.c and src/util/io.c
Great work so far Abhishek! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Fri 22 Mar 2013 04:32:52 AM EDT, Abhishek Singh wrote:
Hi,
I have attached a patch that contains cmocka unittest for io.c . Kindly review it.
This is a good start, but I'd like to see some more comprehensive tests. In the success tests, could you please use fcntl(F_GETFD) to confirm that the file descriptor actually has the correct flags? You can take a look at the sss_open_cloexec() function implementation for inspiration here.
New tests added to success tests. Please review it.
Thanks,
On Fri, Mar 22, 2013 at 4:48 PM, Stephen Gallagher sgallagh@redhat.comwrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Fri 22 Mar 2013 04:32:52 AM EDT, Abhishek Singh wrote:
Hi,
I have attached a patch that contains cmocka unittest for io.c . Kindly review it.
This is a good start, but I'd like to see some more comprehensive tests. In the success tests, could you please use fcntl(F_GETFD) to confirm that the file descriptor actually has the correct flags? You can take a look at the sss_open_cloexec() function implementation for inspiration here. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlFMPg0ACgkQeiVVYja6o6MHsACfTSehzUmCKYeRVmFeBnOKFUco dJIAnj6CGoKH3psZ97dchvTil5jwEIvE =hEyx -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/22/2013 02:33 PM, Abhishek Singh wrote:
New tests added to success tests. Please review it.
Almost :)
You can't just test for integer equivalence, because it's conceivable that there may be other flags returned beyond just the FD_CLOEXEC flag.
You need to do: assert_true(ret_flag & FD_CLOEXEC)
The bitwise comparison will ensure that it's non-zero if the FD_CLOEXEC flag is set.
changes done.
On Sat, Mar 23, 2013 at 12:07 AM, Stephen Gallagher sgallagh@redhat.comwrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/22/2013 02:33 PM, Abhishek Singh wrote:
New tests added to success tests. Please review it.
Almost :)
You can't just test for integer equivalence, because it's conceivable that there may be other flags returned beyond just the FD_CLOEXEC flag.
You need to do: assert_true(ret_flag & FD_CLOEXEC)
The bitwise comparison will ensure that it's non-zero if the FD_CLOEXEC flag is set. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlFMpO4ACgkQeiVVYja6o6OJsQCfeifrXZefJL2CfKR5qFl267XG LzwAn2AzSlodw/YfT1phEHOFuk6fTXSs =EyN7 -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
combined unit test patch for find_uid and io added.
On Sat, Mar 23, 2013 at 12:17 AM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
changes done.
On Sat, Mar 23, 2013 at 12:07 AM, Stephen Gallagher sgallagh@redhat.comwrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/22/2013 02:33 PM, Abhishek Singh wrote:
New tests added to success tests. Please review it.
Almost :)
You can't just test for integer equivalence, because it's conceivable that there may be other flags returned beyond just the FD_CLOEXEC flag.
You need to do: assert_true(ret_flag & FD_CLOEXEC)
The bitwise comparison will ensure that it's non-zero if the FD_CLOEXEC flag is set. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlFMpO4ACgkQeiVVYja6o6OJsQCfeifrXZefJL2CfKR5qFl267XG LzwAn2AzSlodw/YfT1phEHOFuk6fTXSs =EyN7 -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
patches attached.
On Sat, Mar 23, 2013 at 12:46 AM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
combined unit test patch for find_uid and io added.
On Sat, Mar 23, 2013 at 12:17 AM, Abhishek Singh < abhishekkumarsingh.cse@gmail.com> wrote:
changes done.
On Sat, Mar 23, 2013 at 12:07 AM, Stephen Gallagher sgallagh@redhat.comwrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/22/2013 02:33 PM, Abhishek Singh wrote:
New tests added to success tests. Please review it.
Almost :)
You can't just test for integer equivalence, because it's conceivable that there may be other flags returned beyond just the FD_CLOEXEC flag.
You need to do: assert_true(ret_flag & FD_CLOEXEC)
The bitwise comparison will ensure that it's non-zero if the FD_CLOEXEC flag is set. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlFMpO4ACgkQeiVVYja6o6OJsQCfeifrXZefJL2CfKR5qFl267XG LzwAn2AzSlodw/YfT1phEHOFuk6fTXSs =EyN7 -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Sat, Mar 23, 2013 at 01:49:26AM +0530, Abhishek Singh wrote:
patches attached.
Hello Abhishek,
in general the tests look good to me, thank you! I only have a couple of nitpicks now. See inline.
From 2f3281a7b0af9f4a5f53dec0b911b024251ad828 Mon Sep 17 00:00:00 2001 From: Abhishek Singh abhishekkumarsingh.cse@gmail.com Date: Sat, 23 Mar 2013 01:42:48 +0530 Subject: [PATCH] cmocka unittest for io added
Makefile.am | 13 +++- src/tests/cmocka/test_io.c | 148 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 src/tests/cmocka/test_io.c
diff --git a/Makefile.am b/Makefile.am index aa79ae6fbd637c7b1301f37130e729013602ff0d..fa906e91542764403e926200d880970e8125b8d7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -147,7 +147,8 @@ endif if HAVE_CMOCKA non_interactive_cmocka_based_tests = \ nss-srv-tests \
test-find-uid
test-find-uid \
test-io
endif
check_PROGRAMS = \ @@ -1232,6 +1233,16 @@ test_find_uid_LDADD = \ $(DHASH_LIBS) \ $(CMOCKA_LIBS) \ libsss_util.la
+test_io_DEPENDENCIES = \
- $(ldblib_LTLIBRARIES)
I don't think you need to link against ldb here, you don't use the cache in your test. ldb is the library that we use as an on-disk cache for the SSSD.
+test_io_SOURCES = \
- src/tests/cmocka/test_io.c \
- src/util/io.c
+test_io_CFLAGS = \
- $(AM_CFLAGS)
+test_io_LDADD = \
- $(CMOCKA_LIBS)
endif
noinst_PROGRAMS = pam_test_client diff --git a/src/tests/cmocka/test_io.c b/src/tests/cmocka/test_io.c new file mode 100644 index 0000000000000000000000000000000000000000..bed7ef196e73e7e93879b8b659721369e6c0cbb4 --- /dev/null +++ b/src/tests/cmocka/test_io.c @@ -0,0 +1,148 @@ +/*
- SSSD
- find_uid - Utilities tests
- Authors:
Abhishek Singh <abhishekkumarsingh.cse@gmail.com>
- Copyright (C) 2013 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <stdio.h> +#include <fcntl.h> +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> +#include <dirent.h> +#include <unistd.h>
+#include "util/io.h"
+#define FILE_PATH "/tmp/test.in"
Instead of hardcoding the file in /tmp I would prefer if the test used TEST_DIR (see its usage elsewhere in the SSSD) as a base directory for the test file. FILE_PATH could then be defined as (not tested, just an idea):
#define FILE_PATH TEST_DIR"/test.in".
Maybe also instead of using test.in you could use mkstemp to cover the odd case there would be several testcases running concurrently.
+#define NON_EX_PATH "non-existent-path"
+static void setup_all(void) +{
- FILE *fp;
- fp = fopen(FILE_PATH, "w");
- if (fp != NULL)
We prefer to use brackets even for single-line statements. In other words, use:
if (fp != NULL) { fclose(fp); }
fclose(fp);
- else
printf("Error! file test.in can't be created");
I would prefer fprintf(stderr) here.
+}
+static void teardown_all(void) +{
- remove(FILE_PATH);
+}
+static int get_dirfd(void) +{
- int dir_fd;
- DIR *tmp = opendir("/tmp");
if (tmp != NULL)
dir_fd = dirfd(tmp);
This seems like a bad indentation to me. Also the directory could be a parameter instead of hardcoding it. The caller could then hardcode it to TEST_DIR, but still, I would prefer a parameter.
- return dir_fd;
+}
+void test_sss_open_cloexec_success(void **state) +{
- int fd;
- int ret;
- int ret_flag;
- int expec_flag;
- int flags = O_RDWR;
- fd = sss_open_cloexec(FILE_PATH, flags, &ret);
- ret_flag = fcntl(fd, F_GETFD, 0);
- expec_flag = FD_CLOEXEC;
- assert_true(fd != -1);
I think you should first test for fd validity and only then call GETFD and then test for the flag.
- assert_true(ret_flag & expec_flag);
- close(fd);
+}
+void test_sss_open_cloexec_fail(void **state) +{
- int fd;
- int ret;
- int flags = O_RDWR;
- fd = sss_open_cloexec(NON_EX_PATH, flags, &ret);
- assert_true(fd == -1);
- assert_int_not_equal(ret, 0);
- close(fd);
+}
+void test_sss_openat_cloexec_success(void **state) +{
- int fd;
- int ret;
- int ret_flag;
- int expec_flag;
- int dir_fd;
- int flags = O_RDWR;
- dir_fd = get_dirfd();
- fd = sss_openat_cloexec(dir_fd, "test.in", flags, &ret);
- ret_flag = fcntl(fd, F_GETFD, 0);
- expec_flag = FD_CLOEXEC;
- assert_true(fd != -1);
- assert_true(ret_flag & expec_flag);
- close(fd);
+}
+void test_sss_openat_cloexec_fail(void **state) +{
- int fd;
- int ret;
- int dir_fd;
- int flags = O_RDWR;
- dir_fd = get_dirfd();
- fd = sss_openat_cloexec(dir_fd, NON_EX_PATH, flags, &ret);
- assert_true(fd == -1);
- assert_int_not_equal(ret, 0);
- close(fd);
+}
+int main(void) +{
- const UnitTest tests[] = {
unit_test_setup_teardown(test_sss_open_cloexec_success, setup_all,
teardown_all),
unit_test_setup_teardown(test_sss_open_cloexec_fail, setup_all,
teardown_all),
unit_test_setup_teardown(test_sss_openat_cloexec_success, setup_all,
teardown_all),
unit_test_setup_teardown(test_sss_openat_cloexec_fail, setup_all,
teardown_all)
- };
- return run_tests(tests);
+}
1.8.1.4
The rest looks good to me.
From b9f90d1313b91fb7213b006b4efaaf6c50568e26 Mon Sep 17 00:00:00 2001 From: Abhishek Singh abhishekkumarsingh.cse@gmail.com Date: Sat, 23 Mar 2013 01:47:23 +0530 Subject: [PATCH] filename in comment is corrected
src/util/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/io.c b/src/util/io.c index 4d1f2e74a1607bf7f6a8977ca97455185eba7403..7f979c941d11e78d409f6a9c0f06dd98a29be6e4 100644 --- a/src/util/io.c +++ b/src/util/io.c @@ -1,7 +1,7 @@ /* SSSD
- compact.c
io.c
Authors: Lukas Slebodnik lslebodn@redhat.com
-- 1.8.1.4
Ack to this patch, I will push this one out-of band.
On Tue, Mar 26, 2013 at 10:39:42PM +0100, Jakub Hrozek wrote:
From b9f90d1313b91fb7213b006b4efaaf6c50568e26 Mon Sep 17 00:00:00 2001 From: Abhishek Singh abhishekkumarsingh.cse@gmail.com Date: Sat, 23 Mar 2013 01:47:23 +0530 Subject: [PATCH] filename in comment is corrected
src/util/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/io.c b/src/util/io.c index 4d1f2e74a1607bf7f6a8977ca97455185eba7403..7f979c941d11e78d409f6a9c0f06dd98a29be6e4 100644 --- a/src/util/io.c +++ b/src/util/io.c @@ -1,7 +1,7 @@ /* SSSD
- compact.c
io.c
Authors: Lukas Slebodnik lslebodn@redhat.com
-- 1.8.1.4
Ack to this patch, I will push this one out-of band.
Pushed this one to master.
On Wed, Mar 27, 2013 at 09:39:53AM +0100, Jakub Hrozek wrote:
On Tue, Mar 26, 2013 at 10:39:42PM +0100, Jakub Hrozek wrote:
From b9f90d1313b91fb7213b006b4efaaf6c50568e26 Mon Sep 17 00:00:00 2001 From: Abhishek Singh abhishekkumarsingh.cse@gmail.com Date: Sat, 23 Mar 2013 01:47:23 +0530 Subject: [PATCH] filename in comment is corrected
src/util/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/io.c b/src/util/io.c index 4d1f2e74a1607bf7f6a8977ca97455185eba7403..7f979c941d11e78d409f6a9c0f06dd98a29be6e4 100644 --- a/src/util/io.c +++ b/src/util/io.c @@ -1,7 +1,7 @@ /* SSSD
- compact.c
io.c
Authors: Lukas Slebodnik lslebodn@redhat.com
-- 1.8.1.4
Ack to this patch, I will push this one out-of band.
Pushed this one to master.
One thing I noted when pushing the patch -- you can keep the default .patch suffix for the patch files, that's completely OK.
On Wed, 2013-03-27 at 10:25 +0100, Jakub Hrozek wrote:
On Wed, Mar 27, 2013 at 09:39:53AM +0100, Jakub Hrozek wrote:
On Tue, Mar 26, 2013 at 10:39:42PM +0100, Jakub Hrozek wrote:
From b9f90d1313b91fb7213b006b4efaaf6c50568e26 Mon Sep 17 00:00:00 2001 From: Abhishek Singh abhishekkumarsingh.cse@gmail.com Date: Sat, 23 Mar 2013 01:47:23 +0530 Subject: [PATCH] filename in comment is corrected
src/util/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/io.c b/src/util/io.c index 4d1f2e74a1607bf7f6a8977ca97455185eba7403..7f979c941d11e78d409f6a9c0f06dd98a29be6e4 100644 --- a/src/util/io.c +++ b/src/util/io.c @@ -1,7 +1,7 @@ /* SSSD
- compact.c
io.c
Authors: Lukas Slebodnik lslebodn@redhat.com
-- 1.8.1.4
Ack to this patch, I will push this one out-of band.
Pushed this one to master.
One thing I noted when pushing the patch -- you can keep the default .patch suffix for the patch files, that's completely OK.
I guess Abhishek is adding the .txt as I asked him to send the patch in plain/text and that may be the only workaround with gmail ?
Just guessing, but it works, all his renamed patches made it to patchwork, while the first ones did not.
Simo.
Hi Jakub,
I have done requested changes and have attached the patch with this mail. Kindly review it.
thanks,
On Wed, Mar 27, 2013 at 6:37 PM, Simo Sorce simo@redhat.com wrote:
On Wed, 2013-03-27 at 10:25 +0100, Jakub Hrozek wrote:
On Wed, Mar 27, 2013 at 09:39:53AM +0100, Jakub Hrozek wrote:
On Tue, Mar 26, 2013 at 10:39:42PM +0100, Jakub Hrozek wrote:
From b9f90d1313b91fb7213b006b4efaaf6c50568e26 Mon Sep 17 00:00:00
2001
From: Abhishek Singh abhishekkumarsingh.cse@gmail.com Date: Sat, 23 Mar 2013 01:47:23 +0530 Subject: [PATCH] filename in comment is corrected
src/util/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/io.c b/src/util/io.c index
4d1f2e74a1607bf7f6a8977ca97455185eba7403..7f979c941d11e78d409f6a9c0f06dd98a29be6e4 100644
--- a/src/util/io.c +++ b/src/util/io.c @@ -1,7 +1,7 @@ /* SSSD
- compact.c
io.c
Authors: Lukas Slebodnik lslebodn@redhat.com
-- 1.8.1.4
Ack to this patch, I will push this one out-of band.
Pushed this one to master.
One thing I noted when pushing the patch -- you can keep the default .patch suffix for the patch files, that's completely OK.
I guess Abhishek is adding the .txt as I asked him to send the patch in plain/text and that may be the only workaround with gmail ?
Just guessing, but it works, all his renamed patches made it to patchwork, while the first ones did not.
Simo.
-- Simo Sorce * Red Hat, Inc * New York
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Apr 04, 2013 at 02:44:47AM +0530, Abhishek Singh wrote:
Hi Jakub,
I have done requested changes and have attached the patch with this mail. Kindly review it.
thanks,
Hi Abhishek,
this is much better and we're almost there. I only have a couple of comments before I push the patch:
1) I couldn't apply the patch cleanly. Can you make sure that the patches you send to the list can be applied on top of origin/master? In most cases, all you have to do is continuously rebase using "git rebase origin/master" and fix the conflicts.
If there are any patches that needs to be applied along with this one, feel free to send them along in the same e-mail as another attachment.
2) because I couldn't apply the patch I only did visual review. I have one nitpick:
+static char *get_filepath(char path[23]) +{ + int ret; + + strncpy(path, FILE_PATH, 23);
I'm not fond of the magic constant "23" used here. I think it would be better to either declare that path is always PATH_MAX long or pass its size as additional parameter and declare path as "char *".
3) The get_dirfd() function assigns a DIR pointer but never calls closedir() to free this resource. It's not a big deal in tests, but we should still try to make the code as good as possible.
I think in this case actually a "fixture" might be a good idea. A fixture in unit tests is a function that gets run before the test is executed and another after the test is done. I think for this test it might be a good idea to include a setup fixture to open the directory with opendir() and a teardown fixture to close the handle with closedir().
Feel free to use either a global variable or the **state pointer for storing the dir handle, both would work for me. If you need more information on using the cmocka fixtures, some documentation is available at: https://cmockery.googlecode.com/svn/trunk/doc/index.html#Test_State
Feel free to ping me on #sssd if you need any help.
Hi Jakub,
I have made necessary changes, please review it.
thanks,
On Fri, Apr 5, 2013 at 4:09 AM, Jakub Hrozek jhrozek@redhat.com wrote:
On Thu, Apr 04, 2013 at 02:44:47AM +0530, Abhishek Singh wrote:
Hi Jakub,
I have done requested changes and have attached the patch with this mail. Kindly review it.
thanks,
Hi Abhishek,
this is much better and we're almost there. I only have a couple of comments before I push the patch:
- I couldn't apply the patch cleanly. Can you make sure that the
patches you send to the list can be applied on top of origin/master? In most cases, all you have to do is continuously rebase using "git rebase origin/master" and fix the conflicts.
If there are any patches that needs to be applied along with this one, feel free to send them along in the same e-mail as another attachment.
- because I couldn't apply the patch I only did visual review. I have
one nitpick:
+static char *get_filepath(char path[23]) +{
- int ret;
- strncpy(path, FILE_PATH, 23);
I'm not fond of the magic constant "23" used here. I think it would be better to either declare that path is always PATH_MAX long or pass its size as additional parameter and declare path as "char *".
- The get_dirfd() function assigns a DIR pointer but never calls
closedir() to free this resource. It's not a big deal in tests, but we should still try to make the code as good as possible.
I think in this case actually a "fixture" might be a good idea. A fixture in unit tests is a function that gets run before the test is executed and another after the test is done. I think for this test it might be a good idea to include a setup fixture to open the directory with opendir() and a teardown fixture to close the handle with closedir().
Feel free to use either a global variable or the **state pointer for storing the dir handle, both would work for me. If you need more information on using the cmocka fixtures, some documentation is available at: https://cmockery.googlecode.com/svn/trunk/doc/index.html#Test_State
Feel free to ping me on #sssd if you need any help. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Apr 05, 2013 at 06:34:45PM +0530, Abhishek Singh wrote:
Hi Jakub,
I have made necessary changes, please review it.
thanks,
Hi Abhishek,
the test compiles and runs fine, the patches apply cleanly on top of one another and the teardown function is used cleanly.
I only have one last comment -- I wasn't completely clear with how I would like to see the PATH_MAX used. PATH_MAX is a constant defined in the standard header file limits.h, you don't have to define it yourself, just include limits.h.
You should also pass PATH_MAX-1 not PATH_MAX as the third argument of snprintf, otherwise you risk a off-by-one error in case the buffers had the same length.
This is the last comment I have, then we'll push the patch :)
Hi Jakub,
You should also pass PATH_MAX-1 not PATH_MAX as the third argument of snprintf, otherwise you risk a off-by-one error in case the buffers had the same length.
snprintf ? Do you mean strncpy instead of instead of snprintf? since I haven't used snprintf in my code.
On Tue, Apr 9, 2013 at 3:01 AM, Jakub Hrozek jhrozek@redhat.com wrote:
On Fri, Apr 05, 2013 at 06:34:45PM +0530, Abhishek Singh wrote:
Hi Jakub,
I have made necessary changes, please review it.
thanks,
Hi Abhishek,
the test compiles and runs fine, the patches apply cleanly on top of one another and the teardown function is used cleanly.
I only have one last comment -- I wasn't completely clear with how I would like to see the PATH_MAX used. PATH_MAX is a constant defined in the standard header file limits.h, you don't have to define it yourself, just include limits.h.
You should also pass PATH_MAX-1 not PATH_MAX as the third argument of snprintf, otherwise you risk a off-by-one error in case the buffers had the same length.
This is the last comment I have, then we'll push the patch :) _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Apr 09, 2013 at 03:13:20AM +0530, Abhishek Singh wrote:
Hi Jakub,
You should also pass PATH_MAX-1 not PATH_MAX as the third argument of snprintf, otherwise you risk a off-by-one error in case the buffers had the same length.
snprintf ? Do you mean strncpy instead of instead of snprintf? since I haven't used snprintf in my code.
Yes, strncpy. Sorry.
Hi Jakub,
Please check this patch.
On Tue, Apr 9, 2013 at 3:16 AM, Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 09, 2013 at 03:13:20AM +0530, Abhishek Singh wrote:
Hi Jakub,
You should also pass PATH_MAX-1 not PATH_MAX as the third argument of snprintf, otherwise you risk a off-by-one error in case the buffers had the same length.
snprintf ? Do you mean strncpy instead of instead of snprintf? since I haven't used snprintf in my code.
Yes, strncpy. Sorry. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Apr 09, 2013 at 03:32:04AM +0530, Abhishek Singh wrote:
Hi Jakub,
Please check this patch.
Ack with a one change I can do myself before pushing, no need to re-send:
I would prefer if the get_filepath declaration didn't hardcode the array size, but rather looked like:
static char *get_filepath(char path[])
I think just stating that get_filepath() assumes that the buffer is big enough is OK for a unit test. But in real defensive code, I would prefer if the function had a second parameter to specify the size and allow checks on it.
But as I said above, I'll fix the get_filepath declaration before pushing and then push.
Thanks for the contribution!
On Tue, Apr 09, 2013 at 12:13:44AM +0200, Jakub Hrozek wrote:
On Tue, Apr 09, 2013 at 03:32:04AM +0530, Abhishek Singh wrote:
Hi Jakub,
Please check this patch.
Ack with a one change I can do myself before pushing, no need to re-send:
I would prefer if the get_filepath declaration didn't hardcode the array size, but rather looked like:
static char *get_filepath(char path[])
I think just stating that get_filepath() assumes that the buffer is big enough is OK for a unit test. But in real defensive code, I would prefer if the function had a second parameter to specify the size and allow checks on it.
But as I said above, I'll fix the get_filepath declaration before pushing and then push.
Thanks for the contribution!
Fixed the declaration and pushed to master.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 04/10/2013 08:23 AM, Jakub Hrozek wrote:
On Tue, Apr 09, 2013 at 12:13:44AM +0200, Jakub Hrozek wrote:
On Tue, Apr 09, 2013 at 03:32:04AM +0530, Abhishek Singh wrote:
Hi Jakub,
Please check this patch.
Ack with a one change I can do myself before pushing, no need to re-send:
I would prefer if the get_filepath declaration didn't hardcode the array size, but rather looked like:
static char *get_filepath(char path[])
I think just stating that get_filepath() assumes that the buffer is big enough is OK for a unit test. But in real defensive code, I would prefer if the function had a second parameter to specify the size and allow checks on it.
But as I said above, I'll fix the get_filepath declaration before pushing and then push.
Thanks for the contribution!
Fixed the declaration and pushed to master.
Great job, Abhishek!
sssd-devel@lists.fedorahosted.org