[PATCH nft 0/6] add infrastructure for unit tests

[PATCH nft 0/6] add infrastructure for unit tests

[Thomas Haller]

There are new new make targets:

  - "build-all"
  - "check" (runs "normal" tests, like unit tests and "tools/check-tree.sh").
  - "check-more" (runs extra tests, like "tests/build")
  - "check-all" (runs "check" + "check-more")
  - "check-local" (a subset of "check")
  - "check-TESTS" (the unit tests)

The unit tests have a test runner "tools/test-runner.sh". See
`tools/test-runner.sh -h` for options, like valgrind, GDB, or make.
It also runs the test in a separate namespace (rootless).

To run unit tests only, `make check-TESTS` or `tools/test-runner.sh
tests/unit/test-libnftables-static -m`.

The unit tests are of course still empty. "tests/unit" is the place
where tests shall be added.

Thomas Haller (6):
  gitignore: ignore build artifacts from top level file
  build: add basic "check-{local,more,all}" and "build-all" make targets
  build: add `make check-tests-build` to add build test
  build: add check for consistency of source tree
  build: cleanup if blocks for conditional compilation
  tests/unit: add unit tests for libnftables

 .gitignore                           |  15 +-
 Makefile.am                          | 128 ++++++++++++---
 src/.gitignore                       |   5 -
 tests/unit/nft-test.h                |  14 ++
 tests/unit/test-libnftables-static.c |  16 ++
 tests/unit/test-libnftables.c        |  21 +++
 tools/test-runner.sh                 | 228 +++++++++++++++++++++++++++
 7 files changed, 399 insertions(+), 28 deletions(-)
 create mode 100644 tests/unit/nft-test.h
 create mode 100644 tests/unit/test-libnftables-static.c
 create mode 100644 tests/unit/test-libnftables.c
 create mode 100755 tools/test-runner.sh

-- 
2.41.0

[PATCH nft 1/6] gitignore: ignore build artifacts from top level file

[Thomas Haller]

I don't think that having multiple .gitignore files for a small project
like nftables is best. Anyway. The build artifacts like "*.o" will not
only be found under "src/". Move those patterns.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 .gitignore     | 7 ++++++-
 src/.gitignore | 5 -----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/.gitignore b/.gitignore
index a62e31f31c6b..51429020ceb6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,6 +1,11 @@
-# Generated by autoconf/configure/automake
+# Generated by autoconf/configure/automake/make
 *.m4
+*.la
+*.lo
+*.o
+.deps/
 .dirstamp
+.libs/
 Makefile
 Makefile.in
 stamp-h1
diff --git a/src/.gitignore b/src/.gitignore
index 2d907425cbb0..f34105c6cda4 100644
--- a/src/.gitignore
+++ b/src/.gitignore
@@ -1,8 +1,3 @@
-*.la
-*.lo
-*.o
-.deps/
-.libs/
 nft
 parser_bison.c
 parser_bison.h
-- 
2.41.0

[PATCH nft 2/6] build: add basic "check-{local,more,all}" and "build-all" make targets

[Thomas Haller]

Add targets "check-local" and "check-more", which later will hook
up additional tests. For now, they are empty targets.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Makefile.am | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 0ed831a19e95..93bd47970077 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -31,8 +31,11 @@ lib_LTLIBRARIES =
 noinst_LTLIBRARIES =
 sbin_PROGRAMS =
 check_PROGRAMS =
+check_LTLIBRARIES =
 dist_man_MANS =
 CLEANFILES =
+check_local =
+check_more =
 
 ###############################################################################
 
@@ -409,3 +412,23 @@ EXTRA_DIST += \
 
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libnftables.pc
+
+###############################################################################
+
+build-all: all $(check_PROGRAMS) $(check_LTLIBRARIES)
+
+.PHONY: build-all
+
+###############################################################################
+
+check-local: build-all $(check_local)
+
+.PHONY: check-local $(check_local)
+
+check-more: build-all $(check_more)
+
+.PHONY: check-more $(check_more)
+
+check-all: check check-more
+
+.PHONY: check-all
-- 
2.41.0

