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