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(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>