[PATCH nft 3/6] build: add `make check-tests-build` to add build test

[Thomas Haller]

Call "./tests/build/run-tests.sh" from a new make target
"check-tests-build". This script performs the build check.

As this script takes a rather long time, it's not hooked with
"check-local", and consequently not run by `make check`. Instead, it is
a dependency of `make check-more`.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Makefile.am | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 93bd47970077..f39d6cdd0ca3 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -421,6 +421,14 @@ build-all: all $(check_PROGRAMS) $(check_LTLIBRARIES)
 
 ###############################################################################
 
+check-tests-build: build-all
+	cd "$(srcdir)/tests/build/" ; \
+	CC="$(CC)" CFLAGS='-Werror' ./run-tests.sh
+
+check_more += check-tests-build
+
+###############################################################################
+
 check-local: build-all $(check_local)
 
 .PHONY: check-local $(check_local)
-- 
2.41.0

[PATCH nft 4/6] build: add check for consistency of source tree

[Thomas Haller]

The script "tools/check-tree.sh" performs some consistency checks of the
source tree. Call it from a make target "check-tree".

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Makefile.am | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index f39d6cdd0ca3..94c40fc3c6a2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -408,6 +408,7 @@ EXTRA_DIST += \
 EXTRA_DIST += \
 	files \
 	tests \
+	tools \
 	$(NULL)
 
 pkgconfigdir = $(libdir)/pkgconfig
@@ -429,6 +430,13 @@ check_more += check-tests-build
 
 ###############################################################################
 
+check-tree:
+	"$(srcdir)/tools/check-tree.sh"
+
+check_local += check-tree
+
+###############################################################################
+
 check-local: build-all $(check_local)
 
 .PHONY: check-local $(check_local)
-- 
2.41.0

[PATCH nft 5/6] build: cleanup if blocks for conditional compilation

[Thomas Haller]

`configure` sets those $(*_LIBS) variables to something empty, if the
dependency is not available. It's cumbersome and unnecessary to
explicitly checking.

Also, the order in which libraries are specified on the command line
matters. Commonly we want our own libraries first (src/libminigmp.la
should come pretty early). That is cumbersome to get right otherwise.

Also, as "xt.c" is always built, just directly add it to the list of
source files.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 Makefile.am | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 94c40fc3c6a2..f06fab8e3b9f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -195,6 +195,12 @@ src_libminigmp_la_CFLAGS = \
 	-Wno-sign-compare \
 	$(NULL)
 
+LIBMINIGMP_LIBS = src/libminigmp.la
+
+else
+
+LIBMINIGMP_LIBS =
+
 endif
 
 ###############################################################################
@@ -247,10 +253,9 @@ src_libnftables_la_SOURCES = \
 	src/tcpopt.c \
 	src/utils.c \
 	src/xfrm.c \
+	src/xt.c \
 	$(NULL)
 
-src_libnftables_la_SOURCES += src/xt.c
-
 if BUILD_JSON
 src_libnftables_la_SOURCES += \
 	src/json.c \
@@ -264,23 +269,14 @@ src_libnftables_la_LDFLAGS = \
 	$(NULL)
 
 src_libnftables_la_LIBADD = \
+	src/libparser.la \
+	$(LIBMINIGMP_LIBS) \
 	$(LIBMNL_LIBS) \
 	$(LIBNFTNL_LIBS) \
-	src/libparser.la \
+	$(XTABLES_LIBS) \
+	$(JANSSON_LIBS) \
 	$(NULL)
 
-if BUILD_MINIGMP
-src_libnftables_la_LIBADD += src/libminigmp.la
-endif
-
-if BUILD_XTABLES
-src_libnftables_la_LIBADD += $(XTABLES_LIBS)
-endif
-
-if BUILD_JSON
-src_libnftables_la_LIBADD += $(JANSSON_LIBS)
-endif
-
 ###############################################################################
 
 sbin_PROGRAMS += src/nft
-- 
2.41.0

[PATCH nft 6/6] tests/unit: add unit tests for libnftables

[Thomas Haller]

We have a lot of tests/shell tests, that use the nft binary.  However,
it's useful to also write unit tests, that can test the internal C code
specifically.

