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