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

Jakub Hrozek jhrozek at redhat.com
Thu Jan 15 10:26:53 UTC 2015


On Thu, Jan 15, 2015 at 07:50:35AM +0100, Lukas Slebodnik wrote:
> 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

* master: fd52e9e51bdfe00e035e0ab08ce9a6a5d6b7a974



More information about the sssd-devel mailing list