Since no such tests exist yet, it would be cumbersome to add a test. Add
two binaries, that can be used as a place for such tests. Currently
there are no real tests there, it's only to show how it works, and that
those dummy tests pass (including that the linking and execution works).

To access the internals, build an intermediate static library
src/libnftables-static.la, which then makes up the public, dynamic
src/libnftables.la library. There are two tests:

  - tests/unit/test-libnftables-static
  - tests/unit/test-libnftables

The former statically links with src/libnftables-static.la and can test
internal API from headers under "include/*.h". The latter dynamically
links with src/libnftables.la, and can only test the public API from
includes/nftables/libnftables.h.

You can run the unit tests alone with `make check-TESTS`. Calling
'VALGRIND=y make check' works as expected.

Also add a LOG_COMPILER script "tools/test-runner.sh". This wraps the
execution of the tests. Even for manual testing, you likely don't want
to run "tests/unit/test-*" directly, but rather

  $ ./tools/test-runner.sh tests/unit/test-libnftables

This sets up an unshared namespace and honors VALGRIND=y to run the
test under valgrind. Set NFT_TEST_WRAPPER=gdb to start with a debugger.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 .gitignore                           |   8 +-
 Makefile.am                          |  69 ++++++--
 tests/unit/nft-test.h                |  14 ++
 tests/unit/test-libnftables-static.c |  16 ++
 tests/unit/test-libnftables.c        |  21 +++
 tools/test-runner.sh                 | 228 +++++++++++++++++++++++++++
 6 files changed, 346 insertions(+), 10 deletions(-)
 create mode 100644 tests/unit/nft-test.h
 create mode 100644 tests/unit/test-libnftables-static.c
 create mode 100644 tests/unit/test-libnftables.c
 create mode 100755 tools/test-runner.sh

diff --git a/.gitignore b/.gitignore
index 51429020ceb6..369678a13987 100644
--- a/.gitignore
+++ b/.gitignore
@@ -25,7 +25,13 @@ libtool
 
 # Generated by tests
 *.payload.got
-tests/build/tests.log
+test-suite.log
+tests/**/*.log
+tests/**/*.trs
+tests/**/*.valgrind-log
+
+tests/unit/test-libnftables
+tests/unit/test-libnftables-static
 
 # Debian package build temporary files
 build-stamp
diff --git a/Makefile.am b/Makefile.am
index f06fab8e3b9f..074d2076c53d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -31,12 +31,18 @@ lib_LTLIBRARIES =
 noinst_LTLIBRARIES =
 sbin_PROGRAMS =
 check_PROGRAMS =
+noinst_PROGRAMS =
 check_LTLIBRARIES =
 dist_man_MANS =
 CLEANFILES =
+TESTS =
+test_programs =
 check_local =
 check_more =
 
+TESTS_ENVIRONMENT = VERBOSE="$(V)" NFT_TEST_MAKE=n
+LOG_COMPILER = "$(srcdir)/tools/test-runner.sh" --srcdir "$(abs_srcdir)" --builddir "$(abs_builddir)" --
+
 ###############################################################################
 
 pkginclude_HEADERS = \
@@ -205,11 +211,9 @@ endif
 
 ###############################################################################
 
-lib_LTLIBRARIES += src/libnftables.la
+noinst_LTLIBRARIES += src/libnftables-static.la
 
-src_libnftables_la_SOURCES = \
-	src/libnftables.map \
-	\
+src_libnftables_static_la_SOURCES = \
 	src/cache.c \
 	src/cmd.c \
 	src/ct.c \
@@ -257,20 +261,36 @@ src_libnftables_la_SOURCES = \
 	$(NULL)
 
 if BUILD_JSON
-src_libnftables_la_SOURCES += \
+src_libnftables_static_la_SOURCES += \
 	src/json.c \
 	src/parser_json.c \
 	$(NULL)
 endif
 
