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

Message

Philippe Blain via GitGitGadget Oct. 18, 2022, 10:59 a.m. UTC

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.
      
          Signed-off-by: Johannes Schindelin <[e-mail]>
      
 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