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