+src_libnftables_static_la_LIBADD = \
+	src/libparser.la \
+	$(LIBMINIGMP_LIBS) \
+	$(LIBMNL_LIBS) \
+	$(LIBNFTNL_LIBS) \
+	$(XTABLES_LIBS) \
+	$(JANSSON_LIBS) \
+	$(NULL)
+
+###############################################################################
+
+lib_LTLIBRARIES += src/libnftables.la
+
+src_libnftables_la_SOURCES = \
+	src/libnftables.map \
+	$(NULL)
+
 src_libnftables_la_LDFLAGS = \
 	-version-info "${libnftables_LIBVERSION}" \
 	-Wl,--version-script="$(srcdir)/src//libnftables.map" \
 	$(NULL)
 
 src_libnftables_la_LIBADD = \
-	src/libparser.la \
-	$(LIBMINIGMP_LIBS) \
+	src/libnftables-static.la \
 	$(LIBMNL_LIBS) \
 	$(LIBNFTNL_LIBS) \
 	$(XTABLES_LIBS) \
@@ -303,6 +323,22 @@ examples_nft_json_file_LDADD = src/libnftables.la
 
 ###############################################################################
 
+EXTRA_DIST += tests/unit/nft-test.h
+
+###############################################################################
+
+test_programs += tests/unit/test-libnftables-static
+
+tests_unit_test_libnftables_static_LDADD = src/libnftables-static.la
+
+###############################################################################
+
+test_programs += tests/unit/test-libnftables
+
+tests_unit_test_libnftables_LDADD = src/libnftables.la
+
+###############################################################################
+
 if BUILD_MAN
 
 dist_man_MANS += \
@@ -390,6 +426,16 @@ dist_pkgsysconf_DATA = \
 
 ###############################################################################
 
+EXTRA_DIST += \
+	tests/build \
+	tests/json_echo \
+	tests/monitor \
+	tests/py \
+	tests/shell \
+	$(NULL)
+
+###############################################################################
+
 EXTRA_DIST += \
 	py/pyproject.toml \
 	py/setup.cfg \
@@ -403,7 +449,6 @@ EXTRA_DIST += \
 
 EXTRA_DIST += \
 	files \
-	tests \
 	tools \
 	$(NULL)
 
@@ -412,7 +457,13 @@ pkgconfig_DATA = libnftables.pc
 
 ###############################################################################
 
-build-all: all $(check_PROGRAMS) $(check_LTLIBRARIES)
+check_PROGRAMS += $(test_programs)
+
+TESTS += $(test_programs)
+
+###############################################################################
+
+build-all: all $(check_PROGRAMS) $(test_programs) $(check_LTLIBRARIES)
 
 .PHONY: build-all
 
