diff mbox series

[RFC,2/3] Makefile: wire up the clar unit testing framework

Message ID 5195d084d3c1bee76e7e424afec2c09bff8f5dde.1722415748.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Introduce clar testing framework | expand

Commit Message

Patrick Steinhardt July 31, 2024, 9:04 a.m. UTC 
Wire up the clar unit testing framework by introducing a new
"unit-tests" executable. In contrast to the existing framework, this
will result in a single executable for all test suites. The ability to
pick specific tests to execute is retained via functionality built into
the clar itself.

Note that we need to be a bit careful about how we need to invalidate
our Makefile rules. While we obviously have to regenerate the clar suite
when our test suites change, we also have to invalidate it in case any
of the test suites gets removed. We do so by using our typical pattern
of creating a `GIT-TEST-SUITES` file that gets updated whenever the set
of test suites changes, so that we can easily depend on that file.

Another specialty is that we generate a "clar-decls.h" file. The test
functions are neither static, nor do they have external declarations.
This is because they are getting parsed via "generate.py", which then
creates the external generations that get populated into an array. These
declarations are only seen by the main function though.

The consequence is that we will get a bunch of "missing prototypes"
errors from our compiler for each of these test functions. To fix those
errors, we extract the `extern` declarations from "clar.suite" and put
them into a standalone header that then gets included by each of our
unit tests. This gets rid of compiler warnings for every function which
has been extracted by "generate.py". More importantly though, it does
_not_ get rid of warnings in case a function really isn't being used by
anything. Thus, it would cause a compiler error if a function name was
mistyped and thus not picked up by "generate.py".

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 .gitignore               |  1 +
 Makefile                 | 34 ++++++++++++++++++++++++++++++----
 t/Makefile               |  1 +
 t/unit-tests/.gitignore  |  3 +++
 t/unit-tests/unit-test.c | 16 ++++++++++++++++
 t/unit-tests/unit-test.h |  3 +++
 6 files changed, 54 insertions(+), 4 deletions(-)
 create mode 100644 t/unit-tests/unit-test.c
 create mode 100644 t/unit-tests/unit-test.h

Comments

René Scharfe July 31, 2024, 4:48 p.m. UTC | #1 
Am 31.07.24 um 11:04 schrieb Patrick Steinhardt:
> Wire up the clar unit testing framework by introducing a new
> "unit-tests" executable. In contrast to the existing framework, this
> will result in a single executable for all test suites. The ability to
> pick specific tests to execute is retained via functionality built into
> the clar itself.
>
> Note that we need to be a bit careful about how we need to invalidate
> our Makefile rules. While we obviously have to regenerate the clar suite
> when our test suites change, we also have to invalidate it in case any
> of the test suites gets removed. We do so by using our typical pattern
> of creating a `GIT-TEST-SUITES` file that gets updated whenever the set
> of test suites changes, so that we can easily depend on that file.
>
> Another specialty is that we generate a "clar-decls.h" file. The test
> functions are neither static, nor do they have external declarations.
> This is because they are getting parsed via "generate.py", which then
> creates the external generations that get populated into an array. These
> declarations are only seen by the main function though.
>
> The consequence is that we will get a bunch of "missing prototypes"
> errors from our compiler for each of these test functions. To fix those
> errors, we extract the `extern` declarations from "clar.suite" and put
> them into a standalone header that then gets included by each of our
> unit tests. This gets rid of compiler warnings for every function which
> has been extracted by "generate.py". More importantly though, it does
> _not_ get rid of warnings in case a function really isn't being used by
> anything. Thus, it would cause a compiler error if a function name was
> mistyped and thus not picked up by "generate.py".
>

> +$(UNIT_TEST_DIR)/clar.suite: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(UNIT_TESTS_SUITES)) GIT-TEST-SUITES
> +	$(QUIET_GEN)$(UNIT_TEST_DIR)/clar/generate.py $(UNIT_TEST_DIR) >/dev/null

