diff mbox series

[RFC,v2,4/4] unit test: add basic example and build rules

Message ID 20230517-unit-tests-v2-v2-4-21b5b60f4b32@google.com (mailing list archive)
State New, archived
Headers show
Series Add an external testing library for unit tests | expand

Commit Message

Josh Steadmon May 17, 2023, 11:56 p.m. UTC 
Integrate a simple strbuf unit test with Git's Makefiles.

You can build and run the unit tests with `make unit-tests` (or just
build them with `make build-unit-tests`). By default we use the basic
test runner from the C-TAP project, but users who prefer prove as a test
runner can set `DEFAULT_UNIT_TEST_TARGET=prove-unit-tests` instead.

We modify the `#include`s in the C TAP libraries so that we can build
them without having to include the t/ directory in our include search
path.

Signed-off-by: Calvin Wan <calvinwan@google.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
Change-Id: Ie61eafd2bd8f8dc5b30449af1e436889f91da3b7
---
 .gitignore      |  2 ++
 Makefile        | 24 +++++++++++++++++++++++-
 t/Makefile      | 10 ++++++++++
 t/strbuf-test.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 t/tap/basic.c   |  2 +-
 t/tap/basic.h   |  2 +-
 6 files changed, 91 insertions(+), 3 deletions(-)

Comments

Phillip Wood May 18, 2023, 1:32 p.m. UTC | #1 
On 18/05/2023 00:56, steadmon@google.com wrote:
> Integrate a simple strbuf unit test with Git's Makefiles.
> 
> You can build and run the unit tests with `make unit-tests` (or just
> build them with `make build-unit-tests`). By default we use the basic
> test runner from the C-TAP project, but users who prefer prove as a test
> runner can set `DEFAULT_UNIT_TEST_TARGET=prove-unit-tests` instead.
> 
> We modify the `#include`s in the C TAP libraries so that we can build
> them without having to include the t/ directory in our include search
> path.

Thanks for adding some example tests, it is really helpful to see how 
the library will be used.

I tried building the units test with SANITIZE=address set and I get lots 
of link errors complaining about undefined references to __asan_*

> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> Change-Id: Ie61eafd2bd8f8dc5b30449af1e436889f91da3b7

> diff --git a/t/strbuf-test.c b/t/strbuf-test.c
> new file mode 100644
> index 0000000000..8f8d4e11db
> --- /dev/null
> +++ b/t/strbuf-test.c
> @@ -0,0 +1,54 @@
> +#include "tap/basic.h"
> +
> +#include "../git-compat-util.h"
> +#include "../strbuf.h"
> +
> +int strbuf_init_test()
> +{
> +	struct strbuf *buf = malloc(sizeof(void*));

Is there a reason to use dynamic allocation here. Also I think you need 
sizeof(*buf) to allocate the correct size.

> +	strbuf_init(buf, 0);
> +
> +	if (buf->buf[0] != '\0')
> +		return 0;
> +	if (buf->alloc != 0)
> +		return 0;
> +	if (buf->len != 0)
> +		return 0;
> +	return 1;
> +}

This test nicely illustrates why I'd prefer a different approach. The 
test author has to maintain the pass/fail state and there are no 
diagnostics if it fails to tell you which check failed. To be clear I 
view the lack of diagnostics as the fault of the test framework, not the 
test author. I'd prefer something like

	void strbuf_init_test(void)
	{
		struct strbuf buf;

		strbuf_init(&buf, 0);
		check_char(buf.buf[0] == '\0');
		check_uint(buf.alloc, ==, 0);
		check_uint(buf.len, ==, 0);
	}

which would be run as

	TEST(strbuf_init_test(), "strbuf_init initializes properly");

in main() and provide diagnostics like

     # check "buf.alloc == 0" failed at my-test.c:102
     #    left: 2
     #   right: 0

when a check fails.

> +int strbuf_init_test2() {
> +	struct strbuf *buf = malloc(sizeof(void*));
> +	strbuf_init(buf, 100);
> +
> +	if (buf->buf[0] != '\0')
> +		return 0;
> +	if (buf->alloc != 101)

Strictly speaking I think the API guarantees that at least 100 bytes 
will be allocated, not the exact amount as does alloc_grow() below.

> +		return 0;
> +	if (buf->len != 0)
> +		return 0;
> +	return 1;
> +}
> +
> +
> +int strbuf_grow_test() {
> +	struct strbuf *buf = malloc(sizeof(void*));
> +	strbuf_grow(buf, 100);
> +
> +	if (buf->buf[0] != '\0')
> +		return 0;
> +	if (buf->alloc != 101)
> +		return 0;
> +	if (buf->len != 0)
> +		return 0;
> +	return 1;
> +}

Best Wishes

Phillip
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index e875c59054..464e301345 100644
--- a/.gitignore
+++ b/.gitignore
@@ -245,3 +245,5 @@  Release/
 /git.VC.db
 *.dSYM
 /contrib/buildsystems/out
+/t/runtests
+/t/unit-tests/
diff --git a/Makefile b/Makefile
index 8ee7c7e5a8..aa94e3ba45 100644
--- a/Makefile
+++ b/Makefile
@@ -661,6 +661,7 @@  BUILTIN_OBJS =
 BUILT_INS =
 COMPAT_CFLAGS =
 COMPAT_OBJS =
+CTAP_OBJS =
 XDIFF_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
@@ -682,6 +683,8 @@  TEST_BUILTINS_OBJS =
 TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
+UNIT_TEST_PROGRAMS =
+UNIT_TEST_DIR = t/unit-tests
 
 # Having this variable in your environment would break pipelines because
 # you cause "cd" to echo its destination to stdout.  It can also take
