CMake(Visual C) support for js/doc-unit-tests

The recent patch series that adds proper unit testing to Git requires a couple of add-on patches to make it work with the CMake build on Windows (Visual C). This patch series aims to provide that support.

This patch series is based on js/doc-unit-tests.

Johannes Schindelin (4):
cmake: also build unit tests
unit-tests: do not mistake .pdb files for being executable
unit-tests: do show relative file paths
artifacts-tar: when including .dll files, don't forget the
unit-tests

 Makefile                            |  2 +-
 contrib/buildsystems/CMakeLists.txt | 18 +++++++++++
 t/Makefile                          |  2 +-
 t/unit-tests/test-lib.c             | 48 ++++++++++++++++++++++++++---
 4 files changed, 64 insertions(+), 6 deletions(-)

base-commit: 03f9bc407975bba86d1d1807519d76e1693ff66f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1579%2Fdscho%2Fdoc-unit-tests-and-cmake-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1579/dscho/doc-unit-tests-and-cmake-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1579

gitgitgadget

From: Johannes Schindelin <[email protected]>

A new, better way to run unit tests was just added to Git. This adds support for building those unit tests via CMake.

---
 contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

index 2f6e0197ffa..45016213358 100644
@@ -965,6 +965,24 @@ target_link_libraries(test-fake-ssh common-main)
 parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
 list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 