diff --git a/tests/unit/nft-test.h b/tests/unit/nft-test.h
new file mode 100644
index 000000000000..cab97b42c669
--- /dev/null
+++ b/tests/unit/nft-test.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __NFT_TEST__H__
+#define __NFT_TEST__H__
+
+#undef NDEBUG
+
+#include <nft.h>
+
+#include <assert.h>
+#include <stdio.h>
+#include <string.h>
+
+#endif /* __NFT_TEST__H__ */
diff --git a/tests/unit/test-libnftables-static.c b/tests/unit/test-libnftables-static.c
new file mode 100644
index 000000000000..e34fcfd77f39
--- /dev/null
+++ b/tests/unit/test-libnftables-static.c
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "nft-test.h"
+
+#include <datatype.h>
+
+static void test_datatype(void)
+{
+	assert(!datatype_lookup(-1));
+}
+
+int main(int argc, char **argv)
+{
+	test_datatype();
+	return 0;
+}
diff --git a/tests/unit/test-libnftables.c b/tests/unit/test-libnftables.c
new file mode 100644
index 000000000000..100558cd5e0f
--- /dev/null
+++ b/tests/unit/test-libnftables.c
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "nft-test.h"
+
+#include <nftables/libnftables.h>
+
+static void test_nft_ctx(void)
+{
+	struct nft_ctx *ctx;
+
+	ctx = nft_ctx_new(0);
+	assert(ctx);
+
+	nft_ctx_free(ctx);
+}
+
+int main(int argc, char **argv)
+{
+	test_nft_ctx();
+	return 0;
+}
diff --git a/tools/test-runner.sh b/tools/test-runner.sh
new file mode 100755
index 000000000000..18658ba4adf1
--- /dev/null
+++ b/tools/test-runner.sh
@@ -0,0 +1,228 @@
+#!/bin/bash
+
+set -e
+
+die() {
+	printf '%s\n' "$*"
+	exit 1
+}
+
+as_bool() {
+	if [ "$#" -eq 0 ] ; then
+		return 1
+	fi
+	case "$1" in
+		y|Y|yes|Yes|YES|1|true|True|TRUE)
+			return 0
+			;;
+		n|N|no|No|NO|0|false|False|FALSE)
+			return 1
+			;;
+		*)
+			# Fallback to the next in the list.
+			shift
+			rc=0
+			as_bool "$@" || rc=$?
+			return "$rc"
+	esac
+}
+
+as_bool_str() {
+	if as_bool "$@" ; then
+		printf y
+	else
+		printf n
+	fi
+}
+
+usage() {
+	echo "  $0 [OPTIONS] TEST"
+	echo
+	echo "Run TEST"
+	echo
+	echo "Options:"
+	echo "  --srcdir dir: Sets SRCDIR=dir."
+	echo "  --builddir dir: Sets BUILDDIR=dir."
+	echo "  -V|--valgrind: Sets VALGRIND=y."
+	echo "  -G|--gdb: Sets NFT_TEST_WRAPPER=gdb."
+	echo "  -m|--make: Sets NFT_TEST_MAKE=y."
+	echo "  --: Separates options from test name."
+	echo "  TEST: the path of the test executable to run."
+	echo
+	echo "Environment variables:"
+	echo "  SRCDIR: set to \$(srcdir) of the project."
+	echo "  BUILDDIR: set to \$(builddir) of the project."
+	echo "  VERBOSE: for verbose output."
+	echo "  VALGRIND: if set to TRUE, run the test under valgrind. NFT_TEST_UNDER_VALGRIND is ignored."
+	echo "  NFT_TEST_UNSHARE_CMD: override the command to unshare the netns."
+	echo "      Set to empty to not unshare."
+	echo "  NFT_TEST_WRAPPER: usually empty. For manual testing, this is prepended to TEST."
+	echo "      Set for example got \"gdb\"."
+	echo "  NFT_TEST_MAKE: if true, call \`make\` on the test first."
+}
+
+usage_and_die() {
+	usage
+	echo
+	die "$@"
+}
+
+TIMESTAMP=$(date '+%Y%m%d-%H%M%S.%3N')
+
+# SRCDIR
+# BUILDDIR
+VERBOSE="$(as_bool_str "$VERBOSE")"
+VALGRIND="$(as_bool_str "$VALGRIND")"
+NFT_TEST_UNSHARE_CMD="${NFT_TEST_UNSHARE_CMD-unshare -m -U --map-root-user -n}"
+NFT_TEST_WRAPPER="${NFT_TEST_WRAPPER}"
+NFT_TEST_MAKE="$(as_bool_str "$NFT_TEST_MAKE")"
+
+unset TEST
+
+while [ $# -gt 0 ] ; do
+	A="$1"
+	shift
+	case "$A" in
+		-h|--help)
+			usage
+			exit 0
+			;;
+		--srcdir)
+			SRCDIR="$1"
+			shift
+			;;
+		--builddir)
+			BUILDDIR="$1"
+			shift
+			;;
+		-V|--valgrind)
+			VALGRIND=y
+			;;
+		-G|--gdb)
+			NFT_TEST_WRAPPER=gdb
+			;;
+		-m|--make)
+			NFT_TEST_MAKE=y
+			;;
+		--)
+			if [ $# -ne 1 ] ; then
+				usage_and_die "Requires a TEST argument after --"
+			fi
+			TEST="$1"
+			shift
+			;;
+		*)
+			if [ -n "${TEST+x}" ] ; then
+				usage_and_die "Unknown argument \"$A\""
+			fi
+			TEST="$A"
+			;;
+	esac
+done
+
+if [ -z "$TEST" ] ; then
+	usage_and_die "Missing test argument. See --help"
+fi
+
+if [ -z "${SRCDIR+x}" ] ; then
+	SRCDIR="$(readlink -f "$(dirname "$0")/..")"
+fi
+if [ ! -d "$SRCDIR" ] ; then
+	die "Invalid \$SRCDIR=\"$SRCDIR\""
+fi
+
+if [ -z "${BUILDDIR+x}" ] ; then
+	re='^(.*/|)(tests/unit/test-[^/]*)$'
+	if [[ "$TEST" =~ $re ]] ; then
+		BUILDDIR="$(readlink -f "${BASH_REMATCH[1]:-.}")" || :
+	else
+		BUILDDIR="$SRCDIR"
+	fi
+fi
+if [ ! -d "$BUILDDIR" ] ; then
+	die "Invalid \$BUILDDIR=\"$BUILDDIR\""
+fi
+
+export TESTDIR="$(dirname "$TEST")"
+export SRCDIR
+export BUILDDIR
+
+if [ "$VALGRIND" = y ] ; then
+	export NFT_TEST_UNDER_VALGRIND=1
+fi
+
+run_unit() {
+	local TEST="$1"
+
+	if [ "$NFT_TEST_MAKE" = y ] ; then
+		local d="$(readlink -f "$BUILDDIR")"
+		local tf="$(readlink -f "$TEST")"
+		local t="${tf#$d/}"
+
+		if [ "$tf" != "$d/$t" ] ; then
+			die "Cannot detect paths for making \"$TEST\" in \"$BUILDDIR\" (\"$d\" and \"$t\")"
+		fi
+		# Don't use "make -C" to avoid the extra "Entering/Leaving directory" messages
+		(cd "$d" && make "$t" ) || die "Making \"$TEST\" failed"
+	fi
+
+	if [ "$VALGRIND" != y ] ; then
+		rc_test=0
+		$NFT_TEST_UNSHARE_CMD \
+			libtool \
+			--mode=execute \
+			$NFT_TEST_WRAPPER \
+			"$TEST" || rc_test=$?
+		if [ "$rc_test" -ne 0 -a "$rc_test" -ne 77 ] ; then
+			die "exec \"$TEST\" failed with exit code $rc_test"
+		fi
+		exit "$rc_test"
+	fi
+
+	LOGFILE="$TEST.valgrind-log"
+
+	rc_test=0
+	$NFT_TEST_UNSHARE_CMD \
+		libtool \
+		--mode=execute \
+		valgrind \
+		--quiet \
+		--error-exitcode=122 \
+		--leak-check=full \
+		--gen-suppressions=all \
+		--num-callers=100 \
+		--log-file="$LOGFILE" \
+		--vgdb-prefix="${TMP:-/tmp}/vgdb-pipe-nft-test-runner-$TIMESTAMP-$$" \
+		$NFT_TEST_VALGRIND_OPTS \
+		"$TEST" \
+		|| rc_test=$?
+
+	if [ -s "$LOGFILE" ] ; then
+		if [ "$rc_test" -eq 0 -o "$rc_test" -eq 122 ] ; then
+			echo "valgrind failed. Logfile at \"$LOGFILE\" :"
+			cat "$LOGFILE"
+			rc_test=122
+		else
+			echo "valgrind also failed. Check logfile at \"$LOGFILE\""
+		fi
+	elif [ ! -f "$LOGFILE" ] ; then
+		echo "valgrind logfile \"$LOGFILE\" missing"
+		if [ "$rc_test" -eq 0 ] ; then
+			rc_test=122
+		fi
+	else
+		rm -rf "$LOGFILE"
+	fi
+
+	exit "$rc_test"
+}
+
+case "$TEST" in
+	tests/unit/test-* | \
+	*/tests/unit/test-* )
+		run_unit "$TEST"
+		;;
+	*)
+		die "Unrecognized test \"$TEST\""
+		;;
+esac
-- 
2.41.0

