[systemd-devel] [PATCH 0/3] Using assert_se() on actions with side effects on test cases

[Filipe Brandenburger]

I bumped into these when building systemd with CPPFLAGS='-DNDEBUG' (which is
the default for one of my build environments) which ends up optimizing out the
assert() statements. It turns out that they were being used in some places in
test cases where there was a side effect, so optimizing them out would cause
the tests to crash.

More to the point, maybe it would make sense to have *all* the assertions in
tests be assert_se. Maybe a global search/replace?

Not sure if this issue is not present elsewhere in systemd either... Not sure
if it's really worth supporting -DNDEBUG. Considering systemd is already
redefining assert(), maybe make it unconditional?

Cheers,
Filipe


Filipe Brandenburger (3):
test-compress: make sure asserts with side effects use assert_se()
test-path-util: use assert_se in all assertions
test-util: use assert_se() for call to safe_mkdir with side effect

src/journal/test-compress.c | 4 ++--
src/test/test-path-util.c | 30 +++++++++++++++---------------
src/test/test-util.c | 2 +-
3 files changed, 18 insertions(+), 18 deletions(-)

[Filipe Brandenburger]

Otherwise the test fails when built with CPPFLAGS='-DNDEBUG' which disables
assertions.

Tested:
- make check TESTS='test-compress' CPPFLAGS='-DNDEBUG'
---
src/journal/test-compress.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/journal/test-compress.c b/src/journal/test-compress.c
index f5f5f8df3902..026d630ac2a8 100644
--- a/src/journal/test-compress.c
+++ b/src/journal/test-compress.c
@@ -145,11 +145,11 @@ static void test_compress_stream(int compression,

assert_se((dst = mkostemp_safe(pattern, O_RDWR|O_CLOEXEC)) >= 0);

- assert(compress(src, dst, -1) == 0);
+ assert_se(compress(src, dst, -1) == 0);

if (cat) {
assert_se(asprintf(&cmd, "%s %s | diff %s -", cat, pattern, srcfile) > 0);
- assert(system(cmd) == 0);
+ assert_se(system(cmd) == 0);
}

log_debug("/* test decompression */");

[Filipe Brandenburger]

Otherwise they get optimized out when CPPFLAGS='-DNDEBUG' is used, and that
causes the tests to fail.

Tested:
- make check TESTS='test-path-util' CPPFLAGS='-DNDEBUG'
---
src/test/test-path-util.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/test/test-path-util.c b/src/test/test-path-util.c
index c8dcd853978f..01afb3e6feed 100644
--- a/src/test/test-path-util.c
+++ b/src/test/test-path-util.c
@@ -79,35 +79,35 @@ static void test_path(void) {
char p2[] = "//aaa/.////ccc";
char p3[] = "/./";

- assert(path_equal(path_kill_slashes(p1), "aaa/bbb/ccc"));
- assert(path_equal(path_kill_slashes(p2), "/aaa/./ccc"));
- assert(path_equal(path_kill_slashes(p3), "/./"));
+ assert_se(path_equal(path_kill_slashes(p1), "aaa/bbb/ccc"));
+ assert_se(path_equal(path_kill_slashes(p2), "/aaa/./ccc"));
+ assert_se(path_equal(path_kill_slashes(p3), "/./"));
}
}

static void test_find_binary(const char *self) {
char *p;

- assert(find_binary("/bin/sh", &p) == 0);
+ assert_se(find_binary("/bin/sh", &p) == 0);
puts(p);
- assert(streq(p, "/bin/sh"));
+ assert_se(streq(p, "/bin/sh"));
free(p);

- assert(find_binary(self, &p) == 0);
+ assert_se(find_binary(self, &p) == 0);
puts(p);
- assert(endswith(p, "/test-path-util"));
- assert(path_is_absolute(p));
+ assert_se(endswith(p, "/test-path-util"));
+ assert_se(path_is_absolute(p));
free(p);

- assert(find_binary("sh", &p) == 0);
+ assert_se(find_binary("sh", &p) == 0);
puts(p);
- assert(endswith(p, "/sh"));
- assert(path_is_absolute(p));
+ assert_se(endswith(p, "/sh"));
+ assert_se(path_is_absolute(p));
free(p);

- assert(find_binary("xxxx-xxxx", &p) == -ENOENT);
+ assert_se(find_binary("xxxx-xxxx", &p) == -ENOENT);

- assert(find_binary("/some/dir/xxxx-xxxx", &p) == -ENOENT);
+ assert_se(find_binary("/some/dir/xxxx-xxxx", &p) == -ENOENT);
}

static void test_prefixes(void) {
@@ -156,8 +156,8 @@ static void test_prefixes(void) {

b = false;
PATH_FOREACH_PREFIX_MORE(s, "") {
- assert(!b);
- assert(streq(s, ""));
+ assert_se(!b);
+ assert_se(streq(s, ""));
b = true;
}
}

[Filipe Brandenburger]

Otherwise it gets optimized out when CPPFLAGS='-DNDEBUG' is used.

Tested:
- make check TESTS='test-util' CPPFLAGS='-DNDEBUG'
---
src/test/test-util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/test-util.c b/src/test/test-util.c
index 34d5f2ed7d23..4d9b28f9c8e7 100644
--- a/src/test/test-util.c
+++ b/src/test/test-util.c
@@ -863,7 +863,7 @@ static void test_readlink_and_make_absolute(void) {
char name_alias[] = "/tmp/test-readlink_and_make_absolute-alias";
char *r = NULL;

- assert(mkdir_safe(tempdir, 0755, getuid(), getgid()) >= 0);
+ assert_se(mkdir_safe(tempdir, 0755, getuid(), getgid()) >= 0);
assert_se(touch(name) >= 0);

assert_se(symlink(name, name_alias) >= 0);

[Lennart Poettering]

On Mon, 25.08.14 22:05, Filipe Brandenburger (***@google.com) wrote:

Applied all three! Thanks!

And yes, I agree, all tests should use assert_se(), never
assert(). Quite a few tests snuck in though that used assert()
instead. WOuld be happy to take a patch correcting this!

Post by Filipe Brandenburger
I bumped into these when building systemd with CPPFLAGS='-DNDEBUG' (which is
the default for one of my build environments) which ends up optimizing out the
assert() statements. It turns out that they were being used in some places in
test cases where there was a side effect, so optimizing them out would cause
the tests to crash.
More to the point, maybe it would make sense to have *all* the assertions in
tests be assert_se. Maybe a global search/replace?
Not sure if this issue is not present elsewhere in systemd either... Not sure
if it's really worth supporting -DNDEBUG. Considering systemd is already
redefining assert(), maybe make it unconditional?
Cheers,
Filipe
test-compress: make sure asserts with side effects use assert_se()
test-path-util: use assert_se in all assertions
test-util: use assert_se() for call to safe_mkdir with side effect
src/journal/test-compress.c | 4 ++--
src/test/test-path-util.c | 30 +++++++++++++++---------------
src/test/test-util.c | 2 +-
3 files changed, 18 insertions(+), 18 deletions(-)

Lennart