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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
