catch_discover_tests: Escape test-invariant parts of CTest script only once

Previously, the `catch_discover_tests` would prepare the entire CTest
command (e.g. `add_test(...)` or `set_tests_properties(...)`) first, and
then escape it when finished. However, this caused lot of the command args
to be escaped over and over again (e.g. executable name or Catch2's reporter
args), for no reason, as they were always the same, and thus their escaping
was always the same.

Until recently, the performance overhead didn't matter as there were many
spots which had quadratic runtime in number of tests. However, the recent
refactorings fixed these, and this commit now improves the throughput by
10-20%.

Also extended the benchmarked COUNTS in `benchmark_discovery.py`,
because the performance is now good enough that it is reasonable
to benchmark 16k tests.

benchmarks/discover_tests/benchmark_discovery.py

@@ -21,7 +21,7 @@ import time
 HERE = os.path.dirname(os.path.abspath(__file__))
 TEMPLATE = os.path.join(HERE, "listing_template.json")
 SHIM = os.path.join(HERE, "copy_shim.cmake")
-COUNTS = [1, 10, 500, 1000, 2000, 4000, 8000]
+COUNTS = [1, 10, 500, 1000, 2000, 4000, 8000, 16000]
 #COUNTS = [1, 10, 100]
 REPEATS = 5
 

extras/CatchAddTests.cmake

@@ -140,35 +140,28 @@ function(split_json_array json_array_var out_var)
   set(${out_var} "${array_elements}" PARENT_SCOPE)
 endfunction()
 
-# 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.
+# Prepare (a part of) command with bracketed arguments and return it via `out_var`.
 #
-# 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)
+# This allows the registration script to escape parts of the test script
+# only once, instead of escaping the unchanged arguments over and over again.
+function(prepare_command_fragment out_var)
   set(_args "")
-  # use ARGV* instead of ARGN, because ARGN splits arrays into multiple arguments
   math(EXPR _last_arg ${ARGC}-1)
-  foreach(_n RANGE 1 ${_last_arg})
-    set(_arg "${ARGV${_n}}")
-    if(_arg MATCHES "[^-./:a-zA-Z0-9_]")
-      set(_args "${_args} [==[${_arg}]==]") # form a bracket_argument
-    else()
-      set(_args "${_args} ${_arg}")
-    endif()
-  endforeach()
-  set(_Command "${NAME}(${_args})\n" PARENT_SCOPE)
+  if(_last_arg GREATER_EQUAL 1)
+    foreach(_n RANGE 1 ${_last_arg})
+      set(_arg "${ARGV${_n}}")
+      if(_arg MATCHES "[^-./:a-zA-Z0-9_]")
+        set(_args "${_args} [==[${_arg}]==]") # form a bracket_argument
+      else()
+        set(_args "${_args} ${_arg}")
+      endif()
+    endforeach()
+  endif()
+  set(${out_var} "${_args}" PARENT_SCOPE)
 endfunction()
 
+
 # Generates random filename in the temp folder.
 # Temp folder is retrieved by checking env vars from various platforms.
 function(make_temp_file_path OUT_VARIABLE FALLBACK_PATH)
@@ -395,6 +388,41 @@ function(catch_discover_tests_impl)
   # so that each test name can be appended file without further processing.
   set(test_names "set(${_TEST_LIST}")
 
+  # Most of the commands/arguments in the CTest script are identical
+  # for every test registered with one `catch_discover_tests` call.
+  # To avoid repeating the work in escaping them, we escape them before
+  # the per-test loop and reuse the escaped fragments.
+  #
+  # `add_test` calls are
+  #   add_test(<name><exec><exe><escaped_name><extra_args><reporter><out_dir>)
+  # Of these, <exec>,<exe>,<extra_args>, and <reporter> are the same between
+  # all tests.
+  prepare_command_fragment(_exec_exe_fragment
+    ${_TEST_EXECUTOR}
+    "${_TEST_EXECUTABLE}"
+  )
+  prepare_command_fragment(_args_reporter_fragment
+    ${extra_args}
+    "${reporter_arg}"
+  )
+
+  # `set_tests_properties` calls are
+  #   set_tests_properties(<name> PROPERTIES WORKING_DIRECTORY <dir> <properties>)
+  #   set_tests_properties(<name> PROPERTIES ENVIRONMENT_MODIFICATION <env_mod>)
+  # Of these, only the <name> changes between tests.
+  prepare_command_fragment(_properties_fragment
+    PROPERTIES
+    WORKING_DIRECTORY "${_TEST_WORKING_DIR}"
+    ${properties}
+  )
+  # Env modification is optional, so we prepare it in a separate command
+  if(environment_modifications)
+    prepare_command_fragment(_env_modification_fragment
+      PROPERTIES
+      ENVIRONMENT_MODIFICATION "${environment_modifications}"
+    )
+  endif()
+
   # Each element in the tests is JSON-string representing one test object.
   # We have to parse it and then turn it into CTest script commands.
   foreach(single_test IN LISTS tests)
@@ -431,24 +459,15 @@ function(catch_discover_tests_impl)
       set(output_dir_arg "--out ${output_dir}/${output_prefix}${escaped_name_clean}${output_suffix}")
     endif()
 