This uses the Python interpreter from the shebang line in generate.py,
which is python.  On my system I only have python3 and python3.12, but
not python.  Easily fixed, of course, but a way to configure the
interpreter name would be nice.

This gave me extra motivation to come up with the clunky patch below
to replace Python with sed and awk.  That and your statement that clar
doesn't have to be perfect in the other thread. ;)

It reverses the order of dependencies (builds clar-decls.h first), not
sure if that has downsides.  And the sed pattern is simpler than the
one in generate.py, just out of laziness.

René

---
 .gitignore                |  1 -
 Makefile                  | 20 ++++++-----------
 t/unit-tests/.gitignore   |  1 -
 t/unit-tests/generate.awk | 47 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 15 deletions(-)
 create mode 100644 t/unit-tests/generate.awk

diff --git a/.gitignore b/.gitignore
index 6687bd6db4..8caf3700c2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,7 +9,6 @@
 /GIT-PYTHON-VARS
 /GIT-SCRIPT-DEFINES
 /GIT-SPATCH-DEFINES
-/GIT-TEST-SUITES
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index 8ebcbdc95a..1ffde38de5 100644
--- a/Makefile
+++ b/Makefile
@@ -3706,7 +3706,7 @@ cocciclean:

 clean: profile-clean coverage-clean cocciclean
 	$(RM) -r .build $(UNIT_TEST_BIN)
-	$(RM) $(UNIT_TEST_DIR)/clar.suite $(UNIT_TEST_DIR)/.clarcache
+	$(RM) $(UNIT_TEST_DIR)/clar.suite $(UNIT_TEST_DIR)/clar-decls.h
 	$(RM) po/git.pot po/git-core.pot
 	$(RM) git.res
 	$(RM) $(OBJECTS)
@@ -3866,19 +3866,13 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS)

-GIT-TEST-SUITES: FORCE
-	@FLAGS='$(UNIT_TESTS_SUITES)'; \
-	    if test x"$$FLAGS" != x"`cat GIT-TEST-SUITES 2>/dev/null`" ; then \
-		echo >&2 "    * new test suites"; \
-		echo "$$FLAGS" >GIT-TEST-SUITES; \
-		rm -f $(UNIT_TESTS_DIR)/.clarcache; \
-            fi
+$(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h $(UNIT_TEST_DIR)/generate.awk
+	$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/generate.awk $(UNIT_TEST_DIR)/clar-decls.h >$@
+$(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(UNIT_TESTS_SUITES))
+	$(QUIET_GEN)for suite in $(UNIT_TESTS_SUITES); do \
+		sed -ne "s/^\(void test_$${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$$\)/extern \1;/p" $(UNIT_TEST_DIR)/$$suite.c; \
+	done >$@

-$(UNIT_TEST_DIR)/clar.suite: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(UNIT_TESTS_SUITES)) GIT-TEST-SUITES
-	$(QUIET_GEN)$(UNIT_TEST_DIR)/clar/generate.py $(UNIT_TEST_DIR) >/dev/null
-	@touch $@
-$(UNIT_TEST_DIR)/clar-decls.h: $(UNIT_TEST_DIR)/clar.suite
-	$(QUIET_GEN)grep '^extern void' $^ >$@
 $(UNIT_TESTS_OBJS): $(UNIT_TEST_DIR)/clar-decls.h
 $(UNIT_TESTS_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) -I$(UNIT_TEST_DIR)/clar
 $(UNIT_TESTS_PROG): $(UNIT_TEST_DIR)/clar.suite $(UNIT_TESTS_OBJS) $(GITLIBS) GIT-LDFLAGS
diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
index b8d46f7bb1..d0632ec7f9 100644
--- a/t/unit-tests/.gitignore
+++ b/t/unit-tests/.gitignore
@@ -1,4 +1,3 @@
 /bin
-/.clarcache
 /clar.suite
 /clar-decls.h
