Skip to content

Some fixes and an improvement for using CTest on Windows

Johannes Schindelin via GitGitGadget git 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest5 patches 5 repliesIn review
  2. v25 patches 14 repliesSuperseded
  3. v15 patches 21 repliesSuperseded

Changes in v3 from the cover letter

* Enhanced a commit message to clarify how the "cumbersome approach" looks like when a developer wishes to pass options to the test scripts in Visual Studio.
* Changed the base branch to ac/fuzzers, to avoid merge conflicts in .gitignore.
* Moved the PANIC check in test-lib.sh so that it is not skipped when the file GIT-BUILD-DIR exists.

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/5cmake: make it easier to diagnose regressions in CTest runscontrib/buildsystems/CMakeLists.txt+1 -11 reply
2/5cmake: copy the merge tools for testingcontrib/buildsystems/CMakeLists.txt+2 -11 reply
3/5add -p: avoid ambiguous signed/unsigned comparisonadd-patch.c+1 -11 reply
4/5cmake: avoid editing t/test-lib.sh.gitignore Makefile contrib/buildsystems/CMakeLists.txt +1 more+13 -62 replies
5/5cmake: increase time-out for a long-running testcontrib/buildsystems/CMakeLists.txt+4 -00 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v3 yet.

Read the v3 thread

Cover letter [PATCH v3 0/5] Some fixes and an improvement for using CTest on Windows

Visual Studio users enjoy support for running the test suite via CTest, thanks to Git's CMake definition.

In https://github.com/git-for-windows/git/issues/3966, it has been reported that this does not work out of the box, though, but causes a couple of test failures instead. These problems are not caught by Git's CI runs because the vs-tests jobs actually use prove to run the test suite, not CTest.

In addition to fixing these problems, this patch series also addresses a long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified (but these modification should never be committed, they refer to a local-only, configuration-dependent directory).

Note: The signed/unsigned comparison bug in git add -p that is fixed in this here patch series is a relatively big one, and it merits further investigation whether there are similar bugs lurking in Git's code base. However, this is a much bigger project than can be addressed as part of this patch series, in particular because the analysis would require tools other than GCC's -Wsign-compare option (which totally misses the instance that is fixed in this here patch series).

Changes since v2:

* Enhanced a commit message to clarify how the "cumbersome approach" looks like when a developer wishes to pass options to the test scripts in Visual Studio.
* Changed the base branch to ac/fuzzers, to avoid merge conflicts in .gitignore.
* Moved the PANIC check in test-lib.sh so that it is not skipped when the file GIT-BUILD-DIR exists.

Changes since v1:

* Clarified why it is a good idea to pass --no-bin-wrappers and --no-chain-lint when running on Windows.
* Clarified why the add -p bug has not been caught earlier.
* Clarified the scope of this patch series to fix running Git's tests within Visual Studio.
* Increased the time-out for the very slow t7112 test script.
* The test_chmod was determined to be not only faulty, but unneeded, and was dropped.

Johannes Schindelin (5):
cmake: make it easier to diagnose regressions in CTest runs
cmake: copy the merge tools for testing
add -p: avoid ambiguous signed/unsigned comparison
cmake: avoid editing t/test-lib.sh
cmake: increase time-out for a long-running test

 .gitignore                          |  1 +
 Makefile                            |  1 +
 add-patch.c                         |  2 +-
 contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------
 t/test-lib.sh                       | 10 ++++++++++
 5 files changed, 21 insertions(+), 9 deletions(-)

base-commit: 6713bfc70c4dc6da1fa4084f000b72f5d74fecfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1320

Range-diff vs v2:

1: e00cb37b98a ! 1: 356b2e9a100 cmake: make it easier to diagnose regressions in CTest runs
@@ Commit message

Like in Git's CI runs, when running the tests in Visual Studio via the CTest route, it is cumbersome or at least requires a very unintuitive
- approach to pass options to the test scripts.
+ approach to pass options to the test scripts: the CMakeLists.txt file
+ would have to be modified, passing the desired options to _all_ test
+ scripts, and then the CMake Cache would have to be reconfigured before
+ running the test in question individually. Unintuitive at best, and
+ opposite to the niceties IDE users expect.

So let's just pass those options by default: This will not clutter any output window but the log that is written to a log file will have
@@ Commit message

While at it, also imitate what the Windows jobs in Git's CI runs do to accelerate running the test scripts: pass the --no-bin-wrappers and
- --no-chain-lint options. This makes the test runs noticeably faster
- because the bin-wrappers/ scripts as well as the chain-lint code
- make heavy use of POSIX shell scripting, which is really, really slow on
- Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
+ --no-chain-lint options.
+
+ This makes the test runs noticeably faster because the bin-wrappers/
+ scripts as well as the chain-lint code make heavy use of POSIX shell
+ scripting, which is really, really slow on Windows due to the need to
+ emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
+ Sunshine, it added two minutes (!) just to perform the chain-lint task.
+
+ The idea of adding a CMake config option (á la GIT_TEST_OPTS) was
+ considered during the development of this patch, but then dropped: such
+ a setting is global, across _all_ tests, where e.g. --run=... would
+ not make sense. Users wishing to override these new defaults are better
+ advised running the test script manually, in a Git Bash, with full
+ control over the command line.

 2:  de7b47a9aa7 = 2:  9faca9d5cbe cmake: copy the merge tools for testing
 3:  f96d5ab484c = 3:  41a8021a4bd add -p: avoid ambiguous signed/unsigned comparison
 4:  22473d6b8f3 ! 4:  5b0c2a150e9 cmake: avoid editing t/test-lib.sh

@@ Commit message
## .gitignore ##
@@
/fuzz_corpora
- /fuzz-pack-headers
- /fuzz-pack-idx
+/GIT-BUILD-DIR
/GIT-BUILD-OPTIONS
/GIT-CFLAGS
@@ contrib/buildsystems/CMakeLists.txt: endif()

## t/test-lib.sh ##
@@ t/test-lib.sh: then
- TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
+ echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
+ exit 1
fi
- GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
--if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+ GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
@@ t/test-lib.sh: then
+ GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+ ;;
+ esac
-+elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
- then
- echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
- exit 1
++fi
+
+ # Prepend a string to a VAR using an arbitrary ":" delimiter, not
+ # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
5: 6aaa675301c = 5: 40cf872f483 cmake: increase time-out for a long-running test

Signature · 1 line
gitgitgadget