Optimize adding test commands in catch_discover_tests

The previous approach was for `add_command` to behave as append function
via concatenating the command string internally and then saving it into
`PARENT_SCOPE`. Because this in practice ended up meaning concatenating
a copy of the string inside the function and then overwriting the string
outside the function, the performance was lacking.

The new approach is for `prepare_command` to only escape & return the
command from single call, and the caller is responsible for concatenating
the result. Since the actual concatenation no longer crosses function
scope boundaries, the performance is much better, even though the scaling
is still quadratic.

The new approach only takes 1/4-1/5 of the time, saving ~1s at 1k tests,
4.5s at 2k tests and 19s at 4k tests.

3 files changed, 97 insertions, 8 deletions

diff --git a/extras/CatchAddTests.cmake b/extras/CatchAddTests.cmake
index 5c67c874..8005f31f 100644
--- a/extras/CatchAddTests.cmake
+++ b/extras/CatchAddTests.cmake
@@ -1,7 +1,21 @@
 # Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
 # file Copyright.txt or https://cmake.org/licensing for details.
 
-function(add_command NAME)
+# TBD: Further possible optimization is that most arguments for per-test
+#      `prepare_command` call are constant across one invocation of
+#      `catch_discover_tests`, and thus need checking and escaping only
+#      once, instead of for each test.
+#      This would provide nice speed-up of the actual command preparation,
+#      but it will make the script much harder to read, and it is utterly
+#      dwarfed by the quadratic scaling of parsing JSON arrays in CMake.
+
+
+# Prepare command with escaped (bracketed) arguments and return it via `_Command` out variable.
+#
+# To avoid quadratic performance when concatenating all commands together,
+# the actual concatenation must be done by the caller, by appending it
+# into a string of all other commands.
+function(prepare_command NAME)
   set(_args "")
   # use ARGV* instead of ARGN, because ARGN splits arrays into multiple arguments
   math(EXPR _last_arg ${ARGC}-1)
@@ -13,7 +27,7 @@ function(add_command NAME)
       set(_args "${_args} ${_arg}")
     endif()
   endforeach()
-  set(script "${script}${NAME}(${_args})\n" PARENT_SCOPE)
+  set(_Command "${NAME}(${_args})\n" PARENT_SCOPE)
 endfunction()
 
 # Generates random filename in the temp folder.
@@ -220,7 +234,7 @@ function(catch_discover_tests_impl)
     endif()
 
     # ...and add to script