Re: [PATCH nft 1/6] gitignore: ignore build artifacts from top level file

[Pablo Neira Ayuso]

On Fri, Nov 03, 2023 at 12:05:43PM +0100, Thomas Haller wrote:
> I don't think that having multiple .gitignore files for a small project
> like nftables is best. Anyway. The build artifacts like "*.o" will not
> only be found under "src/". Move those patterns.
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  .gitignore     | 7 ++++++-
>  src/.gitignore | 5 -----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index a62e31f31c6b..51429020ceb6 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,6 +1,11 @@
> -# Generated by autoconf/configure/automake
> +# Generated by autoconf/configure/automake/make
>  *.m4
> +*.la
> +*.lo
> +*.o
> +.deps/
>  .dirstamp
> +.libs/
>  Makefile
>  Makefile.in
>  stamp-h1
> diff --git a/src/.gitignore b/src/.gitignore
> index 2d907425cbb0..f34105c6cda4 100644
> --- a/src/.gitignore
> +++ b/src/.gitignore
> @@ -1,8 +1,3 @@
> -*.la
> -*.lo
> -*.o
> -.deps/
> -.libs/

Please, move things where they used to be.

>  nft
>  parser_bison.c
>  parser_bison.h
> -- 
> 2.41.0
> 

Re: [PATCH nft 0/6] add infrastructure for unit tests