-    # ...and add to script
-    prepare_command(add_test
-      "${prefix}${plain_name}${suffix}"
-      ${_TEST_EXECUTOR}
-      "${_TEST_EXECUTABLE}"
-      "${escaped_name}"
-      ${extra_args}
-      "${reporter_arg}"
-      "${output_dir_arg}"
-    )
-    string(APPEND script "${_Command}")
-    prepare_command(set_tests_properties
-      "${prefix}${plain_name}${suffix}"
-      PROPERTIES
-      WORKING_DIRECTORY "${_TEST_WORKING_DIR}"
-      ${properties}
-    )
-    string(APPEND script "${_Command}")
+    set(full_name "${prefix}${plain_name}${suffix}")
+    prepare_command_fragment(_full_name_fragment "${full_name}")
+    prepare_command_fragment(_escaped_name_fragment "${escaped_name}")
+    prepare_command_fragment(_outdir_fragment "${output_dir_arg}")
+
+    string(APPEND script
+      "add_test(${_full_name_fragment}${_exec_exe_fragment}${_escaped_name_fragment}${_args_reporter_fragment}${_outdir_fragment})\n")
+    string(APPEND script
+      "set_tests_properties(${_full_name_fragment}${_properties_fragment})\n")
 
     if(add_tags)
       string(JSON num_tags LENGTH "${test_tags}")
@@ -468,21 +487,16 @@ function(catch_discover_tests_impl)
           list(APPEND tag_list "${a_tag}")
         endforeach()
 
-        prepare_command(set_tests_properties
-          "${prefix}${plain_name}${suffix}"
+        prepare_command_fragment(_labels_fragment
           PROPERTIES
           LABELS "${tag_list}"
         )
-        string(APPEND script "${_Command}")
+        string(APPEND script "set_tests_properties(${_full_name_fragment}${_labels_fragment})\n")
       endif()
     endif(add_tags)
 
     if(environment_modifications)
-      prepare_command(set_tests_properties
-        "${prefix}${plain_name}${suffix}"
-        PROPERTIES
-        ENVIRONMENT_MODIFICATION "${environment_modifications}")
-      string(APPEND script "${_Command}")
+      string(APPEND script "set_tests_properties(${_full_name_fragment}${_env_modification_fragment})\n")
     endif()
 
     # The test name has to be escaped using the same rules as prepare_command

tests/CMakeLists.txt

@@ -672,11 +672,11 @@ if(CATCH_ENABLE_CMAKE_HELPER_TESTS)
       LABELS "uses-python"
   )
 
-  add_test(NAME "CMakeHelper::PrepareCommand"
+  add_test(NAME "CMakeHelper::PrepareCommandFragment"
     COMMAND
       "${CMAKE_COMMAND}"
       "-DCATCH_ADD_TESTS_SCRIPT=${CATCH_DIR}/extras/CatchAddTests.cmake"
-      -P "${CMAKE_CURRENT_LIST_DIR}/TestScripts/DiscoverTests/TestPrepareCommand.cmake"
+      -P "${CMAKE_CURRENT_LIST_DIR}/TestScripts/DiscoverTests/TestPrepareCommandFragment.cmake"
   )
 
   add_test(NAME "CMakeHelper::DecomposeJsonArray"

tests/TestScripts/DiscoverTests/TestPrepareCommandFragment.cmake

@@ -1,12 +1,12 @@
 # SPDX-License-Identifier: BSL-1.0
 
-# Unit tests for `prepare_command` helper in `extras/CatchAddTests.cmake`.
+# Unit tests for `prepare_command_fragment` 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
+#           -P TestPrepareCommandFragment.cmake
 
 
 cmake_minimum_required(VERSION 3.19)
@@ -37,25 +37,25 @@ function(expect_equal description actual expected)
   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_fragment(test_fragment SimpleName /path/to/tests)
+expect_equal("Simple arg, no quotes" "${test_fragment}" " SimpleName /path/to/tests")
 
-prepare_command(add_test "Name with spaces")
-expect_equal("Spaces in arg, needs quotes" "${_Command}" "add_test( [==[Name with spaces]==])\n")
+prepare_command_fragment(test_fragment "Name with spaces")
+expect_equal("Spaces in arg, needs quotes" "${test_fragment}" " [==[Name with spaces]==]")
 
-prepare_command(set_tests_properties Foo PROPERTIES LABELS "tagA\;tagB\;tagC")
+prepare_command_fragment(test_fragment 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")
+  "${test_fragment}" " Foo PROPERTIES LABELS [==[tagA\;tagB\;tagC]==]")
 
-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")
+set(test_fragment "PRE-EXISTING")
+prepare_command_fragment(test_fragment Foo PROPERTIES BAR baz)
+expect_equal("out var does no accumulate commands"
+  "${test_fragment}" " Foo PROPERTIES BAR baz")
 
 
 
 if(_failures GREATER 0)
-  message(FATAL_ERROR "${_failures} prepare_command test(s) failed")
+  message(FATAL_ERROR "${_failures} prepare_command_fragment test(s) failed")
 else()
-  message(STATUS "All prepare_command tests passed")
+  message(STATUS "All prepare_command_fragment tests passed")
 endif()