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.
> 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