[Pablo Neira Ayuso]

On Fri, Nov 03, 2023 at 12:05:42PM +0100, Thomas Haller wrote:
> There are new new make targets:
> 
>   - "build-all"
>   - "check" (runs "normal" tests, like unit tests and "tools/check-tree.sh").
>   - "check-more" (runs extra tests, like "tests/build")
>   - "check-all" (runs "check" + "check-more")
>   - "check-local" (a subset of "check")
>   - "check-TESTS" (the unit tests)
> 
> The unit tests have a test runner "tools/test-runner.sh". See
> `tools/test-runner.sh -h` for options, like valgrind, GDB, or make.
> It also runs the test in a separate namespace (rootless).
> 
> To run unit tests only, `make check-TESTS` or `tools/test-runner.sh
> tests/unit/test-libnftables-static -m`.
> 
> The unit tests are of course still empty. "tests/unit" is the place
> where tests shall be added.

Thanks a lot for improving tests infrastructure.

Re: [PATCH nft 0/6] add infrastructure for unit tests

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:

Thanks for sending an initial empty skeleton.

> There are new new make targets:
> 
>   - "build-all"
>   - "check" (runs "normal" tests, like unit tests and "tools/check-tree.sh").
>   - "check-more" (runs extra tests, like "tests/build")
>   - "check-all" (runs "check" + "check-more")
>   - "check-local" (a subset of "check")
>   - "check-TESTS" (the unit tests)

"check-unit" perhaps?  TESTS isn't very descriptive.  Also,
why CAPS? If this is some pre-established standard, then maybe just
document that in the commit changelog.

Please don't do anything yet and wait for more comments, but
I would prefer 'make check' to run all tests that we have.

I would suggest
- "check" (run all tests)
- "check-unit" (the unit tests only)
- "check-shell" (shell tests only)
- "check-py" (python based tests only)
- "check-json" (python based tests in json mode and json_echo)

...  and so on.

Re: [PATCH nft 1/6] gitignore: ignore build artifacts from top level file

[Thomas Haller]

On Fri, 2023-11-03 at 12:30 +0100, Pablo Neira Ayuso wrote:
> 
> Please, move things where they used to be.

will do in v2.

Thomas

Re: [PATCH nft 0/6] add infrastructure for unit tests

[Thomas Haller]

