[SSSD] [PATCH] server-tests: do not use complex condition in assert

Lukas Slebodnik lslebodn at redhat.com
Thu Jan 15 06:50:35 UTC 2015


On (14/01/15 21:24), Pavel Březina wrote:
>On 01/14/2015 04:25 PM, Lukas Slebodnik wrote:
>>On (14/01/15 15:57), Pavel Březina wrote:
>>>From 52460bd061607b4d853831298d268197f9e86a25 Mon Sep 17 00:00:00 2001
>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>>>Date: Wed, 14 Jan 2015 15:54:41 +0100
>>>Subject: [PATCH 3/3] server-tests: do not test for specific errno value
>>>
>>>Even though strtouint32 sets errno to 0 and uses only
>>>ERANGE error code we should not rely on it.
>>>---
>>>src/tests/cwrap/test_server.c | 3 ++-
>>>1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
>>>index 323c17d45407cfdd6fa3065b065427b4309b9ac6..84f3925c37203ade07b909267ed674844fb34390 100644
>>>--- a/src/tests/cwrap/test_server.c
>>>+++ b/src/tests/cwrap/test_server.c
>>>@@ -75,9 +75,10 @@ static void wait_for_bg_server(const char *pidfile)
>>>
>>>     buf[sizeof(buf) - 1] = '\0';
>>>
>>>+    errno = 0;
>>>     tmp = strtouint32(buf, NULL, 10);
>>>     assert_false(tmp == 0);
>>>-    assert_false(errno == ERANGE);
>>>+    assert_true(errno == 0);
>>NACK
>>Please use assert_int_equal. In case of error we will be able to see value of
>>errno. The same applies to asser "tmp == 0" changed in previous patch.
>
>Good idea. New patch is attached.
>
>>BTW: Do we really need 3 patches for such simple change?
>
>I personally prefer to put different changes to different patch even though
>it is small and in the same code. But I don't really mind having it as a
>single patch. I squashed it together.
Generally, I agree with you but splitting "7 insertions(+), 4 deletions(-)"
into 3 patches was to much.

>From ccc4cb6961a66a8f2c60596c7c933d0fb26de62b Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>Date: Fri, 9 Jan 2015 15:12:31 +0100
>Subject: [PATCH] server-tests: use strtouint32 instead strtol
>
>PID may be greater than 0xffff thus we remove this check but it is
>supposed to be in range of uint32.
>
>There are also some improvements to get more information from
>assertions.
>---
> src/tests/cwrap/test_server.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/src/tests/cwrap/test_server.c b/src/tests/cwrap/test_server.c
>index d0aeac47d0b067fdbc3399037c0a74f150337a23..55ef2ecae852199b7b9e5300b56fdcb751d0d31c 100644
>--- a/src/tests/cwrap/test_server.c
>+++ b/src/tests/cwrap/test_server.c
>@@ -26,6 +26,7 @@
> 
> #include <popt.h>
> #include "util/util.h"
>+#include "util/strtonum.h"
> #include "tests/cmocka/common_mock.h"
> 
> static void wait_for_fg_server(pid_t pid)
>@@ -44,7 +45,7 @@ static void wait_for_fg_server(pid_t pid)
> static void wait_for_bg_server(const char *pidfile)
> {
>     int fd;
>-    long tmp;
>+    uint32_t tmp;
>     char buf[16];
>     pid_t pid;
>     int ret;
>@@ -74,10 +75,12 @@ static void wait_for_bg_server(const char *pidfile)
> 
>     buf[sizeof(buf) - 1] = '\0';
> 
>-    tmp = strtol(buf, NULL, 10);
>-    assert_false(tmp == 0 || tmp > 0xFFFF || errno == ERANGE);
>+    errno = 0;
>+    tmp = strtouint32(buf, NULL, 10);
>+    assert_int_not_equal(tmp, 0);
>+    assert_int_equal(errno, 0);
> 
>-    pid = (pid_t) (tmp & 0xFFFF);
>+    pid = (pid_t) (tmp);
> 
>     /* Make sure the daemon goes away! */
>     ret = kill(pid, SIGTERM);

Thank you very much for all changes.
ACK

http://sssd-ci.duckdns.org/logs/job/5/99/summary.html

LS



More information about the sssd-devel mailing list