Simplify CMake Tests

Remove dependency on CTest. Parallel execution is handled by `test_driver.py`
internally now, so CTest is no longer relevant for performance.

This also removes CMake targets for single tests. As a replacement,
`test_driver.py` can be called directly with the path to the build directory as
the first argument and the path to the desired test as the second argument.
Ensuring that the executables in the build directory are up to date needs to be
done separately.
For a pure cargo build, an example of running a single test would be:
`cargo b && tests/test_driver.py target/debug tests/checks/abbr.fish`

The recommended way of running tests is `build_tools/check.sh`, which runs more
extensive tests and does not depend on CMake. That script does not work in CI
yet, so CMake testing is retained for now.

Update CI config to use the new `FISH_TEST_MAX_CONCURRENCY`.
Also update the FreeBSD version, since the previous one is outdated and does not
support the semaphore logic in `test_driver.py`.

.cirrus.yml

@@ -26,7 +26,7 @@ linux_task:
         - lscpu || true
         - (cat /proc/meminfo | grep MemTotal) || true
         - mkdir build && cd build
-        - cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCTEST_PARALLEL_LEVEL=6 ..
+        - FISH_TEST_MAX_CONCURRENCY=6 cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug ..
         - ninja -j 6 fish
         - ninja fish_run_tests
     only_if: $CIRRUS_REPO_OWNER == 'fish-shell'
@@ -45,7 +45,7 @@ linux_arm_task:
         - lscpu || true
         - (cat /proc/meminfo | grep MemTotal) || true
         - mkdir build && cd build
-        - cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCTEST_PARALLEL_LEVEL=6 ..
+        - FISH_TEST_MAX_CONCURRENCY=6 cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug ..
         - ninja -j 6 fish
         - file ./fish
         - ninja fish_run_tests
@@ -54,17 +54,11 @@ linux_arm_task:
 
 freebsd_task:
     matrix:
-        # - name: FreeBSD 14
-        #   freebsd_instance:
-        #       image_family: freebsd-14-0-snap
-        - name: FreeBSD 13
+        - name: FreeBSD 14
           freebsd_instance:
-              image: freebsd-13-2-release-amd64
-        # - name: FreeBSD 12.3
-        #   freebsd_instance:
-        #       image: freebsd-12-3-release-amd64
+              image: freebsd-14-3-release-amd64-ufs
     tests_script:
-        - pkg install -y cmake-core devel/pcre2 devel/ninja misc/py-pexpect git-lite terminfo-db
+        - pkg install -y cmake-core devel/pcre2 devel/ninja lang/rust misc/py-pexpect git-lite
         # libclang.so is a required build dependency for rust-c++ ffi bridge
         - pkg install -y llvm
         # BSDs have the following behavior: root may open or access files even if
@@ -77,15 +71,7 @@ freebsd_task:
         - mkdir build && cd build
         - chown -R fish-user ..
         - sudo -u fish-user -s whoami
-        # FreeBSD's pkg currently has rust 1.66.0 while we need rust 1.70.0+. Use rustup to install
-        # the latest, but note that it only installs rust per-user.
-        - sudo -u fish-user -s fetch -qo - https://sh.rustup.rs > rustup.sh
-        - sudo -u fish-user -s sh ./rustup.sh -y --profile=minimal
-        # `sudo -s ...` does not invoke a login shell so we need a workaround to make sure the
-        # rustup environment is configured for subsequent `sudo -s ...` commands.
-        # For some reason, this doesn't do the job:
-        # - sudo -u fish-user sh -c 'echo source \$HOME/.cargo/env >> $HOME/.cshrc'
-        - sudo -u fish-user -s cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCTEST_PARALLEL_LEVEL=1 ..
-        - sudo -u fish-user sh -c '. $HOME/.cargo/env; ninja -j 6 fish'
-        - sudo -u fish-user sh -c '. $HOME/.cargo/env; ninja fish_run_tests'
+        - sudo -u fish-user -s FISH_TEST_MAX_CONCURRENCY=1 cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug ..
+        - sudo -u fish-user -s ninja -j 6 fish
+        - sudo -u fish-user -s ninja fish_run_tests
     only_if: $CIRRUS_REPO_OWNER == 'fish-shell'

