diff mbox series

[RFC,1/4] t0080: turn t-basic unit test into a helper

Message ID a9f67ed703c8314f0f0507ffb83b503717b846b3.1705443632.git.steadmon@google.com (mailing list archive)
State Superseded
Headers show
Series test-tool: add unit test suite runner | expand

Commit Message

Josh Steadmon Jan. 16, 2024, 10:22 p.m. UTC
While t/unit-tests/t-basic.c uses the unit-test framework added in
e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is
not a true unit test in that it intentionally fails in order to exercise
various codepaths in the unit-test framework. Thus, we intentionally
exclude it when running unit tests through the various t/Makefile
targets. Instead, it is executed by t0080-unit-test-output.sh, which
verifies its output follows the TAP format expected for the various
pass, skip, or fail cases.

As such, it makes more sense for t-basic to be a helper item for
t0080-unit-test-output.sh, so let's move it to t/t0080/t-basic.c and
adjust Makefiles and .gitignores as necessary.

This has the additional benefit that test harnesses seeking to run all
unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
with no exceptions needed. This will be important in a later patch where
we add support for running the unit tests via a test-tool subcommand.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Makefile                          | 17 +++++++++++++----
 t/Makefile                        |  2 +-
 t/t0080-unit-test-output.sh       | 24 ++++++++++++------------
 t/t0080/.gitignore                |  1 +
 t/{unit-tests => t0080}/t-basic.c |  2 +-
 5 files changed, 28 insertions(+), 18 deletions(-)
 create mode 100644 t/t0080/.gitignore
 rename t/{unit-tests => t0080}/t-basic.c (98%)

Comments

Junio C Hamano Jan. 16, 2024, 10:54 p.m. UTC | #1
Josh Steadmon <steadmon@google.com> writes:

> As such, it makes more sense for t-basic to be a helper item for
> t0080-unit-test-output.sh, ...

Up to this part was very understandable and agreeable, but I was
surprised to see ...

> ... so let's move it to t/t0080/t-basic.c and
> adjust Makefiles and .gitignores as necessary.

... this conclusion.  I somehow thought that t-basic part would be a
good test-tool subcommand, as it is run from the suite of shell
scripts for end-to-end testing CLI interaction.

Do we have any precedent to place programs placed under t/tXXXX/ and
get them compiled?

> This has the additional benefit that test harnesses seeking to run all
> unit tests can find them with a simple glob of "t/unit-tests/bin/t-*",
> with no exceptions needed.

And this motivation is very much understandable, and for that, as
long as it is outside t/unit-tests/, it would be good.  I just did
not expect it to hang under t/t0080/, inviting more unit test pieces
that are triggerable from the numberd shell scripts to hang under
random t/t[0-9][0-9][0-9][0-9]/ directories.
Jeff King Jan. 23, 2024, 12:43 a.m. UTC | #2
On Tue, Jan 16, 2024 at 02:54:21PM -0800, Junio C Hamano wrote:

> > ... so let's move it to t/t0080/t-basic.c and
> > adjust Makefiles and .gitignores as necessary.
> 
> ... this conclusion.  I somehow thought that t-basic part would be a
> good test-tool subcommand, as it is run from the suite of shell
> scripts for end-to-end testing CLI interaction.

Heh, I was about to ask the same thing.

In particular...

> Do we have any precedent to place programs placed under t/tXXXX/ and
> get them compiled?

...no, I don't think we do. And quite often I exclude those directories
when grepping around the code base, because there is often code there
that is purely used as a data fixture. E.g., t4256 contains a copy of
mailinfo.c which it uses as input for some of the tests. That code also
happens to have out-of-bounds memory reads which we have since fixed in
the real mailinfo.c, but of course "grep" finds them both. :)

So I would prefer a rule like "no buildable code in t/t[0-9]*". Barring
that, maybe we could avoid using things that look too much like real Git
code in our tests (though we sometimes do need fake code for things like
funclines, and even that might end up creating false positives).

