[PATCH 0/4] CMake(Visual C) support for js/doc-unit-tests

[PATCH 0/4] CMake(Visual C) support for js/doc-unit-tests

[Johannes Schindelin via 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.

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

[PATCH 1/4] cmake: also build unit tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ffa..45016213358 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

Re: [PATCH 1/4] cmake: also build unit tests

[Phillip Wood]

Hi Johannes

On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> A new, better way to run unit tests was just added to Git. This adds
> support for building those unit tests via CMake.

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

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 2f6e0197ffa..45016213358 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -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")
>   

Re: [PATCH 1/4] cmake: also build unit tests

[Johannes Schindelin]

Hi Phillip,

On Mon, 11 Sep 2023, Phillip Wood wrote:

> On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > A new, better way to run unit tests was just added to Git. This adds
> > support for building those unit tests via CMake.
>
> 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?

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

[PATCH 2/4] unit-tests: do not mistake `.pdb` files for being executable

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 095334bfdec..38fe0ded5bd 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 %.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

[PATCH 3/4] unit-tests: do show relative file paths

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/unit-tests/test-lib.c | 48 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 70030d587ff..bd94079af61 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -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

Re: [PATCH 3/4] unit-tests: do show relative file paths

[Phillip Wood]

Hi Johannes

On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

Oh, that's a pain

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

Makes sense

> +#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;

So far test-lib.c avoids using things like struct strbuf that it will be 
used to test. In this instance we're only using it on one particular 
compiler so it may not matter so much. We could avoid it but I'm not 
sure it is worth the extra complexity. One thing I noted in this patch 
is that prefix is leaked but I'm not sure if you run any leak checkers 
on the msvc build.

static const char *make_relative(const char *location)
{
	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;
}

Best Wishes

Phillip

Re: [PATCH 3/4] unit-tests: do show relative file paths

[Johannes Schindelin]

Hi Phillip,

On Mon, 11 Sep 2023, Phillip Wood wrote:

> On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> Oh, that's a pain
>
> > So let's make sure that the unit tests only emit such paths.
>
> Makes sense
>
> > +#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;
>
> So far test-lib.c avoids using things like struct strbuf that it will be used
> to test. In this instance we're only using it on one particular compiler so it
> may not matter so much. We could avoid it but I'm not sure it is worth the
> extra complexity. One thing I noted in this patch is that prefix is leaked but
> I'm not sure if you run any leak checkers on the msvc build.

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

[PATCH 4/4] artifacts-tar: when including `.dll` files, don't forget the unit-tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4016da6e39c..d95a7b19b50 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v2 0/7] CMake(Visual C) support for js/doc-unit-tests

[Johannes Schindelin via 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

[PATCH v2 1/7] cmake: also build unit tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ffa..45016213358 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

[PATCH v2 2/7] unit-tests: do not mistake `.pdb` files for being executable

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 095334bfdec..38fe0ded5bd 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 %.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

[PATCH v2 3/7] unit-tests: do show relative file paths

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 70030d587ff..744e223ee98 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -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

Re: [PATCH v2 3/7] unit-tests: do show relative file paths

[Phillip Wood]

Hi Johannes

On 18/09/2023 21:54, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

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

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 70030d587ff..744e223ee98 100644
> --- a/t/unit-tests/test-lib.c
> +++ b/t/unit-tests/test-lib.c
> @@ -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();

[PATCH v2 4/7] artifacts-tar: when including `.dll` files, don't forget the unit-tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4016da6e39c..d95a7b19b50 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v2 5/7] cmake: fix typo in variable name

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 45016213358..ad197ea433f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

[PATCH v2 6/7] cmake: use test names instead of full paths

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index ad197ea433f..ff1a8cc348f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

Re: [PATCH v2 6/7] cmake: use test names instead of full paths

[Phillip Wood]

Hi Johannes

On 18/09/2023 21:54, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

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

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   contrib/buildsystems/CMakeLists.txt | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index ad197ea433f..ff1a8cc348f 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -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

Re: [PATCH v2 6/7] cmake: use test names instead of full paths

[Johannes Schindelin]

Hi Phillip,

On Fri, 22 Sep 2023, Phillip Wood wrote:

> On 18/09/2023 21:54, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> 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)

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

[PATCH v2 7/7] cmake: handle also unit tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index ff1a8cc348f..35d451856a0 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

Re: [PATCH v2 7/7] cmake: handle also unit tests

[Phillip Wood]

Hi Johannes

On 18/09/2023 21:54, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The unit tests should also be available e.g. in Visual Studio's Test
> Explorer when configuring Git's source code via CMake.
> 
> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

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

> ---
>   contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index ff1a8cc348f..35d451856a0 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -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

[PATCH v3 0/7] CMake(Visual C) support for js/doc-unit-tests

[Johannes Schindelin via 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 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
     +                 [...]
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ 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

[PATCH v3 1/7] cmake: also build unit tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ffa..45016213358 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

[PATCH v3 2/7] unit-tests: do not mistake `.pdb` files for being executable

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 095334bfdec..38fe0ded5bd 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 %.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

[PATCH v3 3/7] unit-tests: do show relative file paths

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/unit-tests/test-lib.c | 52 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 70030d587ff..744e223ee98 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -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

[PATCH v3 4/7] artifacts-tar: when including `.dll` files, don't forget the unit-tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4016da6e39c..d95a7b19b50 100644
--- a/Makefile
+++ b/Makefile
@@ -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

[PATCH v3 5/7] cmake: fix typo in variable name

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 45016213358..ad197ea433f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

[PATCH v3 6/7] cmake: use test names instead of full paths

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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
	     [...]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index ad197ea433f..5e0c237dfd4 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

[PATCH v3 7/7] cmake: handle also unit tests

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/buildsystems/CMakeLists.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 5e0c237dfd4..d21835ca65c 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -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

Re: [PATCH v3 0/7] CMake(Visual C) support for js/doc-unit-tests

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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.

Thanks.  Will replace.

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

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