.github/workflows/main.yml

@@ -3,7 +3,7 @@ name: make fish_run_tests
 on: [push, pull_request]
 
 env:
-  CTEST_PARALLEL_LEVEL: "4"
+  FISH_TEST_MAX_CONCURRENCY: "4"
   CMAKE_BUILD_PARALLEL_LEVEL: "4"
 
 permissions:

cmake/Tests.cmake

@@ -1,92 +1,33 @@
-# This adds ctest support to the project
-enable_testing()
-
-# By default, ctest runs tests serially
-# Support CTEST_PARALLEL_LEVEL as an environment variable in addition to a CMake variable
-if(NOT CTEST_PARALLEL_LEVEL)
-  set(CTEST_PARALLEL_LEVEL $ENV{CTEST_PARALLEL_LEVEL})
-  if(NOT CTEST_PARALLEL_LEVEL)
-    include(ProcessorCount)
-    ProcessorCount(CORES)
-    math(EXPR halfcores "${CORES} / 2")
-    set(CTEST_PARALLEL_LEVEL ${halfcores})
-  endif()
-endif()
-
-
-# Put in a tests folder to reduce the top level targets in IDEs.
-set(CMAKE_FOLDER tests)
-
-# We will use 125 as a reserved exit code to indicate that a test has been skipped, i.e. it did not
-# pass but it should not be considered a failed test run, either.
-set(SKIP_RETURN_CODE 125)
-
-# Even though we are using CMake's ctest for testing, we still define our own `make fish_run_tests` target
-# rather than use its default for many reasons:
-#  * CMake doesn't run tests in-proc or even add each tests as an individual node in the ninja
-#    dependency tree, instead it just bundles all tests into a target called `test` that always just
-#    shells out to `ctest`, so there are no build-related benefits to not doing that ourselves.
-#  * CMake devs insist that it is appropriate for `make fish_run_tests` to never depend on `make all`, i.e.
-#    running `make fish_run_tests` does not require any of the binaries to be built before testing.
-#  * It is not possible to set top-level CTest options/settings such as CTEST_PARALLEL_LEVEL from
-#    within the CMake configuration file.
-#  * The only way to have a test depend on a binary is to add a fake test with a name like
-#    "build_fish" that executes CMake recursively to build the `fish` target.
-#  * Circling back to the point about individual tests not being actual Makefile targets, CMake does
-#    not offer any way to execute a named test via the `make`/`ninja`/whatever interface; the only
-#    way to manually invoke test `foo` is to to manually run `ctest` and specify a regex matching
-#    `foo` as an argument, e.g. `ctest -R ^foo$`... which is really crazy.
-
-# The top-level test target is "fish_run_tests".
-add_custom_target(fish_run_tests
-  COMMAND env CTEST_PARALLEL_LEVEL=${CTEST_PARALLEL_LEVEL} FISH_FORCE_COLOR=1
-          ${CMAKE_CTEST_COMMAND} --force-new-ctest-process # --verbose
-          --output-on-failure --progress
-  DEPENDS fish fish_indent fish_key_reader fish_test_helper
-  USES_TERMINAL
-)
-
-# CMake being CMake, you can't just add a DEPENDS argument to add_test to make it depend on any of
-# your binaries actually being built before `make fish_run_tests` is executed (requiring `make all` first),
-# and the only dependency a test can have is on another test. So we make building fish
-# prerequisites to our entire top-level `test` target.
-function(add_test_target NAME)
-  string(REPLACE "/" "-" NAME ${NAME})
-  add_custom_target("test_${NAME}" COMMAND ${CMAKE_CTEST_COMMAND} --output-on-failure -R "^${NAME}$$"
-    DEPENDS fish fish_indent fish_key_reader fish_test_helper USES_TERMINAL)
-endfunction()
-
 add_executable(fish_test_helper tests/fish_test_helper.c)
 
 FILE(GLOB FISH_CHECKS CONFIGURE_DEPENDS ${CMAKE_SOURCE_DIR}/tests/checks/*.fish)
 foreach(CHECK ${FISH_CHECKS})
   get_filename_component(CHECK_NAME ${CHECK} NAME)
-  get_filename_component(CHECK ${CHECK} NAME_WE)
-  add_test(NAME ${CHECK_NAME}
+  add_custom_target(
+    test_${CHECK_NAME}
     COMMAND ${CMAKE_SOURCE_DIR}/tests/test_driver.py ${CMAKE_CURRENT_BINARY_DIR}
-                checks/${CHECK}.fish
+                checks/${CHECK_NAME}
     WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests
+    DEPENDS fish fish_indent fish_key_reader fish_test_helper
+    USES_TERMINAL
   )
-  set_tests_properties(${CHECK_NAME} PROPERTIES SKIP_RETURN_CODE ${SKIP_RETURN_CODE})
-  set_tests_properties(${CHECK_NAME} PROPERTIES ENVIRONMENT FISH_FORCE_COLOR=1)
-  add_test_target("${CHECK_NAME}")
 endforeach(CHECK)
 
 FILE(GLOB PEXPECTS CONFIGURE_DEPENDS ${CMAKE_SOURCE_DIR}/tests/pexpects/*.py)
 foreach(PEXPECT ${PEXPECTS})
   get_filename_component(PEXPECT ${PEXPECT} NAME)
-  add_test(NAME ${PEXPECT}
+  add_custom_target(
+    test_${PEXPECT}
     COMMAND ${CMAKE_SOURCE_DIR}/tests/test_driver.py ${CMAKE_CURRENT_BINARY_DIR}
                 pexpects/${PEXPECT}
     WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests
+    DEPENDS fish fish_indent fish_key_reader fish_test_helper
+    USES_TERMINAL
   )
-  set_tests_properties(${PEXPECT} PROPERTIES SKIP_RETURN_CODE ${SKIP_RETURN_CODE})
-  set_tests_properties(${PEXPECT} PROPERTIES ENVIRONMENT FISH_FORCE_COLOR=1)
-  add_test_target("${PEXPECT}")
 endforeach(PEXPECT)
 
-set(cargo_test_flags)
 # Rust stuff.
+set(cargo_test_flags)
 if(DEFINED ASAN)
     # Rust w/ -Zsanitizer=address requires explicitly specifying the --target triple or else linker
     # errors pertaining to asan symbols will ensue.
@@ -107,10 +48,17 @@ if(DEFINED Rust_CARGO_TARGET)
     list(APPEND cargo_test_flags "--lib")
 endif()
 
-add_test(
-    NAME "cargo-test"
-    COMMAND env ${VARS_FOR_CARGO} cargo test --no-default-features ${CARGO_FLAGS} --workspace --target-dir ${rust_target_dir} ${cargo_test_flags}
-    WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}"
+set(max_concurrency_flag)
+if(DEFINED ENV{FISH_TEST_MAX_CONCURRENCY})
+    list(APPEND max_concurrency_flag "--max-concurrency" $ENV{FISH_TEST_MAX_CONCURRENCY})
+endif()
+
+# The top-level test target is "fish_run_tests".
+add_custom_target(fish_run_tests
+  # TODO: This should be replaced with a unified solution, possibly build_tools/check.sh.
+  COMMAND ${CMAKE_SOURCE_DIR}/tests/test_driver.py ${max_concurrency_flag} ${CMAKE_CURRENT_BINARY_DIR}
+  COMMAND env ${VARS_FOR_CARGO} cargo test --no-default-features ${CARGO_FLAGS} --workspace --target-dir ${rust_target_dir} ${cargo_test_flags}
+  WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}"
+  DEPENDS fish fish_indent fish_key_reader fish_test_helper
+  USES_TERMINAL
 )
-set_tests_properties("cargo-test" PROPERTIES SKIP_RETURN_CODE ${SKIP_RETURN_CODE})
-add_test_target("cargo-test")