diff --git a/t/unit-tests/generate.awk b/t/unit-tests/generate.awk
new file mode 100644
index 0000000000..3c78253866
--- /dev/null
+++ b/t/unit-tests/generate.awk
@@ -0,0 +1,47 @@
+function add_suite(suite, initialize, cleanup, count) {
+	if (!suite) return
+	suite_count++
+	callback_count += count
+	suites = suites "    {\n"
+	suites = suites "        \"" suite "\",\n"
+	suites = suites "        " initialize ",\n"
+	suites = suites "        " cleanup ",\n"
+	suites = suites "        _clar_cb_" suite ", " count ", 1\n"
+	suites = suites "    },\n"
+}
+BEGIN {
+	suites = "static struct clar_suite _clar_suites[] = {\n"
+}
+{
+	print
+	name = $3; sub(/\(.*$/, "", name)
+	suite = name; sub(/^test_/, "", suite); sub(/__.*$/, "", suite)
+	short_name = name; sub(/^.*__/, "", short_name)
+	cb = "{ \"" short_name "\", &" name " }"
+	if (suite != prev_suite) {
+		add_suite(prev_suite, initialize, cleanup, count)
+		if (callbacks) callbacks = callbacks "};\n"
+		callbacks = callbacks "static const struct clar_func _clar_cb_" suite "[] = {\n"
+		initialize = "{ NULL, NULL }"
+		cleanup = "{ NULL, NULL }"
+		count = 0
+		prev_suite = suite
+	}
+	if (short_name == "initialize") {
+		initialize = cb
+	} else if (short_name == "cleanup") {
+		cleanup = cb
+	} else {
+		callbacks = callbacks "    " cb ",\n"
+		count++
+	}
+}
+END {
+	add_suite(suite, initialize, cleanup, count)
+	suites = suites "};"
+	if (callbacks) callbacks = callbacks "};"
+	print callbacks
+	print suites
+	print "static const size_t _clar_suite_count = " suite_count ";"
+	print "static const size_t _clar_callback_count = " callback_count ";"
+}
--
2.46.0
Junio C Hamano July 31, 2024, 5:01 p.m. UTC | #2 
A trivial fix-up to be squashed into this step.

 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git c/Makefile w/Makefile
index 8ebcbdc95a..d561789582 100644
--- c/Makefile
+++ w/Makefile
@@ -3735,6 +3735,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-BUILD-OPTIONS
+	$(RM) GIT-TEST-SUITES $(UNIT_TEST_DIR)/clar-decls.h
 	$(RM) GIT-USER-AGENT GIT-PREFIX
 	$(RM) GIT-SCRIPT-DEFINES GIT-PERL-DEFINES GIT-PERL-HEADER GIT-PYTHON-VARS
 ifdef MSVC
Junio C Hamano July 31, 2024, 9:39 p.m. UTC | #3 
Junio C Hamano <gitster@pobox.com> writes:

> A trivial fix-up to be squashed into this step.

I won't be able to address this today, but "make sparse" and other
auxiliary targets also seem to break, due to their lack of
dependence on the generated clar.suite file.
Patrick Steinhardt Aug. 1, 2024, 9:32 a.m. UTC | #4 
On Wed, Jul 31, 2024 at 06:48:36PM +0200, René Scharfe wrote:
> Am 31.07.24 um 11:04 schrieb Patrick Steinhardt:
> > Wire up the clar unit testing framework by introducing a new
> > "unit-tests" executable. In contrast to the existing framework, this
> > will result in a single executable for all test suites. The ability to
> > pick specific tests to execute is retained via functionality built into
> > the clar itself.
> >
> > Note that we need to be a bit careful about how we need to invalidate
> > our Makefile rules. While we obviously have to regenerate the clar suite
> > when our test suites change, we also have to invalidate it in case any
> > of the test suites gets removed. We do so by using our typical pattern
> > of creating a `GIT-TEST-SUITES` file that gets updated whenever the set
> > of test suites changes, so that we can easily depend on that file.
> >
> > Another specialty is that we generate a "clar-decls.h" file. The test
> > functions are neither static, nor do they have external declarations.
> > This is because they are getting parsed via "generate.py", which then
> > creates the external generations that get populated into an array. These
> > declarations are only seen by the main function though.
> >
> > The consequence is that we will get a bunch of "missing prototypes"
> > errors from our compiler for each of these test functions. To fix those
> > errors, we extract the `extern` declarations from "clar.suite" and put
> > them into a standalone header that then gets included by each of our
> > unit tests. This gets rid of compiler warnings for every function which
> > has been extracted by "generate.py". More importantly though, it does
> > _not_ get rid of warnings in case a function really isn't being used by
> > anything. Thus, it would cause a compiler error if a function name was
> > mistyped and thus not picked up by "generate.py".
> >
> 
> > +$(UNIT_TEST_DIR)/clar.suite: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(UNIT_TESTS_SUITES)) GIT-TEST-SUITES
> > +	$(QUIET_GEN)$(UNIT_TEST_DIR)/clar/generate.py $(UNIT_TEST_DIR) >/dev/null
> 
> This uses the Python interpreter from the shebang line in generate.py,
> which is python.  On my system I only have python3 and python3.12, but
> not python.  Easily fixed, of course, but a way to configure the
> interpreter name would be nice.
> 
> This gave me extra motivation to come up with the clunky patch below
> to replace Python with sed and awk.  That and your statement that clar
> doesn't have to be perfect in the other thread. ;)