@@ -1318,6 +1321,10 @@  BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/worktree.o
 BUILTIN_OBJS += builtin/write-tree.o
 
+CTAP_OBJS += t/tap/basic.o
+UNIT_TEST_RUNNER = t/runtests
+UNIT_TEST_PROGRAMS += $(UNIT_TEST_DIR)/strbuf-test-t
+
 # THIRD_PARTY_SOURCES is a list of patterns compatible with the
 # $(filter) and $(filter-out) family of functions. They specify source
 # files which are taken from some third-party source where we want to be
@@ -2673,6 +2680,7 @@  OBJECTS += $(TEST_OBJS)
 OBJECTS += $(XDIFF_OBJS)
 OBJECTS += $(FUZZ_OBJS)
 OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
+OBJECTS += $(CTAP_OBJS)
 
 ifndef NO_CURL
 	OBJECTS += http.o http-walker.o remote-curl.o
@@ -3654,7 +3662,7 @@  clean: profile-clean coverage-clean cocciclean
 	$(RM) $(OBJECTS)
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
-	$(RM) $(TEST_PROGRAMS)
+	$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_RUNNER) $(UNIT_TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
 	$(RM) $(HCC)
@@ -3832,3 +3840,17 @@  $(FUZZ_PROGRAMS): all
 		$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
 
 fuzz-all: $(FUZZ_PROGRAMS)
+
+$(UNIT_TEST_DIR):
+	$(QUIET)mkdir $(UNIT_TEST_DIR)
+
+$(UNIT_TEST_PROGRAMS): $(UNIT_TEST_DIR) $(CTAP_OBJS) $(GITLIBS)
+	$(QUIET_CC)$(CC) -o $@ t/$(patsubst %-t,%,$(notdir $@)).c $(CTAP_OBJS) $(LIBS)
+
+$(UNIT_TEST_RUNNER): $(patsubst %,%.c,$(UNIT_TEST_RUNNER))
+	$(QUIET_CC)$(CC) -o $@ $^
+
+.PHONY: build-unit-tests unit-tests
+build-unit-tests: $(UNIT_TEST_PROGRAMS)
+unit-tests: $(UNIT_TEST_PROGRAMS) $(UNIT_TEST_RUNNER)
+	$(MAKE) -C t/ unit-tests
diff --git a/t/Makefile b/t/Makefile
index 3e00cdd801..9df1a4e34b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -17,6 +17,7 @@  TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove
 DEFAULT_TEST_TARGET ?= test
+DEFAULT_UNIT_TEST_TARGET ?= run-unit-tests
 TEST_LINT ?= test-lint
 
 ifdef TEST_OUTPUT_DIRECTORY
@@ -41,6 +42,7 @@  TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
+UNIT_TESTS = $(sort $(wildcard unit-tests/*))
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
 # checks all tests in all scripts via a single invocation, so tell individual
@@ -65,6 +67,14 @@  prove: pre-clean check-chainlint $(TEST_LINT)
 $(T):
 	@echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 
+unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
+
+run-unit-tests:
+	./runtests $(UNIT_TESTS)
+
+prove-unit-tests:
+	@echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
+
 pre-clean:
 	$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
 
diff --git a/t/strbuf-test.c b/t/strbuf-test.c
new file mode 100644
index 0000000000..8f8d4e11db
--- /dev/null
+++ b/t/strbuf-test.c
@@ -0,0 +1,54 @@ 
+#include "tap/basic.h"
+
+#include "../git-compat-util.h"
+#include "../strbuf.h"
+
+int strbuf_init_test()
+{
+	struct strbuf *buf = malloc(sizeof(void*));
+	strbuf_init(buf, 0);
+
+	if (buf->buf[0] != '\0')
+		return 0;
+	if (buf->alloc != 0)
+		return 0;
+	if (buf->len != 0)
+		return 0;
+	return 1;
+}
+
+int strbuf_init_test2() {
+	struct strbuf *buf = malloc(sizeof(void*));
+	strbuf_init(buf, 100);
+
+	if (buf->buf[0] != '\0')
+		return 0;
+	if (buf->alloc != 101)
+		return 0;
+	if (buf->len != 0)
+		return 0;
+	return 1;
+}
+
+
+int strbuf_grow_test() {
+	struct strbuf *buf = malloc(sizeof(void*));
+	strbuf_grow(buf, 100);
+
+	if (buf->buf[0] != '\0')
+		return 0;
+	if (buf->alloc != 101)
+		return 0;
+	if (buf->len != 0)
+		return 0;
+	return 1;
+}
+
+int main(void)
+{
+	plan(3);
+	ok(strbuf_init_test(), "strbuf_init initializes properly");
+	ok(strbuf_init_test2(), "strbuf_init with hint initializes properly");
+	ok(strbuf_grow_test(), "strbuf_grow grows properly");
+	return 0;
+}
diff --git a/t/tap/basic.c b/t/tap/basic.c
index 704282b9c1..37c2d6f082 100644
--- a/t/tap/basic.c
+++ b/t/tap/basic.c
@@ -52,7 +52,7 @@ 
 #include <sys/types.h>
 #include <unistd.h>
 
-#include <tap/basic.h>
+#include "basic.h"
 
 /* Windows provides mkdir and rmdir under different names. */
 #ifdef _WIN32
diff --git a/t/tap/basic.h b/t/tap/basic.h
index afea8cb210..a0c0ef2c87 100644
--- a/t/tap/basic.h
+++ b/t/tap/basic.h
@@ -36,7 +36,7 @@ 
 #include <stdarg.h> /* va_list */
 #include <stddef.h> /* size_t */
 #include <stdlib.h> /* free */
-#include <tap/macros.h>
+#include "macros.h"
 
 /*
  * Used for iterating through arrays.  ARRAY_SIZE returns the number of