+#unit-tests
+add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
+
+parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
+foreach(unit_test ${unit_test_PROGRAMS})
+	add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
+	target_link_libraries("${unit_test}" unit-test-lib common-main)
+	set_target_properties("${unit_test}"
+		PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests)
+	if(MSVC)
+		set_target_properties("${unit_test}"
+			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/unit-tests)
+		set_target_properties("${unit_test}"
+			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
+	endif()
+	list(APPEND PROGRAMS_BUILT "${unit_test}")
+endforeach()
+
 #test-tool
 parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
 

gitgitgadget

Hi Johannes

This patch builds the unit tests but does not add them to the list of tests run by CTest - how are the tests typically run on the CMake build?

Best Wishes

Phillip

Hi Phillip,

You're right, I missed that the unit tests are run as part not of t0080, but as a separate target in t/Makefile.

I've added a couple of patches to clean up the CTest part and then run the unit test (t-strbuf, and whatever is added to the UNIT_TEST_PROGRAMS variable in Makefile).

Thank you,
Johannes

From: Johannes Schindelin <[email protected]>

When building the unit tests via CMake, the .pdb files are built. Those are, essentially, files containing the debug information separately from the executables.

Let's not confuse them with the executables we actually want to run.

---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 095334bfdec..38fe0ded5bd 100644
@@ -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 %.h %.c %.o unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.h %.c %.o %.pdb unit-tests/t-basic%,$(wildcard unit-tests/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

gitgitgadget

From: Johannes Schindelin <[email protected]>

Visual C interpolates __FILE__ with the absolute _Windows_ path of the source file. GCC interpolates it with the relative path, and the tests even verify that.

So let's make sure that the unit tests only emit such paths.

---
 t/unit-tests/test-lib.c | 48 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

index 70030d587ff..bd94079af61 100644
@@ -21,6 +21,42 @@ static struct {
 	.result = RESULT_NONE,
 };
 
+#ifndef _MSC_VER
+#define make_relative(location) location
+#else
+/*
+ * Visual C interpolates the absolute Windows path for `__FILE__`,
+ * but we want to see relative paths, as verified by t0080.
+ */
+#include "strbuf.h"
+#include "dir.h"
+
+static const char *make_relative(const char *location)
+{
+	static const char *prefix;
+	static size_t prefix_len;
+	static struct strbuf buf = STRBUF_INIT;
+
+	if (!prefix) {
+		strbuf_addstr(&buf, __FILE__);
+		if (!strbuf_strip_suffix(&buf, "\\t\\unit-tests\\test-lib.c"))
+			die("unexpected suffix of '%s'");
+		strbuf_complete(&buf, '\\');
+		prefix = strbuf_detach(&buf, &prefix_len);
+	}
+
+	/* Does it not start with the expected prefix? */
+	if (fspathncmp(location, prefix, prefix_len))
+		return location;
+
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, location + prefix_len);
+	convert_slashes(buf.buf);
+
+	return buf.buf;
+}
+#endif
+
 static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
 {
 	fflush(stderr);

@@ -147,7 +183,8 @@ int test__run_end(int was_run UNUSED, const char *location, const char *format,
 			break;
 
 		case RESULT_NONE:
-			test_msg("BUG: test has no checks at %s", location);
+			test_msg("BUG: test has no checks at %s",
+				 make_relative(location));
 			printf("not ok %d", ctx.count);
 			print_description(format, ap);
 			ctx.result = RESULT_FAILURE;

@@ -193,13 +230,15 @@ int test_assert(const char *location, const char *check, int ok)
 	assert(ctx.running);
 
 	if (ctx.result == RESULT_SKIP) {
-		test_msg("skipping check '%s' at %s", check, location);
+		test_msg("skipping check '%s' at %s", check,
+			 make_relative(location));
 		return 0;
 	} else if (!ctx.todo) {
 		if (ok) {
 			test_pass();
 		} else {
-			test_msg("check \"%s\" failed at %s", check, location);
+			test_msg("check \"%s\" failed at %s", check,
+				 make_relative(location));
 			test_fail();
 		}
 	}

@@ -224,7 +263,8 @@ int test__todo_end(const char *location, const char *check, int res)
 	if (ctx.result == RESULT_SKIP)
 		return 0;
 	if (!res) {
-		test_msg("todo check '%s' succeeded at %s", check, location);
+		test_msg("todo check '%s' succeeded at %s", check,
+			 make_relative(location));
 		test_fail();
 	} else {
 		test_todo();

gitgitgadget

Hi Johannes

Oh, that's a pain

Makes sense

{
	static char prefix[] = __FILE__
	static size_t *prefix_len = (size_t)-1;
	static char buf[PATH_MAX];

	if (prefix == (size_t)-1) {
		const char *path = "\\t\unit-tests\\test-lib.c";
		size_t path_len = strlen(path);
		
		prefix_len = strlen(prefix);
		if (prefix_len < path_len) ||
		    memcmp(prefix + prefix_len - path_len, path, path_len)
			die(...);
		prefix_len -= path_len - 1; /* keep trailing '\\' */
		prefix[prefix_len] = '\0';
	}

	/* Does it not start with the expected prefix? */
	if (fspathncmp(location, prefix, prefix_len))
		return location;

	if (strlen(location) - prefix_len > sizeof(buf) - 1)
		die(...)

	/* +1 to copy NUL terminator */
	memcpy(buf, location + prefix_len, strlen(location) - prefix_len + 1);
	convert_slashes(buf);

	return buf;
}

Hi Phillip,

I changed the code not to use a strbuf, and I'm now working exclusively on static buffers instead of malloc()ing anything.

Thank you,
Johannes

From: Johannes Schindelin <[email protected]>

As of recent, Git also builds executables in t/unit-tests/. For technical reasons, when building with CMake and Visual C, the dependencies (".dll files") need to be copied there, too, otherwise running the executable will fail "due to missing dependencies".

The CMake definition already contains the directives to copy those .dll files, but we also need to adjust the artifacts-tar rule in the Makefile accordingly to let the vs-test job in the CI runs pass successfully.

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4016da6e39c..d95a7b19b50 100644
@@ -3596,7 +3596,7 @@ rpm::
 .PHONY: rpm
 
 ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \

gitgitgadget

The recent patch series that adds proper unit testing to Git requires a couple of add-on patches to make it work with the CMake build on Windows (Visual C). This patch series aims to provide that support.

This patch series is based on js/doc-unit-tests.

Changes since v1:

* The code added to test-lib.c now avoids using a strbuf.
* The unit tests are now also handled via CTest.
* While at it, I cleaned up a little in the CTest-related definitions.

Johannes Schindelin (7):
cmake: also build unit tests
unit-tests: do not mistake .pdb files for being executable
unit-tests: do show relative file paths
artifacts-tar: when including .dll files, don't forget the
unit-tests
cmake: fix typo in variable name
cmake: use test names instead of full paths
cmake: handle also unit tests

 Makefile                            |  2 +-
 contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++---
 t/Makefile                          |  2 +-
 t/unit-tests/test-lib.c             | 52 ++++++++++++++++++++++++++---
 4 files changed, 84 insertions(+), 10 deletions(-)

base-commit: 03f9bc407975bba86d1d1807519d76e1693ff66f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1579%2Fdscho%2Fdoc-unit-tests-and-cmake-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1579/dscho/doc-unit-tests-and-cmake-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1579

Range-diff vs v1:

 1:  2cc1c03d851 = 1:  2cc1c03d851 cmake: also build unit tests
 2:  90db3d5d41f = 2:  90db3d5d41f unit-tests: do not mistake `.pdb` files for being executable
 3:  2b4e36c05c9 ! 3:  f0b804129e8 unit-tests: do show relative file paths
     @@ t/unit-tests/test-lib.c: static struct {
      + * Visual C interpolates the absolute Windows path for `__FILE__`,
      + * but we want to see relative paths, as verified by t0080.
      + */
     -+#include "strbuf.h"
      +#include "dir.h"
      +
      +static const char *make_relative(const char *location)
      +{
     -+	static const char *prefix;
     ++	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
      +	static size_t prefix_len;
     -+	static struct strbuf buf = STRBUF_INIT;
      +
     -+	if (!prefix) {
     -+		strbuf_addstr(&buf, __FILE__);
     -+		if (!strbuf_strip_suffix(&buf, "\\t\\unit-tests\\test-lib.c"))
     -+			die("unexpected suffix of '%s'");
     -+		strbuf_complete(&buf, '\\');
     -+		prefix = strbuf_detach(&buf, &prefix_len);
     ++	if (!prefix_len) {
     ++		size_t len = strlen(prefix);
     ++		const char *needle = "\\t\\unit-tests\\test-lib.c";
     ++		size_t needle_len = strlen(needle);
     ++
     ++		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
     ++			die("unexpected suffix of '%s'", prefix);
     ++
     ++		/* let it end in a directory separator */
     ++		prefix_len = len - needle_len + 1;
      +	}
      +
      +	/* Does it not start with the expected prefix? */
      +	if (fspathncmp(location, prefix, prefix_len))
      +		return location;
      +
     -+	strbuf_reset(&buf);
     -+	strbuf_addstr(&buf, location + prefix_len);
     -+	convert_slashes(buf.buf);
     ++	strlcpy(buf, location + prefix_len, sizeof(buf));
     ++	/* convert backslashes to forward slashes */
     ++	for (p = buf; *p; p++)
     ++		if (*p == '\\')
     ++			*p = '/';
      +
     -+	return buf.buf;
     ++	return buf;
      +}
      +#endif
      +
 4:  fb03f5aa6e5 = 4:  a70339f57a7 artifacts-tar: when including `.dll` files, don't forget the unit-tests
 -:  ----------- > 5:  75a74571fbe cmake: fix typo in variable name
 -:  ----------- > 6:  41228df1b46 cmake: use test names instead of full paths
 -:  ----------- > 7:  003d44e9f0d cmake: handle also unit tests

gitgitgadget

From: Johannes Schindelin <[email protected]>

A new, better way to run unit tests was just added to Git. This adds support for building those unit tests via CMake.

---
 contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

index 2f6e0197ffa..45016213358 100644
@@ -965,6 +965,24 @@ target_link_libraries(test-fake-ssh common-main)
 parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
 list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 
+#unit-tests
+add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
+
+parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
+foreach(unit_test ${unit_test_PROGRAMS})
+	add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
+	target_link_libraries("${unit_test}" unit-test-lib common-main)
+	set_target_properties("${unit_test}"
+		PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests)
+	if(MSVC)
+		set_target_properties("${unit_test}"
+			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/unit-tests)
+		set_target_properties("${unit_test}"
+			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
+	endif()
+	list(APPEND PROGRAMS_BUILT "${unit_test}")
+endforeach()
+
 #test-tool
 parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

When building the unit tests via CMake, the .pdb files are built. Those are, essentially, files containing the debug information separately from the executables.

Let's not confuse them with the executables we actually want to run.

---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 095334bfdec..38fe0ded5bd 100644
@@ -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 %.h %.c %.o unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.h %.c %.o %.pdb unit-tests/t-basic%,$(wildcard unit-tests/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

gitgitgadget

From: Johannes Schindelin <[email protected]>

Visual C interpolates __FILE__ with the absolute _Windows_ path of the source file. GCC interpolates it with the relative path, and the tests even verify that.

So let's make sure that the unit tests only emit such paths.

---
 t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

index 70030d587ff..744e223ee98 100644
@@ -21,6 +21,46 @@ static struct {
 	.result = RESULT_NONE,
 };
 
+#ifndef _MSC_VER
+#define make_relative(location) location
+#else
+/*
+ * Visual C interpolates the absolute Windows path for `__FILE__`,
+ * but we want to see relative paths, as verified by t0080.
+ */
+#include "dir.h"
+
+static const char *make_relative(const char *location)
+{
+	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
+	static size_t prefix_len;
+
+	if (!prefix_len) {
+		size_t len = strlen(prefix);
+		const char *needle = "\\t\\unit-tests\\test-lib.c";
+		size_t needle_len = strlen(needle);
+
+		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
+			die("unexpected suffix of '%s'", prefix);
+
+		/* let it end in a directory separator */
+		prefix_len = len - needle_len + 1;
+	}
+
+	/* Does it not start with the expected prefix? */
+	if (fspathncmp(location, prefix, prefix_len))
+		return location;
+
+	strlcpy(buf, location + prefix_len, sizeof(buf));
+	/* convert backslashes to forward slashes */
+	for (p = buf; *p; p++)
+		if (*p == '\\')
+			*p = '/';
+
+	return buf;
+}
+#endif
+
 static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
 {
 	fflush(stderr);

@@ -147,7 +187,8 @@ int test__run_end(int was_run UNUSED, const char *location, const char *format,
 			break;
 
 		case RESULT_NONE:
-			test_msg("BUG: test has no checks at %s", location);
+			test_msg("BUG: test has no checks at %s",
+				 make_relative(location));
 			printf("not ok %d", ctx.count);
 			print_description(format, ap);
 			ctx.result = RESULT_FAILURE;

@@ -193,13 +234,15 @@ int test_assert(const char *location, const char *check, int ok)
 	assert(ctx.running);
 
 	if (ctx.result == RESULT_SKIP) {
-		test_msg("skipping check '%s' at %s", check, location);
+		test_msg("skipping check '%s' at %s", check,
+			 make_relative(location));
 		return 0;
 	} else if (!ctx.todo) {
 		if (ok) {
 			test_pass();
 		} else {
-			test_msg("check \"%s\" failed at %s", check, location);
+			test_msg("check \"%s\" failed at %s", check,
+				 make_relative(location));
 			test_fail();
 		}
 	}

@@ -224,7 +267,8 @@ int test__todo_end(const char *location, const char *check, int res)
 	if (ctx.result == RESULT_SKIP)
 		return 0;
 	if (!res) {
-		test_msg("todo check '%s' succeeded at %s", check, location);
+		test_msg("todo check '%s' succeeded at %s", check,
+			 make_relative(location));
 		test_fail();
 	} else {
 		test_todo();

gitgitgadget

Hi Johannes

This looks good to me. Calling strlcpy() without checking the return value is a pet peeve of mine but the silent truncation doesn't matter here and the buffer ought to be long enough anyway.

Best Wishes

Phillip

From: Johannes Schindelin <[email protected]>

As of recent, Git also builds executables in t/unit-tests/. For technical reasons, when building with CMake and Visual C, the dependencies (".dll files") need to be copied there, too, otherwise running the executable will fail "due to missing dependencies".

The CMake definition already contains the directives to copy those .dll files, but we also need to adjust the artifacts-tar rule in the Makefile accordingly to let the vs-test job in the CI runs pass successfully.

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4016da6e39c..d95a7b19b50 100644
@@ -3596,7 +3596,7 @@ rpm::
 .PHONY: rpm
 
 ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \

gitgitgadget

From: Johannes Schindelin <[email protected]>

---
 contrib/buildsystems/CMakeLists.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 45016213358..ad197ea433f 100644
@@ -1102,10 +1102,10 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 endif()
 
-file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
+file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 
 #test
-foreach(tsh ${test_scipts})
+foreach(tsh ${test_scripts})
 	add_test(NAME ${tsh}
 		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)

gitgitgadget

From: Johannes Schindelin <[email protected]>

The primary purpose of Git's CMake definition is to allow developing Git in Visual Studio. As part of that, the CTest feature allows running individual test scripts conveniently in Visual Studio's Test Explorer.

However, this Test Explorer's design targets object-oriented languages and therefore expects the test names in the form <namespace>.<class>.<testname>. And since we specify the full path of the test scripts instead, including the ugly /.././t/ part, these dots confuse the Test Explorer and it uses a large part of the path as "namespace".

Let's just use t.<name> instead. This still adds an ugly "Empty Namespace" layer by default, but at least the ugly absolute path is now gone.

---
 contrib/buildsystems/CMakeLists.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index ad197ea433f..ff1a8cc348f 100644
@@ -1106,13 +1106,14 @@ file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 
 #test
 foreach(tsh ${test_scripts})
-	add_test(NAME ${tsh}
+	string(REGEX REPLACE ".*/(.*)\\.sh" "\\1" test_name ${tsh})
+	add_test(NAME "t.${test_name}"
 		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
 # This test script takes an extremely long time and is known to time out even
 # on fast machines because it requires in excess of one hour to run
-set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+set_tests_properties("t.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)
 
 endif()#BUILD_TESTING

gitgitgadget

Hi Johannes

That does sound like a worthwhile improvement. If we used git.t.<name> would that fix the "Empty Namespace" problem? (probably not worth a re-roll on its own)

Best Wishes

Phillip

Hi Phillip,

Turns out I did not play around with this enough. If I use t.suite. as a prefix, it makes the empty name space go away.

I'll do that, then.

Ciao,
Johannes

From: Johannes Schindelin <[email protected]>

The unit tests should also be available e.g. in Visual Studio's Test Explorer when configuring Git's source code via CMake.

---
 contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

index ff1a8cc348f..35d451856a0 100644
@@ -981,6 +981,17 @@ foreach(unit_test ${unit_test_PROGRAMS})
 			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
 	endif()
 	list(APPEND PROGRAMS_BUILT "${unit_test}")
+
+	# t-basic intentionally fails tests, to validate the unit-test infrastructure.
+	# Therefore, it should only be run as part of t0080, which verifies that it
+	# fails only in the expected ways.
+	#
+	# All other unit tests should be run.
+	if(NOT ${unit_test} STREQUAL "t-basic")
+		add_test(NAME "unit-tests.${unit_test}"
+			COMMAND "./${unit_test}"
+			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t/unit-tests)
+	endif()
 endforeach()
 
 #test-tool

gitgitgadget

Hi Johannes

This looks good to me, it is nice to see it being careful not to run t-basic with the other unit tests.

Best Wishes

Phillip

The recent patch series that adds proper unit testing to Git requires a couple of add-on patches to make it work with the CMake build on Windows (Visual C). This patch series aims to provide that support.

This patch series is based on js/doc-unit-tests.

Changes since v2:

* Thanks to Phillip Wood's prodding, I managed to avoid the "Empty Namespace" problem in Visual Studio's Test Explorer.

Changes since v1:

* The code added to test-lib.c now avoids using a strbuf.
* The unit tests are now also handled via CTest.
* While at it, I cleaned up a little in the CTest-related definitions.

Johannes Schindelin (7):
cmake: also build unit tests
unit-tests: do not mistake .pdb files for being executable
unit-tests: do show relative file paths
artifacts-tar: when including .dll files, don't forget the
unit-tests
cmake: fix typo in variable name
cmake: use test names instead of full paths
cmake: handle also unit tests

 Makefile                            |  2 +-
 contrib/buildsystems/CMakeLists.txt | 38 ++++++++++++++++++---
 t/Makefile                          |  2 +-
 t/unit-tests/test-lib.c             | 52 ++++++++++++++++++++++++++---
 4 files changed, 84 insertions(+), 10 deletions(-)

base-commit: 03f9bc407975bba86d1d1807519d76e1693ff66f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1579%2Fdscho%2Fdoc-unit-tests-and-cmake-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1579/dscho/doc-unit-tests-and-cmake-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1579

Range-diff vs v2:

 1:  2cc1c03d851 = 1:  2cc1c03d851 cmake: also build unit tests
 2:  90db3d5d41f = 2:  90db3d5d41f unit-tests: do not mistake `.pdb` files for being executable
 3:  f0b804129e8 = 3:  f0b804129e8 unit-tests: do show relative file paths
 4:  a70339f57a7 = 4:  a70339f57a7 artifacts-tar: when including `.dll` files, don't forget the unit-tests
 5:  75a74571fbe = 5:  75a74571fbe cmake: fix typo in variable name
 6:  41228df1b46 ! 6:  0a2d08b91e5 cmake: use test names instead of full paths

@@ Commit message
dots confuse the Test Explorer and it uses a large part of the path as "namespace".

- Let's just use t.<name> instead. This still adds an ugly "Empty
- Namespace" layer by default, but at least the ugly absolute path is now
- gone.
+ Let's just use t.suite.<name> instead. This presents the tests in
+ Visual Studio's Test Explorer in the following form by default (i.e.
+ unless the user changes the view via the "Group by" menu):
+
+ ◢ ◈ git
+ ◢ ◈ t
+ ◢ ◈ suite
+ ◈ t0000-basic
+ ◈ t0001-init
+ ◈ t0002-gitfile
+ [...]

@@ contrib/buildsystems/CMakeLists.txt: file(GLOB test_scripts "${CMAKE_SOURCE_DIR}
foreach(tsh ${test_scripts})
- add_test(NAME ${tsh}
+ string(REGEX REPLACE ".*/(.*)\\.sh" "\\1" test_name ${tsh})
-+ add_test(NAME "t.${test_name}"
++ add_test(NAME "t.suite.${test_name}"
COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
@@ contrib/buildsystems/CMakeLists.txt: file(GLOB test_scripts "${CMAKE_SOURCE_DIR}
# This test script takes an extremely long time and is known to time out even # on fast machines because it requires in excess of one hour to run
-set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
-+set_tests_properties("t.t7112-reset-submodule" PROPERTIES TIMEOUT 4000) ++set_tests_properties("t.suite.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)

endif()#BUILD_TESTING
7: 003d44e9f0d ! 7: 64707240a4e cmake: handle also unit tests

     @@ contrib/buildsystems/CMakeLists.txt: foreach(unit_test ${unit_test_PROGRAMS})
      +	#
      +	# All other unit tests should be run.
      +	if(NOT ${unit_test} STREQUAL "t-basic")
     -+		add_test(NAME "unit-tests.${unit_test}"
     ++		add_test(NAME "t.unit-tests.${unit_test}"
      +			COMMAND "./${unit_test}"
      +			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t/unit-tests)
      +	endif()

gitgitgadget

From: Johannes Schindelin <[email protected]>

A new, better way to run unit tests was just added to Git. This adds support for building those unit tests via CMake.

---
 contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

index 2f6e0197ffa..45016213358 100644
@@ -965,6 +965,24 @@ target_link_libraries(test-fake-ssh common-main)
 parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
 list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 
+#unit-tests
+add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
+
+parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
+foreach(unit_test ${unit_test_PROGRAMS})
+	add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
+	target_link_libraries("${unit_test}" unit-test-lib common-main)
+	set_target_properties("${unit_test}"
+		PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests)
+	if(MSVC)
+		set_target_properties("${unit_test}"
+			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/unit-tests)
+		set_target_properties("${unit_test}"
+			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
+	endif()
+	list(APPEND PROGRAMS_BUILT "${unit_test}")
+endforeach()
+
 #test-tool
 parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

When building the unit tests via CMake, the .pdb files are built. Those are, essentially, files containing the debug information separately from the executables.

Let's not confuse them with the executables we actually want to run.

---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 095334bfdec..38fe0ded5bd 100644
@@ -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 %.h %.c %.o unit-tests/t-basic%,$(wildcard unit-tests/t-*)))
+UNIT_TESTS = $(sort $(filter-out %.h %.c %.o %.pdb unit-tests/t-basic%,$(wildcard unit-tests/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

gitgitgadget

From: Johannes Schindelin <[email protected]>

Visual C interpolates __FILE__ with the absolute _Windows_ path of the source file. GCC interpolates it with the relative path, and the tests even verify that.

So let's make sure that the unit tests only emit such paths.

---
 t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

index 70030d587ff..744e223ee98 100644
@@ -21,6 +21,46 @@ static struct {
 	.result = RESULT_NONE,
 };
 
+#ifndef _MSC_VER
+#define make_relative(location) location
+#else
+/*
+ * Visual C interpolates the absolute Windows path for `__FILE__`,
+ * but we want to see relative paths, as verified by t0080.
+ */
+#include "dir.h"
+
+static const char *make_relative(const char *location)
+{
+	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
+	static size_t prefix_len;
+
+	if (!prefix_len) {
+		size_t len = strlen(prefix);
+		const char *needle = "\\t\\unit-tests\\test-lib.c";
+		size_t needle_len = strlen(needle);
+
+		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
+			die("unexpected suffix of '%s'", prefix);
+
+		/* let it end in a directory separator */
+		prefix_len = len - needle_len + 1;
+	}
+
+	/* Does it not start with the expected prefix? */
+	if (fspathncmp(location, prefix, prefix_len))
+		return location;
+
+	strlcpy(buf, location + prefix_len, sizeof(buf));
+	/* convert backslashes to forward slashes */
+	for (p = buf; *p; p++)
+		if (*p == '\\')
+			*p = '/';
+
+	return buf;
+}
+#endif
+
 static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
 {
 	fflush(stderr);

@@ -147,7 +187,8 @@ int test__run_end(int was_run UNUSED, const char *location, const char *format,
 			break;
 
 		case RESULT_NONE:
-			test_msg("BUG: test has no checks at %s", location);
+			test_msg("BUG: test has no checks at %s",
+				 make_relative(location));
 			printf("not ok %d", ctx.count);
 			print_description(format, ap);
 			ctx.result = RESULT_FAILURE;

@@ -193,13 +234,15 @@ int test_assert(const char *location, const char *check, int ok)
 	assert(ctx.running);
 
 	if (ctx.result == RESULT_SKIP) {
-		test_msg("skipping check '%s' at %s", check, location);
+		test_msg("skipping check '%s' at %s", check,
+			 make_relative(location));
 		return 0;
 	} else if (!ctx.todo) {
 		if (ok) {
 			test_pass();
 		} else {
-			test_msg("check \"%s\" failed at %s", check, location);
+			test_msg("check \"%s\" failed at %s", check,
+				 make_relative(location));
 			test_fail();
 		}
 	}

@@ -224,7 +267,8 @@ int test__todo_end(const char *location, const char *check, int res)
 	if (ctx.result == RESULT_SKIP)
 		return 0;
 	if (!res) {
-		test_msg("todo check '%s' succeeded at %s", check, location);
+		test_msg("todo check '%s' succeeded at %s", check,
+			 make_relative(location));
 		test_fail();
 	} else {
 		test_todo();

gitgitgadget

From: Johannes Schindelin <[email protected]>

As of recent, Git also builds executables in t/unit-tests/. For technical reasons, when building with CMake and Visual C, the dependencies (".dll files") need to be copied there, too, otherwise running the executable will fail "due to missing dependencies".

The CMake definition already contains the directives to copy those .dll files, but we also need to adjust the artifacts-tar rule in the Makefile accordingly to let the vs-test job in the CI runs pass successfully.

---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4016da6e39c..d95a7b19b50 100644
@@ -3596,7 +3596,7 @@ rpm::
 .PHONY: rpm
 
 ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
-OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \

gitgitgadget

From: Johannes Schindelin <[email protected]>

---
 contrib/buildsystems/CMakeLists.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 45016213358..ad197ea433f 100644
@@ -1102,10 +1102,10 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
 	file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
 endif()
 
-file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
+file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 
 #test
-foreach(tsh ${test_scipts})
+foreach(tsh ${test_scripts})
 	add_test(NAME ${tsh}
 		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)

gitgitgadget

From: Johannes Schindelin <[email protected]>

The primary purpose of Git's CMake definition is to allow developing Git in Visual Studio. As part of that, the CTest feature allows running individual test scripts conveniently in Visual Studio's Test Explorer.

However, this Test Explorer's design targets object-oriented languages and therefore expects the test names in the form <namespace>.<class>.<testname>. And since we specify the full path of the test scripts instead, including the ugly /.././t/ part, these dots confuse the Test Explorer and it uses a large part of the path as "namespace".

Let's just use t.suite.<name> instead. This presents the tests in Visual Studio's Test Explorer in the following form by default (i.e. unless the user changes the view via the "Group by" menu):

	◢ ◈ git
	 ◢ ◈ t
	  ◢ ◈ suite
	     ◈ t0000-basic
	     ◈ t0001-init
	     ◈ t0002-gitfile
	     [...]

---
 contrib/buildsystems/CMakeLists.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index ad197ea433f..5e0c237dfd4 100644
@@ -1106,13 +1106,14 @@ file(GLOB test_scripts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
 
 #test
 foreach(tsh ${test_scripts})
-	add_test(NAME ${tsh}
+	string(REGEX REPLACE ".*/(.*)\\.sh" "\\1" test_name ${tsh})
+	add_test(NAME "t.suite.${test_name}"
 		COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
 		WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
 endforeach()
 
 # This test script takes an extremely long time and is known to time out even
 # on fast machines because it requires in excess of one hour to run
-set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+set_tests_properties("t.suite.t7112-reset-submodule" PROPERTIES TIMEOUT 4000)
 
 endif()#BUILD_TESTING

gitgitgadget

From: Johannes Schindelin <[email protected]>

The unit tests should also be available e.g. in Visual Studio's Test Explorer when configuring Git's source code via CMake.

---
 contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

index 5e0c237dfd4..d21835ca65c 100644
@@ -981,6 +981,17 @@ foreach(unit_test ${unit_test_PROGRAMS})
 			PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
 	endif()
 	list(APPEND PROGRAMS_BUILT "${unit_test}")
+
+	# t-basic intentionally fails tests, to validate the unit-test infrastructure.
+	# Therefore, it should only be run as part of t0080, which verifies that it
+	# fails only in the expected ways.
+	#
+	# All other unit tests should be run.
+	if(NOT ${unit_test} STREQUAL "t-basic")
+		add_test(NAME "t.unit-tests.${unit_test}"
+			COMMAND "./${unit_test}"
+			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t/unit-tests)
+	endif()
 endforeach()
 
 #test-tool

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Thanks. Will replace.

I somehow liked this part of the log message very much ;-)