Neat! I would certainly prefer to not have a dependency on Python, and I
think awk(1) is a good alternative here that we already require anyway.

Also, another benefit of having our own script is that it allows us to
be more flexible with how exactly our tests are structured.

> It reverses the order of dependencies (builds clar-decls.h first), not
> sure if that has downsides.  And the sed pattern is simpler than the
> one in generate.py, just out of laziness.

We could even integrate the generation of clar-decls.h with the AWK
script.

I'll play around a bit with what you have, thanks a lot for working on
it!

Patrick
Patrick Steinhardt Aug. 1, 2024, 9:32 a.m. UTC | #5 
On Wed, Jul 31, 2024 at 02:39:13PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > A trivial fix-up to be squashed into this step.
> 
> I won't be able to address this today, but "make sparse" and other
> auxiliary targets also seem to break, due to their lack of
> dependence on the generated clar.suite file.

Yeah, as said in the cover letter, this was in quite an unpolished
state, so it is not surprising at all that things break. I just wanted
to provide an unfinished PoC to demonstrate how things would roughly
look like so that we have something to discuss. Definitely not material
for `seen` yet.

Given that initial feedback didn't seem to be all that negative I'm
happy to polish things a bit more.

Patrick
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index 8caf3700c2..6687bd6db4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,6 +9,7 @@ 
 /GIT-PYTHON-VARS
 /GIT-SCRIPT-DEFINES
 /GIT-SPATCH-DEFINES
+/GIT-TEST-SUITES
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index d6479092a0..658acb4d48 100644
--- a/Makefile
+++ b/Makefile
@@ -1332,6 +1332,11 @@  THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
+UNIT_TESTS_PROG = $(UNIT_TEST_BIN)/unit-tests$(X)
+UNIT_TESTS_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TESTS_SUITES))
+UNIT_TESTS_OBJS += $(UNIT_TEST_DIR)/clar/clar.o
+UNIT_TESTS_OBJS += $(UNIT_TEST_DIR)/unit-test.o
+
 UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGRAMS += t-example-decorate
 UNIT_TEST_PROGRAMS += t-hash
@@ -2711,6 +2716,7 @@  OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
 OBJECTS += $(UNIT_TEST_OBJS)
