Hello,
please see simple test adding test for https://fedorahosted.org/sssd/ticket/2922. Sumit proposed to test if mapping of UNIX MAX_ID + 1 fails to be mapped to SID.
Without patch for #2922 test fails otherwise test passes.
Thanks.
On 01/22/2016 02:55 PM, Pavel Reichl wrote:
Hello,
please see simple test adding test for https://fedorahosted.org/sssd/ticket/2922. Sumit proposed to test if mapping of UNIX MAX_ID + 1 fails to be mapped to SID.
Without patch for #2922 test fails otherwise test passes.
Thanks.
Hi,
Please define TEST_2922_MAX_ID_PLUS_ONE macro like this: #define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
I asked Jakub about the asserts in setup functions and he confirmed that we should not use them in test setup functions.
So please do not use asserts in the tests but return an error code. It is OK if you only change the functions that you added, no need to remove the asserts from other setup functions that already exist in the file.
I think it is OK to use DEBUG macro with SSSDBG_FATAL_FAILURE level in the setup fuctions as well.
Michal
On 02/05/2016 03:22 PM, Michal Židek wrote:
On 01/22/2016 02:55 PM, Pavel Reichl wrote:
Hello,
please see simple test adding test for https://fedorahosted.org/sssd/ticket/2922. Sumit proposed to test if mapping of UNIX MAX_ID + 1 fails to be mapped to SID.
Without patch for #2922 test fails otherwise test passes.
Thanks.
Hi,
Please define TEST_2922_MAX_ID_PLUS_ONE macro like this: #define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
I asked Jakub about the asserts in setup functions and he confirmed that we should not use them in test setup functions.
So please do not use asserts in the tests but return an error code. It is OK if you only change the functions that you added, no need to remove the asserts from other setup functions that already exist in the file.
I think it is OK to use DEBUG macro with SSSDBG_FATAL_FAILURE level in the setup fuctions as well.
Michal
OK, thanks for comments. Please see update patch set.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c @@ -43,6 +43,17 @@ #define TEST_OFFSET 1000000 #define TEST_OFFSET_STR "1000000"
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
- }
- /* Range computation should be deterministic. Lets validate that. */
- if (range.min != TEST_2922_MIN_ID
|| range.max != TEST_2922_MAX_ID
|| slice_num != TEST_2922_DFL_SLIDE) {
DEBUG(SSSDBG_FATAL_FAILURE, "Failed to compute range correctly.\n");
return 1;
- }
- err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, &range,
NULL, 0, false /* No external mapping */);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_add_domain_ex failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
- }
- return 0;
+}
LS
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c @@ -43,6 +43,17 @@ #define TEST_OFFSET 1000000 #define TEST_OFFSET_STR "1000000"
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
I agree with the points about cmocka you made above. Maybe some future versions will address this.
Michal
On (05/02/16 17:30), Michal Židek wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c @@ -43,6 +43,17 @@ #define TEST_OFFSET 1000000 #define TEST_OFFSET_STR "1000000"
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
No it's not OK to have same return code. The return codes have to be unique. You might easily overlook as part of review that messages are the same and you will don't know where it failed. (copy & paste problem.
It's much simpler to review that return codes are unique.
LS
On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c @@ -43,6 +43,17 @@ #define TEST_OFFSET 1000000 #define TEST_OFFSET_STR "1000000"
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would prefer the strings to remain #defines, but it is not strong preference.
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
No it's not OK to have same return code. The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:00:17:723118 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:01:17:376602 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
I returned different error code in both cases but the result is the same.
You might easily overlook as part of review that messages are the same and you will don't know where it failed. (copy & paste problem.
It's much simpler to review that return codes are unique.
LS
But the DEBUG macro will tell you what function the message is from. So it does not matter that much what return code you use here. This is just a setup function for a test and all non-zero codes are treated the same way. We can use something like EINVAL if it is something with bad value or whatever we find suitable in errno.h if you think that 1 is too magic value, but since any value will be treated the same by the cmocka, it does not really matter here and returning 1 is just fine IMO. It really is just a fixture for test. The debug message is more important than the error code.
Michal
On (05/02/16 19:34), Michal Židek wrote:
On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c @@ -43,6 +43,17 @@ #define TEST_OFFSET 1000000 #define TEST_OFFSET_STR "1000000"
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would prefer the strings to remain #defines, but it is not strong preference.
I'm sorry but your preference is not type safe. It's much simpler to identify errors caused by wrong usage of constants. If you like macros then you can be honored in future to try find bugs in macro generated code (nss-pam-ldapd).
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
No it's not OK to have same return code. The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:00:17:723118 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:01:17:376602 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
I returned different error code in both cases but the result is the same.
That's because of insufficient desing of libcmocka. Unit test should not contain any if conditions which are not tested. If you run code coverage of unit test it should cover 100% of code path of unit test.
However, the "official way" of handling errors in setup and teardown function does not allow to do this. In other words, returning error code is useless. As you pointed out there's no difference and we need to use debug messages to find out where it failed. that's the worst situation. The error message and line where it happened should be logged automaticaly or thearduwn function should return unique identifier of failure.
The usage of debug message (printf error) is kind of workaround for current state. Workarounds should be removed after fixing the staff. So we will need to retun a unique error code anyway in the end and there's no reason to do it twice.
LS
On 02/05/2016 10:25 PM, Lukas Slebodnik wrote:
On (05/02/16 19:34), Michal Židek wrote:
On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c @@ -43,6 +43,17 @@ #define TEST_OFFSET 1000000 #define TEST_OFFSET_STR "1000000"
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would prefer the strings to remain #defines, but it is not strong preference.
I'm sorry but your preference is not type safe. It's much simpler to identify errors caused by wrong
Well, it's just your preference. It's not in out code style, right? Anyway it's often used in other cmocka tests so it's consistent with out code base.
usage of constants. If you like macros then you can be honored in future to try find bugs in macro generated code (nss-pam-ldapd).
I think this is totally unrelated, please try to focus on purpose of this thread - test for id mapping.
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
No it's not OK to have same return code. The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:00:17:723118 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:01:17:376602 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
I returned different error code in both cases but the result is the same.
That's because of insufficient desing of libcmocka. Unit test should not contain any if conditions which are not tested. If you run code coverage of unit test it should cover 100% of code path of unit test.
However, the "official way" of handling errors in setup and teardown function does not allow to do this. In other words, returning error code is useless. As you pointed out there's no difference and we need to use debug messages to find out where it failed. that's the worst situation. The error message and line where it happened should be logged automaticaly or thearduwn function should return unique identifier of failure.
The usage of debug message (printf error) is kind of workaround for current state. Workarounds should be removed after fixing the staff. So we will need to retun a unique error code anyway in the end and there's no reason to do it twice.
We are not working on cmocka code here and we cannot predict its evolution.
Please stop nitpicking about unrelated stuff and focus on real goal - having test for #2922 in our unit tests.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On (08/02/16 09:24), Pavel Reichl wrote:
On 02/05/2016 10:25 PM, Lukas Slebodnik wrote:
On (05/02/16 19:34), Michal Židek wrote:
On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote: >Hopefully the last one.
>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 >From: Pavel Reichl preichl@redhat.com >Date: Fri, 22 Jan 2016 08:34:14 -0500 >Subject: [PATCH] IDMAP: Add test to validate off by one bug > >Resolves: >https://fedorahosted.org/sssd/ticket/2922 >--- >src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ >1 file changed, 125 insertions(+) > >diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c >index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 >--- a/src/tests/cmocka/test_sss_idmap.c >+++ b/src/tests/cmocka/test_sss_idmap.c >@@ -43,6 +43,17 @@ >#define TEST_OFFSET 1000000 >#define TEST_OFFSET_STR "1000000" > >+#define TEST_2922_MIN_ID 1842600000 >+#define TEST_2922_MAX_ID 1842799999 >+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1) >+ >+#define TEST_2922_DFL_SLIDE 9212 >+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" >+/* Last SID = first SID + (default) rangesize -1 */ >+#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" >+/* Last SID = first SID + rangesize */ >+#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000" >+ We should prefer to use constants instead of #defines. Constants are more type safe.
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would prefer the strings to remain #defines, but it is not strong preference.
I'm sorry but your preference is not type safe. It's much simpler to identify errors caused by wrong
Well, it's just your preference. It's not in out code style, right? Anyway it's often used in other cmocka tests so it's consistent with out code base.
It's not just my preference constants are already used in test (module scope)
sh$ cd src/tests sh$ git grep "^const" | wc -l 64
Could you give me any reason why macros are better from technical point of view? We needn't support sssd on platforms with old C compiler and we use c99(gnu99) anyway.
If no then I will appreciate usage of new-style constants.
usage of constants. If you like macros then you can be honored in future to try find bugs in macro generated code (nss-pam-ldapd).
I think this is totally unrelated, please try to focus on purpose of this thread - test for id mapping.
It just an explanation of my reasoning.
>struct test_ctx { > TALLOC_CTX *mem_idmap; > struct sss_idmap_ctx *idmap_ctx; >@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, > return 0; >} > >+static int setup_ranges_2922(struct test_ctx *test_ctx) >+{ >+ struct sss_idmap_range range; >+ enum idmap_error_code err; >+ const char *name; >+ const char *sid; >+ /* Pick a new slice. */ >+ id_t slice_num = -1; >+ >+ if (test_ctx == NULL) { >+ DEBUG(SSSDBG_FATAL_FAILURE, >+ "test context is NULL\n"); >+ return 1; >+ } >+ >+ name = TEST_DOM_NAME; >+ sid = TEST_DOM_SID; >+ >+ err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num, >+ &range); >+ if (err != IDMAP_SUCCESS) { >+ DEBUG(SSSDBG_FATAL_FAILURE, >+ "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n", >+ IDMAP_SUCCESS, err); >+ return 1; Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
No it's not OK to have same return code. The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:00:17:723118 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:01:17:376602 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
I returned different error code in both cases but the result is the same.
That's because of insufficient desing of libcmocka. Unit test should not contain any if conditions which are not tested. If you run code coverage of unit test it should cover 100% of code path of unit test.
However, the "official way" of handling errors in setup and teardown function does not allow to do this. In other words, returning error code is useless. As you pointed out there's no difference and we need to use debug messages to find out where it failed. that's the worst situation. The error message and line where it happened should be logged automaticaly or thearduwn function should return unique identifier of failure.
The usage of debug message (printf error) is kind of workaround for current state. Workarounds should be removed after fixing the staff. So we will need to retun a unique error code anyway in the end and there's no reason to do it twice.
We are not working on cmocka code here and we cannot predict its evolution.
On the other hand, this patch introduced the new style of error handling in fixtures (setup and teardown functions). This code will be seen as a precedence in future because we do not have a coding guidelines for this. So it would be better to do it properly.
BTW I do not understand few things. You want to introduce new-style of fixtures which has never been used in sssd but you do not want to use new-style of constants which are already used in tests.
Please stop nitpicking about unrelated stuff and focus on real goal - having test for #2922 in our unit tests.
If I was a nitpicer in this case I would ask to change the coding style issues. e.g.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c
//snip
@@ -140,6 +198,23 @@ static int test_sss_idmap_setup_with_domains(void **state) { return 0; }
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
^^^ should be on new line
- struct test_ctx *test_ctx;
- int ret;
- test_sss_idmap_setup(state);
- test_ctx = talloc_get_type(*state, struct test_ctx);
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- ret = setup_ranges_2922(test_ctx);
- return ret;
+}
If you think that usage of new-style error handling in fixtures should not block this patch then please use old-style. In another words, please use old-style of everything or new-style of everything.
It's up to you which way you will choose.
LS
On 02/09/2016 08:58 AM, Lukas Slebodnik wrote:
On (08/02/16 09:24), Pavel Reichl wrote:
On 02/05/2016 10:25 PM, Lukas Slebodnik wrote:
On (05/02/16 19:34), Michal Židek wrote:
On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote: > On (05/02/16 16:56), Pavel Reichl wrote: >> Hopefully the last one. > > >From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 >> From: Pavel Reichl preichl@redhat.com >> Date: Fri, 22 Jan 2016 08:34:14 -0500 >> Subject: [PATCH] IDMAP: Add test to validate off by one bug >> >> Resolves: >> https://fedorahosted.org/sssd/ticket/2922 >> --- >> src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 125 insertions(+) >> >> diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c >> index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 >> --- a/src/tests/cmocka/test_sss_idmap.c >> +++ b/src/tests/cmocka/test_sss_idmap.c >> @@ -43,6 +43,17 @@ >> #define TEST_OFFSET 1000000 >> #define TEST_OFFSET_STR "1000000" >> >> +#define TEST_2922_MIN_ID 1842600000 >> +#define TEST_2922_MAX_ID 1842799999 >> +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1) >> + >> +#define TEST_2922_DFL_SLIDE 9212 >> +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" >> +/* Last SID = first SID + (default) rangesize -1 */ >> +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" >> +/* Last SID = first SID + rangesize */ >> +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000" >> + > We should prefer to use constants instead of #defines. > Constants are more type safe. >
Do you want Pavel to change it? If so, then I would prefer at least the strings to remain as #defines. It is more comfortable to concatenate string literals. But honestly I am ok with the numbers to remain as #defines as well, but I agree with your point in general.
Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would prefer the strings to remain #defines, but it is not strong preference.
I'm sorry but your preference is not type safe. It's much simpler to identify errors caused by wrong
Well, it's just your preference. It's not in out code style, right? Anyway it's often used in other cmocka tests so it's consistent with out code base.
It's not just my preference constants are already used in test (module scope)
sh$ cd src/tests sh$ git grep "^const" | wc -l 64
grep '#define' `find src/tests -name '*.c'` | grep " | wc -l 237
Could you give me any reason why macros are better from technical point of view?
It doesn't matter. Both work and we are wasting time on fruitless discussion. You can always improve code. It's import to know when it's good enough. I say using defines in test is good enough and I don't see real benefit in changing it that would justify time spend on fixing.
We needn't support sssd on platforms with old C compiler and we use c99(gnu99) anyway.
If no then I will appreciate usage of new-style constants.
usage of constants. If you like macros then you can be honored in future to try find bugs in macro generated code (nss-pam-ldapd).
I think this is totally unrelated, please try to focus on purpose of this thread - test for id mapping.
It just an explanation of my reasoning.
> > >> struct test_ctx { >> TALLOC_CTX *mem_idmap; >> struct sss_idmap_ctx *idmap_ctx; >> @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, >> return 0; >> } >> >> +static int setup_ranges_2922(struct test_ctx *test_ctx) >> +{ >> + struct sss_idmap_range range; >> + enum idmap_error_code err; >> + const char *name; >> + const char *sid; >> + /* Pick a new slice. */ >> + id_t slice_num = -1; >> + >> + if (test_ctx == NULL) { >> + DEBUG(SSSDBG_FATAL_FAILURE, >> + "test context is NULL\n"); >> + return 1; >> + } >> + >> + name = TEST_DOM_NAME; >> + sid = TEST_DOM_SID; >> + >> + err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num, >> + &range); >> + if (err != IDMAP_SUCCESS) { >> + DEBUG(SSSDBG_FATAL_FAILURE, >> + "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n", >> + IDMAP_SUCCESS, err); >> + return 1; > Please use different numbers for different failures.
I think it is OK to use the same number in tests, as long as the debug message is present. We can identify the error based on stderr messages.
No it's not OK to have same return code. The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:00:17:723118 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:01:17:376602 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
I returned different error code in both cases but the result is the same.
That's because of insufficient desing of libcmocka. Unit test should not contain any if conditions which are not tested. If you run code coverage of unit test it should cover 100% of code path of unit test.
However, the "official way" of handling errors in setup and teardown function does not allow to do this. In other words, returning error code is useless. As you pointed out there's no difference and we need to use debug messages to find out where it failed. that's the worst situation. The error message and line where it happened should be logged automaticaly or thearduwn function should return unique identifier of failure.
The usage of debug message (printf error) is kind of workaround for current state. Workarounds should be removed after fixing the staff. So we will need to retun a unique error code anyway in the end and there's no reason to do it twice.
We are not working on cmocka code here and we cannot predict its evolution.
On the other hand, this patch introduced the new style of error handling in fixtures (setup and teardown functions). This code will be seen as a precedence in future because we do not have a coding guidelines for this. So it would be better to do it properly.
Well, that's the problem - you think your way is proper and I and Michal don't share it.
BTW I do not understand few things. You want to introduce new-style of fixtures which has never been used in sssd but you do not want to use new-style of constants which are already used in tests.
You are overthinking this, I'm merely adding test for a bug and I want to do it quickly and properly. I'm not inventing anything and I have no intention for my code to be used as example.
Please stop nitpicking about unrelated stuff and focus on real goal - having test for #2922 in our unit tests.
If I was a nitpicer in this case I would ask to change the coding style issues. e.g.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Fri, 22 Jan 2016 08:34:14 -0500 Subject: [PATCH] IDMAP: Add test to validate off by one bug
Resolves: https://fedorahosted.org/sssd/ticket/2922
src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+)
diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 --- a/src/tests/cmocka/test_sss_idmap.c +++ b/src/tests/cmocka/test_sss_idmap.c
//snip
@@ -140,6 +198,23 @@ static int test_sss_idmap_setup_with_domains(void **state) { return 0; }
+static int test_sss_idmap_setup_with_domains_2922(void **state) {
^^^ should be on new line
OK, will change that in next version (if it will be requested).
- struct test_ctx *test_ctx;
- int ret;
- test_sss_idmap_setup(state);
- test_ctx = talloc_get_type(*state, struct test_ctx);
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- ret = setup_ranges_2922(test_ctx);
- return ret;
+}
If you think that usage of new-style error handling in fixtures should not block this patch then please use old-style. In another words, please use old-style of everything or new-style of everything.
It's up to you which way you will choose.
Patch seems to be good enough to Michal and me, unless somebody else agrees with your concerns I will direct my effort to more productive work.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On (09/02/16 11:54), Pavel Reichl wrote:
On 02/09/2016 08:58 AM, Lukas Slebodnik wrote:
On (08/02/16 09:24), Pavel Reichl wrote:
On 02/05/2016 10:25 PM, Lukas Slebodnik wrote:
On (05/02/16 19:34), Michal Židek wrote:
On 02/05/2016 06:32 PM, Lukas Slebodnik wrote:
On (05/02/16 17:30), Michal Židek wrote: >On 02/05/2016 05:13 PM, Lukas Slebodnik wrote: >>On (05/02/16 16:56), Pavel Reichl wrote: >>>Hopefully the last one. >> >>>From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001 >>>From: Pavel Reichl preichl@redhat.com >>>Date: Fri, 22 Jan 2016 08:34:14 -0500 >>>Subject: [PATCH] IDMAP: Add test to validate off by one bug >>> >>>Resolves: >>>https://fedorahosted.org/sssd/ticket/2922 >>>--- >>>src/tests/cmocka/test_sss_idmap.c | 125 ++++++++++++++++++++++++++++++++++++++ >>>1 file changed, 125 insertions(+) >>> >>>diff --git a/src/tests/cmocka/test_sss_idmap.c b/src/tests/cmocka/test_sss_idmap.c >>>index 00e03ffd9ab1532fb55795b9935b254c8a89ec16..2c99a09e6d11e8d731860eaad94eda0900fafabe 100644 >>>--- a/src/tests/cmocka/test_sss_idmap.c >>>+++ b/src/tests/cmocka/test_sss_idmap.c >>>@@ -43,6 +43,17 @@ >>>#define TEST_OFFSET 1000000 >>>#define TEST_OFFSET_STR "1000000" >>> >>>+#define TEST_2922_MIN_ID 1842600000 >>>+#define TEST_2922_MAX_ID 1842799999 >>>+#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1) >>>+ >>>+#define TEST_2922_DFL_SLIDE 9212 >>>+#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" >>>+/* Last SID = first SID + (default) rangesize -1 */ >>>+#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" >>>+/* Last SID = first SID + rangesize */ >>>+#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000" >>>+ >>We should prefer to use constants instead of #defines. >>Constants are more type safe. >> > >Do you want Pavel to change it? If so, then I would prefer >at least the strings to remain as #defines. It is more >comfortable to concatenate string literals. But honestly >I am ok with the numbers to remain as #defines as well, >but I agree with your point in general. > Yes, please.
Define can still be used for simplification of string initialisation e.g. #define DOM_SID "S-1-2-3-4" const char TEST_2922_LAST_SID[] = DOM_SID"-99"; const char TEST_2922_LAST_SID_PLUS_ONE[] = DOM_SID"-100";
Yes, they can. But do you require it? As I said I would prefer the strings to remain #defines, but it is not strong preference.
I'm sorry but your preference is not type safe. It's much simpler to identify errors caused by wrong
Well, it's just your preference. It's not in out code style, right? Anyway it's often used in other cmocka tests so it's consistent with out code base.
It's not just my preference constants are already used in test (module scope)
sh$ cd src/tests sh$ git grep "^const" | wc -l 64
grep '#define' `find src/tests -name '*.c'` | grep " | wc -l 237
This is not technical reason for not using type safe constants. In past we used debug macro with numbers(old-style) and hexadecimal version (SSSDBG_*). Even though the old-style version was used much more times we were adding new debug macros with new-style.
And this is the similar case.
Could you give me any reason why macros are better from technical point of view?
It doesn't matter. Both work and we are wasting time on fruitless discussion. You can always improve code. It's import to know when it's good enough. I say using defines in test is good enough and I don't see real benefit in changing it that would justify time spend on fixing.
I'm sorry I cannot see a technical reason in this paragraph why "#defines" are better then constants. BTW the purpose of pre commit review is to improve code. So I cannot see a reason why we should do it later.
We needn't support sssd on platforms with old C compiler and we use c99(gnu99) anyway.
If no then I will appreciate usage of new-style constants.
usage of constants. If you like macros then you can be honored in future to try find bugs in macro generated code (nss-pam-ldapd).
I think this is totally unrelated, please try to focus on purpose of this thread - test for id mapping.
It just an explanation of my reasoning.
>> >> >>>struct test_ctx { >>> TALLOC_CTX *mem_idmap; >>> struct sss_idmap_ctx *idmap_ctx; >>>@@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, >>> return 0; >>>} >>> >>>+static int setup_ranges_2922(struct test_ctx *test_ctx) >>>+{ >>>+ struct sss_idmap_range range; >>>+ enum idmap_error_code err; >>>+ const char *name; >>>+ const char *sid; >>>+ /* Pick a new slice. */ >>>+ id_t slice_num = -1; >>>+ >>>+ if (test_ctx == NULL) { >>>+ DEBUG(SSSDBG_FATAL_FAILURE, >>>+ "test context is NULL\n"); >>>+ return 1; >>>+ } >>>+ >>>+ name = TEST_DOM_NAME; >>>+ sid = TEST_DOM_SID; >>>+ >>>+ err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num, >>>+ &range); >>>+ if (err != IDMAP_SUCCESS) { >>>+ DEBUG(SSSDBG_FATAL_FAILURE, >>>+ "sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n", >>>+ IDMAP_SUCCESS, err); >>>+ return 1; >>Please use different numbers for different failures. > >I think it is OK to use the same number in tests, as long as >the debug message is present. We can identify the error >based on stderr messages. > No it's not OK to have same return code. The return codes have to be unique.
They do not need to be unique here. Compare:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:00:17:723118 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
With this:
[ RUN ] test_map_id_2922 (Fri Feb 5 19:01:17:376602 2016) [sssd] [test_sss_idmap_setup_with_domains_2922] (0x0010): test contex is NULL Could not run the test - check test fixtures [ ERROR ] test_map_id_2922
I returned different error code in both cases but the result is the same.
That's because of insufficient desing of libcmocka. Unit test should not contain any if conditions which are not tested. If you run code coverage of unit test it should cover 100% of code path of unit test.
However, the "official way" of handling errors in setup and teardown function does not allow to do this. In other words, returning error code is useless. As you pointed out there's no difference and we need to use debug messages to find out where it failed. that's the worst situation. The error message and line where it happened should be logged automaticaly or thearduwn function should return unique identifier of failure.
The usage of debug message (printf error) is kind of workaround for current state. Workarounds should be removed after fixing the staff. So we will need to retun a unique error code anyway in the end and there's no reason to do it twice.
We are not working on cmocka code here and we cannot predict its evolution.
On the other hand, this patch introduced the new style of error handling in fixtures (setup and teardown functions). This code will be seen as a precedence in future because we do not have a coding guidelines for this. So it would be better to do it properly.
Well, that's the problem - you think your way is proper and I and Michal don't share it.
I'm sorry I did a mistake in previous paragraph. s/precedence/precedent/ We do not have a coding guidelines for handling errors in fixtures. This introduced new way of handling errors will be in future seen as a refferential implementation. And there should be common agreement how to do it otherwise different develepers will do it in different way and in the end it will be mess.
This is a main *BLOCKER* for me.
I will repeat here a compromise from previous mail which I will accept. If you think that usage of new-style error handling in fixtures should not block this patch then please use old-style. In another words, Please use old-style of everything or new-style of everything.
It's up to you which way you will choose.
LS
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
Hi, I will chime in here since this mail remains clear...
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
I don't really care about type safety here I care about correct description of magic values and no duplication of them. Given the #defines prefix those can be used only for this test and some of the macros are used across the test and fixture I'd prefer combination of both.
Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID + 1" is descriptive enough. The rest should be as static const since they are relevant only to one function in one test.
Is that good enough compromise?
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
- }
- /* Range computation should be deterministic. Lets validate that. */
- if (range.min != TEST_2922_MIN_ID
|| range.max != TEST_2922_MAX_ID
|| slice_num != TEST_2922_DFL_SLIDE) {
DEBUG(SSSDBG_FATAL_FAILURE, "Failed to compute range correctly.\n");
return 1;
- }
- err = sss_idmap_add_domain_ex(test_ctx->idmap_ctx, name, sid, &range,
NULL, 0, false /* No external mapping */);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_add_domain_ex failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
- }
- return 0;
+}
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On (10/02/16 11:38), Pavel Březina wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
Hi, I will chime in here since this mail remains clear...
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
I don't really care about type safety here I care about correct description of magic values and no duplication of them. Given the #defines prefix those can be used only for this test and some of the macros are used across the test and fixture I'd prefer combination of both.
Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID + 1" is descriptive enough. The rest should be as static const since they are relevant only to one function in one test.
Is that good enough compromise?
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thank you for another oninion. I prefer assertion as well but I was not stricly against error codes.
You proposed more/less the same things as I wrote in my last mails.
LS
On 02/10/2016 12:04 PM, Lukas Slebodnik wrote:
On (10/02/16 11:38), Pavel Březina wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
Hi, I will chime in here since this mail remains clear...
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
I don't really care about type safety here I care about correct description of magic values and no duplication of them. Given the #defines prefix those can be used only for this test and some of the macros are used across the test and fixture I'd prefer combination of both.
Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID + 1" is descriptive enough. The rest should be as static const since they are relevant only to one function in one test.
Is that good enough compromise?
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thank you for another oninion. I prefer assertion as well but I was not stricly against error codes.
You proposed more/less the same things as I wrote in my last mails.
Well no, asserts were out of question because they were banned by Jakub (2nd mail in this thread). If Jakub is fine with asserts I'm all for it.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On Wed, Feb 10, 2016 at 12:43:18PM +0100, Pavel Reichl wrote:
On 02/10/2016 12:04 PM, Lukas Slebodnik wrote:
On (10/02/16 11:38), Pavel Březina wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
Hi, I will chime in here since this mail remains clear...
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
I don't really care about type safety here I care about correct description of magic values and no duplication of them. Given the #defines prefix those can be used only for this test and some of the macros are used across the test and fixture I'd prefer combination of both.
Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID + 1" is descriptive enough. The rest should be as static const since they are relevant only to one function in one test.
Is that good enough compromise?
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thank you for another oninion. I prefer assertion as well but I was not stricly against error codes.
You proposed more/less the same things as I wrote in my last mails.
Well no, asserts were out of question because they were banned by Jakub (2nd mail in this thread). If Jakub is fine with asserts I'm all for it.
I didn't ban anything, I just don't like the way cmocka errors out with asserts in fixtures (and I'm probably half-guilty for that..)
On 02/10/2016 12:52 PM, Jakub Hrozek wrote:
On Wed, Feb 10, 2016 at 12:43:18PM +0100, Pavel Reichl wrote:
On 02/10/2016 12:04 PM, Lukas Slebodnik wrote:
On (10/02/16 11:38), Pavel Březina wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
Hi, I will chime in here since this mail remains clear...
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
I don't really care about type safety here I care about correct description of magic values and no duplication of them. Given the #defines prefix those can be used only for this test and some of the macros are used across the test and fixture I'd prefer combination of both.
Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID + 1" is descriptive enough. The rest should be as static const since they are relevant only to one function in one test.
Is that good enough compromise?
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thank you for another oninion. I prefer assertion as well but I was not stricly against error codes.
You proposed more/less the same things as I wrote in my last mails.
Well no, asserts were out of question because they were banned by Jakub (2nd mail in this thread). If Jakub is fine with asserts I'm all for it.
I didn't ban anything, I just don't like the way cmocka errors out with asserts in fixtures (and I'm probably half-guilty for that..)
AFAIK there is a bug that an error is not printed by that was fixed an hour ago in upstream, so we should not avoid using assert because of that. But writing unit test should be simple and fast so we should not care about any error codes and debug messages and handling of errors.
If there is any specific issue than we should send a patch to improve it instead of avoid using it...
On 02/10/2016 12:52 PM, Jakub Hrozek wrote:
On Wed, Feb 10, 2016 at 12:43:18PM +0100, Pavel Reichl wrote:
On 02/10/2016 12:04 PM, Lukas Slebodnik wrote:
On (10/02/16 11:38), Pavel Březina wrote:
On 02/05/2016 05:13 PM, Lukas Slebodnik wrote:
On (05/02/16 16:56), Pavel Reichl wrote:
Hopefully the last one.
From 58b06b8fee18c4242e301ecf71f92a77f6ba6e7b Mon Sep 17 00:00:00 2001
Hi, I will chime in here since this mail remains clear...
+#define TEST_2922_MIN_ID 1842600000 +#define TEST_2922_MAX_ID 1842799999 +#define TEST_2922_MAX_ID_PLUS_ONE (TEST_2922_MAX_ID + 1)
+#define TEST_2922_DFL_SLIDE 9212 +#define TEST_2922_FIRST_SID TEST_DOM_SID"-0" +/* Last SID = first SID + (default) rangesize -1 */ +#define TEST_2922_LAST_SID TEST_DOM_SID"-199999" +/* Last SID = first SID + rangesize */ +#define TEST_2922_LAST_SID_PLUS_ONE TEST_DOM_SID"-200000"
We should prefer to use constants instead of #defines. Constants are more type safe.
I don't really care about type safety here I care about correct description of magic values and no duplication of them. Given the #defines prefix those can be used only for this test and some of the macros are used across the test and fixture I'd prefer combination of both.
Keep MIN_ID and MAX_ID as macros. Drop _PLUS_ONE completely since "MAX_ID + 1" is descriptive enough. The rest should be as static const since they are relevant only to one function in one test.
Is that good enough compromise?
struct test_ctx { TALLOC_CTX *mem_idmap; struct sss_idmap_ctx *idmap_ctx; @@ -128,6 +139,53 @@ static int setup_ranges(struct test_ctx *test_ctx, bool external_mapping, return 0; }
+static int setup_ranges_2922(struct test_ctx *test_ctx) +{
- struct sss_idmap_range range;
- enum idmap_error_code err;
- const char *name;
- const char *sid;
- /* Pick a new slice. */
- id_t slice_num = -1;
- if (test_ctx == NULL) {
DEBUG(SSSDBG_FATAL_FAILURE,
"test context is NULL\n");
return 1;
- }
- name = TEST_DOM_NAME;
- sid = TEST_DOM_SID;
- err = sss_idmap_calculate_range(test_ctx->idmap_ctx, sid, &slice_num,
&range);
- if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_FATAL_FAILURE,
"sss_idmap_calculate_range failed: exp: [%d] but got [%d]\n",
IDMAP_SUCCESS, err);
return 1;
Please use different numbers for different failures.
Persoanly, I do not like theusage of such error handling in tests. But that was a desigh decision of cmocka developers which I do not consider as the right one.
Old versions of cmocka allowed to use assertions in setup and teradown function. The same applies to check framework. Such decision add unnecassary complexity to the review process. Reviewer need to check that different error codes are returned or debug message is logged with proper debug level. But that's the separete discusssion.
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thank you for another oninion. I prefer assertion as well but I was not stricly against error codes.
You proposed more/less the same things as I wrote in my last mails.
Well no, asserts were out of question because they were banned by Jakub (2nd mail in this thread). If Jakub is fine with asserts I'm all for it.
I didn't ban anything, I just don't like the way cmocka errors out with asserts in fixtures (and I'm probably half-guilty for that..)
Sorry, I meant that I considered them as no way to go - I thought it was based on discussion you and Michal had.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thanks Pavel, can you please check if first version of the patch would work for you?
On 02/10/2016 12:44 PM, Pavel Reichl wrote:
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thanks Pavel, can you please check if first version of the patch would work for you?
Yes, it works for me.
On 02/10/2016 12:50 PM, Pavel Březina wrote:
On 02/10/2016 12:44 PM, Pavel Reichl wrote:
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thanks Pavel, can you please check if first version of the patch would work for you?
Yes, it works for me.
Hello, I added types and made constants local to functions that use them as Lukas and Pavel wished for. I also fixed the trailing '{' it the function definition as Lukas pointed out.
On 02/10/2016 01:14 PM, Pavel Reichl wrote:
On 02/10/2016 12:50 PM, Pavel Březina wrote:
On 02/10/2016 12:44 PM, Pavel Reichl wrote:
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thanks Pavel, can you please check if first version of the patch would work for you?
Yes, it works for me.
Hello, I added types and made constants local to functions that use them as Lukas and Pavel wished for. I also fixed the trailing '{' it the function definition as Lukas pointed out.
My intend was to keep TEST_2922_MIN/MAX_ID as a macro and the const's should be also declared as static, but let's not postpone this patch any longer. It's just a unit test anyway so it doesn't really matter.
Ack from me.
On (11/02/16 12:43), Pavel Březina wrote:
On 02/10/2016 01:14 PM, Pavel Reichl wrote:
On 02/10/2016 12:50 PM, Pavel Březina wrote:
On 02/10/2016 12:44 PM, Pavel Reichl wrote:
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thanks Pavel, can you please check if first version of the patch would work for you?
Yes, it works for me.
Hello, I added types and made constants local to functions that use them as Lukas and Pavel wished for. I also fixed the trailing '{' it the function definition as Lukas pointed out.
My intend was to keep TEST_2922_MIN/MAX_ID as a macro and the const's should be also declared as static, but let's not postpone this patch any longer. It's just a unit test anyway so it doesn't really matter.
Ack from me.
Thank you for review and for ACK. but would you be so kind and add also link to CI results.
I have never seen such link from you :-) If you do not know how to do it just ask me privately.
LS
On 02/12/2016 11:00 AM, Lukas Slebodnik wrote:
On (11/02/16 12:43), Pavel Březina wrote:
On 02/10/2016 01:14 PM, Pavel Reichl wrote:
On 02/10/2016 12:50 PM, Pavel Březina wrote:
On 02/10/2016 12:44 PM, Pavel Reichl wrote:
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
>
Thanks Pavel, can you please check if first version of the patch would work for you?
Yes, it works for me.
Hello, I added types and made constants local to functions that use them as Lukas and Pavel wished for. I also fixed the trailing '{' it the function definition as Lukas pointed out.
My intend was to keep TEST_2922_MIN/MAX_ID as a macro and the const's should be also declared as static, but let's not postpone this patch any longer. It's just a unit test anyway so it doesn't really matter.
Ack from me.
Thank you for review and for ACK. but would you be so kind and add also link to CI results.
No.
I have never seen such link from you :-) If you do not know how to do it just ask me privately.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
On Fri, Feb 12, 2016 at 02:38:45PM +0100, Pavel Březina wrote:
No.
Running CI should be a part of reviewer's responsibility and we should not accept patches without a passing CI link -or- a link where the failure is clearly not related to the diff (maybe a buggy test or an ill CI VM) -or- where the patch is clearly not covered by our tests (maybe a docs fix).
I ran the CI tests for you: http://sssd-ci.duckdns.org/logs/job/37/32/summary.html but feel free to ask me for instructions next time.
And yes, it's suboptimal that pre-commit CI must be run manually and requires access to RH VPN, but that's the way it is for now..
On (11/02/16 12:43), Pavel Březina wrote:
On 02/10/2016 01:14 PM, Pavel Reichl wrote:
On 02/10/2016 12:50 PM, Pavel Březina wrote:
On 02/10/2016 12:44 PM, Pavel Reichl wrote:
On 02/10/2016 11:38 AM, Pavel Březina wrote:
You can still use assertions in fixtures (I just checked with asn to be sure) and that is the way we should lean to. So forget error codes and debug messages here, use assertions to make it cleaner and shorter.
Thanks Pavel, can you please check if first version of the patch would work for you?
Yes, it works for me.
Hello, I added types and made constants local to functions that use them as Lukas and Pavel wished for. I also fixed the trailing '{' it the function definition as Lukas pointed out.
My intend was to keep TEST_2922_MIN/MAX_ID as a macro and the const's should be also declared as static, but let's not postpone this patch any longer. It's just a unit test anyway so it doesn't really matter.
Ack from me.
master: * 9d17f436795a36b1b1126f444923aa847fd0f93a
sssd-1-13: * 8c1f3b27d8e20b271dfea0c41fd864e9af5476f5
BTW I found a candidate for our coding style guide. Or maybe for contributing guide
Patches should not contain unrelated changes. They should contain only what is described in commit message.
Example from this patch. (Coding style changes in unrelated/untouched part of code)
@@ -152,7 +200,8 @@ static int test_sss_idmap_setup_with_domains_sec_slices(void **state) { return 0; }
-static int test_sss_idmap_setup_with_external_mappings(void **state) { +static int test_sss_idmap_setup_with_external_mappings(void **state) +{ struct test_ctx *test_ctx;
test_sss_idmap_setup(state); @@ -164,7 +213,8 @@ static int test_sss_idmap_setup_with_external_mappings(void **state) { return 0; }
-static int test_sss_idmap_setup_with_both(void **state) { +static int test_sss_idmap_setup_with_both(void **state) +{
LS
On Mon, Feb 15, 2016 at 10:13:30AM +0100, Lukas Slebodnik wrote:
Example from this patch. (Coding style changes in unrelated/untouched part of code)
Maybe, but personally I don't care as long as it's a trivial change that would otherwise never get fixed, because nobody would bother sending a fix that corrects one curly brace..
On (15/02/16 17:07), Jakub Hrozek wrote:
On Mon, Feb 15, 2016 at 10:13:30AM +0100, Lukas Slebodnik wrote:
Example from this patch. (Coding style changes in unrelated/untouched part of code)
Maybe, but personally I don't care as long as it's a trivial change that would otherwise never get fixed, because nobody would bother sending a fix that corrects one curly brace..
Such changes can cause conflicts when backporting patches. And if coding style change is in separate patch then we will be sure that we can ignore it and just fix a conflict. It's much harder to say it in case of complicated patch.
Lmp
sssd-devel@lists.fedorahosted.org