On Fri, 2023-11-03 at 13:26 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> 
> Thanks for sending an initial empty skeleton.
> 
> > There are new new make targets:
> > 
> >   - "build-all"
> >   - "check" (runs "normal" tests, like unit tests and "tools/check-
> > tree.sh").
> >   - "check-more" (runs extra tests, like "tests/build")
> >   - "check-all" (runs "check" + "check-more")
> >   - "check-local" (a subset of "check")
> >   - "check-TESTS" (the unit tests)
> 
> "check-unit" perhaps?  TESTS isn't very descriptive.  Also,
> why CAPS? If this is some pre-established standard, then maybe just
> document that in the commit changelog.

"TESTS" is an autotools/automake thing. "check-TESTS" runs the tests
that are setup via "TESTS=".

AFAIU, with autotools, `make check` basically runs the targets `make
check-local` and `make check-TESTS`.


Anyway, I can just alias it via:

  check-unit: check-TESTS



Note that other targets are currently called

  `make check-tests-build`
  `make check-tests-shell` (patch not yet send).

It reminds of the directories "tests/{build,shell,unit}" and would lead
to a name `make check-tests-unit`.

But reconsidering, in v2 I will drop this "check-tests-" prefix then
and call them just "check-{unit,build,shell}".

> 
> Please don't do anything yet and wait for more comments, but
> I would prefer 'make check' to run all tests that we have.

currently `make check-more` only entails `make check-tests-build` (i.e.
`tests/build/run-tests.sh`). Note that

 - `tests/build/run-tests.sh` calls `make distcheck`
 - `make distcheck` runs `make check`.

I don't think it would make sense, to include the build check in `make
check`.

So I think there is a (small) place for "check-more" (and thus "check-
all"). But yes, probably most other tests should be included by the
common `make check`!



> 
> I would suggest
> - "check" (run all tests)
> - "check-unit" (the unit tests only)
> - "check-shell" (shell tests only)
> - "check-py" (python based tests only)
> - "check-json" (python based tests in json mode and json_echo)
> 
> ...  and so on.
> 

ACK. Will send in v2.

Re: [PATCH nft 0/6] add infrastructure for unit tests

[Pablo Neira Ayuso]

On Fri, Nov 03, 2023 at 01:26:41PM +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> 
> Thanks for sending an initial empty skeleton.
> 
> > There are new new make targets:
> > 
> >   - "build-all"
> >   - "check" (runs "normal" tests, like unit tests and "tools/check-tree.sh").
> >   - "check-more" (runs extra tests, like "tests/build")
> >   - "check-all" (runs "check" + "check-more")
> >   - "check-local" (a subset of "check")
> >   - "check-TESTS" (the unit tests)
> 
> "check-unit" perhaps?  TESTS isn't very descriptive.  Also,
> why CAPS? If this is some pre-established standard, then maybe just
> document that in the commit changelog.
> 
> Please don't do anything yet and wait for more comments, but
> I would prefer 'make check' to run all tests that we have.

We had a few tests that have been shown to be unstable.

I just would like that I don't hit this when making the release and
hold back a release because a test fails occasionally.

If we go for `make check' then all test runs must be reliable.

Re: [PATCH nft 0/6] add infrastructure for unit tests

[Thomas Haller]

On Fri, 2023-11-03 at 16:33 +0100, Pablo Neira Ayuso wrote:
> On Fri, Nov 03, 2023 at 01:26:41PM +0100, Florian Westphal wrote:
> > Thomas Haller <thaller@redhat.com> wrote:
> > 
> > Thanks for sending an initial empty skeleton.
> > 
> > > There are new new make targets:
> > > 
> > >   - "build-all"
> > >   - "check" (runs "normal" tests, like unit tests and
> > > "tools/check-tree.sh").
> > >   - "check-more" (runs extra tests, like "tests/build")
> > >   - "check-all" (runs "check" + "check-more")
> > >   - "check-local" (a subset of "check")
> > >   - "check-TESTS" (the unit tests)
> > 
> > "check-unit" perhaps?  TESTS isn't very descriptive.  Also,
> > why CAPS? If this is some pre-established standard, then maybe just
> > document that in the commit changelog.
> > 
> > Please don't do anything yet and wait for more comments, but
> > I would prefer 'make check' to run all tests that we have.
> 
> We had a few tests that have been shown to be unstable.
> 
> I just would like that I don't hit this when making the release and
> hold back a release because a test fails occasionally.
> 
> If we go for `make check' then all test runs must be reliable.
> 

Agree.

Tests must be reliable and `make distcheck/check` usable! Unstable
tests must be fixed. It's a never-ending fight to keep the testsuite
passing well enough.

ATM, the reliability is not great, but not terrible either. Seems
manageable to me.


Thomas