-Peff
Josh Steadmon Feb. 1, 2024, 7:40 p.m. UTC | #3
On 2024.01.22 19:43, Jeff King wrote:
> On Tue, Jan 16, 2024 at 02:54:21PM -0800, Junio C Hamano wrote:
> 
> > > ... so let's move it to t/t0080/t-basic.c and
> > > adjust Makefiles and .gitignores as necessary.
> > 
> > ... this conclusion.  I somehow thought that t-basic part would be a
> > good test-tool subcommand, as it is run from the suite of shell
> > scripts for end-to-end testing CLI interaction.
> 
> Heh, I was about to ask the same thing.
> 
> In particular...
> 
> > Do we have any precedent to place programs placed under t/tXXXX/ and
> > get them compiled?
> 
> ...no, I don't think we do. And quite often I exclude those directories
> when grepping around the code base, because there is often code there
> that is purely used as a data fixture. E.g., t4256 contains a copy of
> mailinfo.c which it uses as input for some of the tests. That code also
> happens to have out-of-bounds memory reads which we have since fixed in
> the real mailinfo.c, but of course "grep" finds them both. :)
> 
> So I would prefer a rule like "no buildable code in t/t[0-9]*". Barring
> that, maybe we could avoid using things that look too much like real Git
> code in our tests (though we sometimes do need fake code for things like
> funclines, and even that might end up creating false positives).
> 
> -Peff

Ack. I've moved this to a test-tool subcommand for V2, which I hope to
send out soon.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 88ba7a3c51..ab32ec1101 100644
--- a/Makefile
+++ b/Makefile
@@ -683,6 +683,7 @@  TEST_OBJS =
 TEST_PROGRAMS_NEED_X =
 THIRD_PARTY_SOURCES =
 UNIT_TEST_PROGRAMS =
+UNIT_TEST_HELPERS =
 UNIT_TEST_DIR = t/unit-tests
 UNIT_TEST_BIN = $(UNIT_TEST_DIR)/bin
 
@@ -1339,10 +1340,12 @@  THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
-UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-strbuf
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_HELPERS += t/t0080/t-basic
+UNIT_TEST_HELPER_PROGS = $(patsubst %,%$X,$(UNIT_TEST_HELPERS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
+UNIT_TEST_OBJS += $(patsubst %,%.o,$(UNIT_TEST_HELPERS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
 
 # xdiff and reftable libs may in turn depend on what is in libgit.a
@@ -3189,7 +3192,9 @@  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)
+
+all:: $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS)
 
 bin-wrappers/%: wrap-for-bin.sh
 	$(call mkdir_p_parent_template)
@@ -3620,7 +3625,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_TEST_HELPER_PROGS) $(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 	test -n "$(ARTIFACTS_DIRECTORY)"
@@ -3682,7 +3687,7 @@  clean: profile-clean coverage-clean cocciclean
 	$(RM) headless-git.o
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
-	$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
+	$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS) $(UNIT_TEST_HELPER_PROGS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
 	$(RM) $(HCC)
@@ -3869,6 +3874,10 @@  $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
 
+$(UNIT_TEST_HELPER_PROGS): %$X: %.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+		$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+
 .PHONY: build-unit-tests unit-tests
 build-unit-tests: $(UNIT_TEST_PROGS)
 unit-tests: $(UNIT_TEST_PROGS)
diff --git a/t/Makefile b/t/Makefile
index b7a6fefe28..0bee7bc6ea 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -42,7 +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 $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.pdb,$(wildcard unit-tests/bin/t-*)))
 
 # `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
diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..7431023d97 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -8,50 +8,50 @@  test_expect_success 'TAP output from unit tests' '
 	cat >expect <<-EOF &&
 	ok 1 - passing test
 	ok 2 - passing test and assertion return 1
-	# check "1 == 2" failed at t/unit-tests/t-basic.c:76
+	# check "1 == 2" failed at t/t0080/t-basic.c:76
 	#    left: 1
 	#   right: 2
 	not ok 3 - failing test
 	ok 4 - failing test and assertion return 0
 	not ok 5 - passing TEST_TODO() # TODO
 	ok 6 - passing TEST_TODO() returns 1