-    add_command(add_test
+    prepare_command(add_test
       "${prefix}${plain_name}${suffix}"
       ${_TEST_EXECUTOR}
       "${_TEST_EXECUTABLE}"
@@ -229,12 +243,14 @@ function(catch_discover_tests_impl)
       "${reporter_arg}"
       "${output_dir_arg}"
     )
-    add_command(set_tests_properties
+    string(APPEND script "${_Command}")
+    prepare_command(set_tests_properties
       "${prefix}${plain_name}${suffix}"
       PROPERTIES
       WORKING_DIRECTORY "${_TEST_WORKING_DIR}"
       ${properties}
     )
+    string(APPEND script "${_Command}")
 
     if(add_tags)
       string(JSON num_tags LENGTH "${test_tags}")
@@ -254,19 +270,21 @@ function(catch_discover_tests_impl)
           list(APPEND tag_list "${a_tag}")
         endforeach()
 
-        add_command(set_tests_properties
+        prepare_command(set_tests_properties
           "${prefix}${plain_name}${suffix}"
           PROPERTIES
           LABELS "${tag_list}"
         )
+        string(APPEND script "${_Command}")
       endif()
     endif(add_tags)
 
     if(environment_modifications)
-      add_command(set_tests_properties
+      prepare_command(set_tests_properties
         "${prefix}${plain_name}${suffix}"
         PROPERTIES
         ENVIRONMENT_MODIFICATION "${environment_modifications}")
+      string(APPEND script "${_Command}")
     endif()
 
     list(APPEND tests "${prefix}${plain_name}${suffix}")
@@ -274,13 +292,16 @@ function(catch_discover_tests_impl)
 
   # Create a list of all discovered tests, which users may use to e.g. set
   # properties on the tests
-  add_command(set ${_TEST_LIST} ${tests})
+  prepare_command(set ${_TEST_LIST} ${tests})
+  string(APPEND script "${_Command}")
 
   # Write CTest script
   file(WRITE "${_CTEST_FILE}" "${script}")
 endfunction()
 
-if(CMAKE_SCRIPT_MODE_FILE)
+# To enable `include`ing this file in the unit test scripts, we only run
+# the impl if an actual `TEST_EXECUTABLE` is provided.
+if(CMAKE_SCRIPT_MODE_FILE AND DEFINED TEST_EXECUTABLE)
   catch_discover_tests_impl(
     TEST_EXECUTABLE ${TEST_EXECUTABLE}
     TEST_EXECUTOR ${TEST_EXECUTOR}
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 7429734f..e2aad5ef 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -625,6 +625,13 @@ if(CATCH_ENABLE_CMAKE_HELPER_TESTS)
       COST 240
       LABELS "uses-python"
   )
+
+  add_test(NAME "CMakeHelper::PrepareCommand"
+    COMMAND
+      "${CMAKE_COMMAND}"
+      "-DCATCH_ADD_TESTS_SCRIPT=${CATCH_DIR}/extras/CatchAddTests.cmake"
+      -P "${CMAKE_CURRENT_LIST_DIR}/TestScripts/DiscoverTests/TestPrepareCommand.cmake"
+  )
 endif()
 
 foreach(reporterName # "Automake" - the simple .trs format does not support any kind of comments/metadata
diff --git a/tests/TestScripts/DiscoverTests/TestPrepareCommand.cmake b/tests/TestScripts/DiscoverTests/TestPrepareCommand.cmake
new file mode 100644
index 00000000..f02bb409
--- /dev/null
+++ b/tests/TestScripts/DiscoverTests/TestPrepareCommand.cmake
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: BSL-1.0
+
+# Unit tests for `prepare_command` helper in `extras/CatchAddTests.cmake`.
+#
+# Yes, we are at the stage where the script helpers need unit tests.
+#
+# Run as
+#     cmake -DCATCH_ADD_TESTS_SCRIPT=/path/to/extras/CatchAddTests.cmake \
+#           -P TestPrepareCommand.cmake
+
+
+cmake_minimum_required(VERSION 3.19)
+
+if(NOT DEFINED CATCH_ADD_TESTS_SCRIPT)
+  message(FATAL_ERROR "Missing argument `CATCH_ADD_TESTS_SCRIPT`")
+endif()
+
+if(NOT EXISTS "${CATCH_ADD_TESTS_SCRIPT}")
+  message(FATAL_ERROR "Cannot find CatchAddTests.cmake at '${CATCH_ADD_TESTS_SCRIPT}'")
+endif()
+
+# Pull in the helper functions. Without `TEST_EXECUTABLE` being defined,
+# `catch_discover_tests_impl` is not called.
+include("${CATCH_ADD_TESTS_SCRIPT}")
+
+set(_failures 0)
+
+function(expect_equal description actual expected)
+  if(actual STREQUAL expected)
+    message("  [PASS] ${description}")
+  else()
+    message("  [FAIL] ${description}")
+    message("         expected: [${expected}]")
+    message("         actual:   [${actual}]")
+    math(EXPR _n "${_failures} + 1")
+    set(_failures "${_n}" PARENT_SCOPE)
+  endif()
+endfunction()
+
+prepare_command(add_test SimpleName /path/to/tests)
+expect_equal("Simple arg, no quotes" "${_Command}" "add_test( SimpleName /path/to/tests)\n")
+
+prepare_command(add_test "Name with spaces")
+expect_equal("Spaces in arg, needs quotes" "${_Command}" "add_test( [==[Name with spaces]==])\n")
+
+prepare_command(set_tests_properties Foo PROPERTIES LABELS "tagA\;tagB\;tagC")
+expect_equal("semicolons in argument are kept and quoted"
+  "${_Command}" "set_tests_properties( Foo PROPERTIES LABELS [==[tagA\;tagB\;tagC]==])\n")
+
+set(_Command "PRE-EXISTING")
+prepare_command(set_tests_properties Foo PROPERTIES BAR baz)
+expect_equal("_Command var does no accumulate commands"
+  "${_Command}" "set_tests_properties( Foo PROPERTIES BAR baz)\n")
+
+
+
+if(_failures GREATER 0)
+  message(FATAL_ERROR "${_failures} prepare_command test(s) failed")
+else()
+  message(STATUS "All prepare_command tests passed")
+endif()