+OBJECTS += $(UNIT_TESTS_OBJS)
 
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
@@ -3213,7 +3219,7 @@  endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
-all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
+all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS) $(UNIT_TESTS_PROG)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
@@ -3644,7 +3650,7 @@  endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
-		$(UNIT_TEST_PROGS) $(MOFILES)
+		$(UNIT_TEST_PROGS) $(UNIT_TESTS_PROG) $(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 	test -n "$(ARTIFACTS_DIRECTORY)"
@@ -3700,6 +3706,7 @@  cocciclean:
 
 clean: profile-clean coverage-clean cocciclean
 	$(RM) -r .build $(UNIT_TEST_BIN)
+	$(RM) $(UNIT_TEST_DIR)/clar.suite $(UNIT_TEST_DIR)/.clarcache
 	$(RM) po/git.pot po/git-core.pot
 	$(RM) git.res
 	$(RM) $(OBJECTS)
@@ -3859,7 +3866,26 @@  $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
 
+GIT-TEST-SUITES: FORCE
+	@FLAGS='$(UNIT_TESTS_SUITES)'; \
+	    if test x"$$FLAGS" != x"`cat GIT-TEST-SUITES 2>/dev/null`" ; then \
+		echo >&2 "    * new test suites"; \
+		echo "$$FLAGS" >GIT-TEST-SUITES; \
+		rm -f $(UNIT_TESTS_DIR)/.clarcache; \
+            fi
+
+$(UNIT_TEST_DIR)/clar.suite: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(UNIT_TESTS_SUITES)) GIT-TEST-SUITES
+	$(QUIET_GEN)$(UNIT_TEST_DIR)/clar/generate.py $(UNIT_TEST_DIR) >/dev/null
+	@touch $@
+$(UNIT_TEST_DIR)/clar-decls.h: $(UNIT_TEST_DIR)/clar.suite
+	$(QUIET_GEN)grep '^extern void' $^ >$@
+$(UNIT_TESTS_OBJS): $(UNIT_TEST_DIR)/clar-decls.h
+$(UNIT_TESTS_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) -I$(UNIT_TEST_DIR)/clar
+$(UNIT_TESTS_PROG): $(UNIT_TEST_DIR)/clar.suite $(UNIT_TESTS_OBJS) $(GITLIBS) GIT-LDFLAGS
+	$(call mkdir_p_parent_template)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
+
 .PHONY: build-unit-tests unit-tests
-build-unit-tests: $(UNIT_TEST_PROGS)
-unit-tests: $(UNIT_TEST_PROGS) t/helper/test-tool$X
+build-unit-tests: $(UNIT_TEST_PROGS) $(UNIT_TESTS_PROG)
+unit-tests: $(UNIT_TEST_PROGS) $(UNIT_TESTS_PROG) t/helper/test-tool$X
 	$(MAKE) -C t/ unit-tests
diff --git a/t/Makefile b/t/Makefile
index 4c30e7c06f..55f740ae29 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -48,6 +48,7 @@  CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.tes
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
 UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
+UNIT_TEST_PROGRAMS += unit-tests/bin/unit-tests$(X)
 UNIT_TESTS = $(sort $(UNIT_TEST_PROGRAMS))
 UNIT_TESTS_NO_DIR = $(notdir $(UNIT_TESTS))
 
diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
index 5e56e040ec..b8d46f7bb1 100644
--- a/t/unit-tests/.gitignore
+++ b/t/unit-tests/.gitignore
@@ -1 +1,4 @@ 
 /bin
+/.clarcache
+/clar.suite
+/clar-decls.h
diff --git a/t/unit-tests/unit-test.c b/t/unit-tests/unit-test.c
new file mode 100644
index 0000000000..d7eecc384c
--- /dev/null
+++ b/t/unit-tests/unit-test.c
@@ -0,0 +1,16 @@ 
+#include "unit-test.h"
+
+int cmd_main(int argc, const char **argv)
+{
+	const char **args;
+	int ret;
+
+	/* Append the "-t" flag such that the tests generate TAP output. */
+	DUP_ARRAY(args, argv, argc + 1);
+	args[argc++] = "-t";
+
+	ret = clar_test(argc, (char **) args);
+
+	free(args);
+	return ret;
+}
diff --git a/t/unit-tests/unit-test.h b/t/unit-tests/unit-test.h
new file mode 100644
index 0000000000..99d59df1b0
--- /dev/null
+++ b/t/unit-tests/unit-test.h
@@ -0,0 +1,3 @@ 
+#include "git-compat-util.h"
+#include "clar.h"
+#include "clar-decls.h"