-	# todo check ${SQ}check(x)${SQ} succeeded at t/unit-tests/t-basic.c:25
+	# todo check ${SQ}check(x)${SQ} succeeded at t/t0080/t-basic.c:25
 	not ok 7 - failing TEST_TODO()
 	ok 8 - failing TEST_TODO() returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:30
+	# check "0" failed at t/t0080/t-basic.c:30
 	# skipping test - missing prerequisite
-	# skipping check ${SQ}1${SQ} at t/unit-tests/t-basic.c:32
+	# skipping check ${SQ}1${SQ} at t/t0080/t-basic.c:32
 	ok 9 - test_skip() # SKIP
 	ok 10 - skipped test returns 1
 	# skipping test - missing prerequisite
 	ok 11 - test_skip() inside TEST_TODO() # SKIP
 	ok 12 - test_skip() inside TEST_TODO() returns 1
-	# check "0" failed at t/unit-tests/t-basic.c:48
+	# check "0" failed at t/t0080/t-basic.c:48
 	not ok 13 - TEST_TODO() after failing check
 	ok 14 - TEST_TODO() after failing check returns 0
-	# check "0" failed at t/unit-tests/t-basic.c:56
+	# check "0" failed at t/t0080/t-basic.c:56
 	not ok 15 - failing check after TEST_TODO()
 	ok 16 - failing check after TEST_TODO() returns 0
-	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/unit-tests/t-basic.c:61
+	# check "!strcmp("\thello\\\\", "there\"\n")" failed at t/t0080/t-basic.c:61
 	#    left: "\011hello\\\\"
 	#   right: "there\"\012"
-	# check "!strcmp("NULL", NULL)" failed at t/unit-tests/t-basic.c:62
+	# check "!strcmp("NULL", NULL)" failed at t/t0080/t-basic.c:62
 	#    left: "NULL"
 	#   right: NULL
-	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/unit-tests/t-basic.c:63
+	# check "${SQ}a${SQ} == ${SQ}\n${SQ}" failed at t/t0080/t-basic.c:63
 	#    left: ${SQ}a${SQ}
 	#   right: ${SQ}\012${SQ}
-	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/unit-tests/t-basic.c:64
+	# check "${SQ}\\\\${SQ} == ${SQ}\\${SQ}${SQ}" failed at t/t0080/t-basic.c:64
 	#    left: ${SQ}\\\\${SQ}
 	#   right: ${SQ}\\${SQ}${SQ}
 	not ok 17 - messages from failing string and char comparison
-	# BUG: test has no checks at t/unit-tests/t-basic.c:91
+	# BUG: test has no checks at t/t0080/t-basic.c:91
 	not ok 18 - test with no checks
 	ok 19 - test with no checks returns 0
 	1..19
 	EOF
 
-	! "$GIT_BUILD_DIR"/t/unit-tests/bin/t-basic >actual &&
+	! "$GIT_BUILD_DIR"/t/t0080/t-basic >actual &&
 	test_cmp expect actual
 '
 
diff --git a/t/t0080/.gitignore b/t/t0080/.gitignore
new file mode 100644
index 0000000000..1903542827
--- /dev/null
+++ b/t/t0080/.gitignore
@@ -0,0 +1 @@ 
+/t-basic
diff --git a/t/unit-tests/t-basic.c b/t/t0080/t-basic.c
similarity index 98%
rename from t/unit-tests/t-basic.c
rename to t/t0080/t-basic.c
index fda1ae59a6..83727221b1 100644
--- a/t/unit-tests/t-basic.c
+++ b/t/t0080/t-basic.c
@@ -1,4 +1,4 @@ 
-#include "test-lib.h"
+#include "../unit-tests/test-lib.h"
 
 /*
  * The purpose of this "unit test" is to verify a few invariants of the unit