GoogleTest: Clear script content buffer on flush and flush less often

There's no need to check if flushing is needed after every command
is added to the variable holding the script content. It is enough to only
check once per test name. This simplifies the flushing logic, removing
the need for a separate flush_script() command. Previously, we were
not clearing the flushed script contents in all cases, but this is now
rigorously enforced at the one location where flushing is performed.

Also simplify the flushing of the list of test names, since that too doesn't
need a separate command. It is simpler and safer to handle that
directly inline where the one call to flush_tests_buffer() was
previously being made.

Fixes: #26431

5 files changed, 74 insertions, 45 deletions

diff --git a/Modules/GoogleTestAddTests.cmake b/Modules/GoogleTestAddTests.cmake
index 0374b8f..1b6275d 100644
--- a/Modules/GoogleTestAddTests.cmake
+++ b/Modules/GoogleTestAddTests.cmake
@@ -4,23 +4,6 @@
 cmake_minimum_required(VERSION 3.30)
 cmake_policy(SET CMP0174 NEW)   # TODO: Remove this when we can update the above to 3.31
 
-# Overwrite possibly existing ${arg_CTEST_FILE} with empty file
-set(flush_tests_MODE WRITE)
-
-# Flushes script to ${arg_CTEST_FILE}
-macro(flush_script)
-  file(${flush_tests_MODE} "${arg_CTEST_FILE}" "${script}")
-  set(flush_tests_MODE APPEND PARENT_SCOPE)
-
-  set(script "")
-endmacro()
-
-# Flushes tests_buffer to tests
-macro(flush_tests_buffer)
-  list(APPEND tests "${tests_buffer}")
-  set(tests_buffer "")
-endmacro()
-
 function(add_command name test_name)
   set(args "")
   foreach(arg ${ARGN})
@@ -31,10 +14,6 @@ function(add_command name test_name)
     endif()
   endforeach()
   string(APPEND script "${name}(${test_name} ${args})\n")
-  string(LENGTH "${script}" script_len)
-  if(${script_len} GREATER "50000")
-    flush_script()
-  endif()
   set(script "${script}" PARENT_SCOPE)
 endfunction()
 
@@ -97,7 +76,12 @@ function(gtest_discover_tests_impl)
   set(script)
   set(suite)
   set(tests)
-  set(tests_buffer)
+  set(tests_buffer "")
+
+  # If a file at ${arg_CTEST_FILE} already exists, we overwrite it.
+  # For performance reasons, we write to this file in chunks, and this variable
+  # is updated to APPEND after the first write.
+  set(file_write_mode WRITE)
 
   if(arg_TEST_FILTER)
     set(filter "--gtest_filter=${arg_TEST_FILTER}")
@@ -229,14 +213,6 @@ function(gtest_discover_tests_impl)
           string(APPEND script " [==[${extra_args}]==]")
         endif()
         string(APPEND script ")\n")
-        string(LENGTH "${script}" script_len)
-        if(${script_len} GREATER "50000")
-          # flush_script() expects to set variables in the parent scope, so we
-          # need to create one since we actually want the changes in our scope
-          block(SCOPE_FOR VARIABLES)
-            flush_script()
-          endblock()
-        endif()
 
         if(suite MATCHES "^DISABLED_" OR test MATCHES "^DISABLED_")
           add_command(set_tests_properties
@@ -260,22 +236,39 @@ function(gtest_discover_tests_impl)
           string(REPLACE [[;]] [[\\;]] testname "${testname}")
           list(APPEND tests_buffer "${testname}")
           list(LENGTH tests_buffer tests_buffer_length)
-          if(${tests_buffer_length} GREATER "250")
-            flush_tests_buffer()
+          if(tests_buffer_length GREATER "250")
+            # Chunk updates to the final "tests" variable, keeping the
+            # "tests_buffer" variable that we append each test to relatively
+            # small. This mitigates worsening performance impacts for the
+            # corner case of having many thousands of tests.
+            list(APPEND tests "${tests_buffer}")
+            set(tests_buffer "")
           endif()
         endif()
       endif()
+
+      # If we've built up a sizable script so far, write it out as a chunk now
+      # so we don't accumulate a massive string to write at the end
+      string(LENGTH "${script}" script_len)
+      if(${script_len} GREATER "50000")
+        file(${file_write_mode} "${arg_CTEST_FILE}" "${script}")
+        set(file_write_mode APPEND)
+        set(script "")
+      endif()
+
     endif()
   endforeach()
 
+  if(NOT tests_buffer STREQUAL "")
+    list(APPEND tests "${tests_buffer}")
+  endif()
 
   # Create a list of all discovered tests, which users may use to e.g. set
   # properties on the tests
-  flush_tests_buffer()
   add_command(set "" ${arg_TEST_LIST} "${tests}")
 
-  # Write CTest script
-  flush_script()
+  # Write remaining content to the CTest script
+  file(${file_write_mode} "${arg_CTEST_FILE}" "${script}")
 
 endfunction()
 
diff --git a/Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake b/Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake
deleted file mode 100644
index 1ae222f..0000000
--- a/Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake
+++ /dev/null
@@ -1,5 +0,0 @@
-list(LENGTH flush_script_test_TESTS LIST_SIZE)
-set(EXPECTED_LIST_SIZE 4)
-if(NOT LIST_SIZE EQUAL ${EXPECTED_LIST_SIZE})
-  message("TEST_LIST should have ${EXPECTED_LIST_SIZE} elements but it has ${LIST_SIZE}")
-endif()
diff --git a/Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake.in b/Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake.in
new file mode 100644
index 0000000..b334d2b
--- /dev/null
+++ b/Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake.in
@@ -0,0 +1,34 @@
+set(expected_number_of_tests 12)
+
+# Check the flushing of the test names buffer
+list(LENGTH flush_script_test_TESTS num_test_names)
+if(NOT num_test_names EQUAL expected_number_of_tests)
+  message(FATAL_ERROR
+    "Test name list has wrong number of test names:\n"
+    "  Expected: ${expected_number_of_tests}\n"
+    "  Actual:   ${num_test_names}"
+  )
+endif()
+
+# Check the flushing of the script content variable.
+# Note that flushing errors would repeat a test name, so such errors are not
+# uncovered by checking the name buffer flushing above.
+
+# PRE_TEST can have a config-specific tests file, POST_BUILD never does
+set(tests_file "@CMAKE_CURRENT_BINARY_DIR@/flush_script_test[1]_tests-Debug.cmake")
+if(NOT EXISTS "${tests_file}")
+  set(tests_file "@CMAKE_CURRENT_BINARY_DIR@/flush_script_test[1]_tests.cmake")
+endif()
+if(NOT EXISTS "${tests_file}")
+  message(FATAL_ERROR "Tests file is missing")
+endif()
+
+file(STRINGS "${tests_file}" add_test_lines REGEX "^add_test" ENCODING UTF-8)
+list(LENGTH add_test_lines num_add_test_lines)
+if(NOT num_add_test_lines EQUAL expected_number_of_tests)
+  message(FATAL_ERROR
+    "Test script has wrong number of add_test() calls:\n"
+    "  Expected: ${expected_number_of_tests}\n"
+    "  Actual:   ${num_add_test_lines}"
+  )
+endif()
diff --git a/Tests/RunCMake/GoogleTest/GoogleTestDiscoveryFlushScript.cmake b/Tests/RunCMake/GoogleTest/GoogleTestDiscoveryFlushScript.cmake
index 2c138c7..b537a5f 100644
--- a/Tests/RunCMake/GoogleTest/GoogleTestDiscoveryFlushScript.cmake
+++ b/Tests/RunCMake/GoogleTest/GoogleTestDiscoveryFlushScript.cmake
@@ -10,5 +10,11 @@ xcode_sign_adhoc(flush_script_test)
 gtest_discover_tests(
   flush_script_test
 )
+
+configure_file(GoogleTest-discovery-flush-script-check-list.cmake.in
+  check-test-lists.cmake
+  @ONLY
+)
 set_property(DIRECTORY APPEND PROPERTY TEST_INCLUDE_FILES
-  ${CMAKE_CURRENT_SOURCE_DIR}/GoogleTest-discovery-flush-script-check-list.cmake)
+  ${CMAKE_CURRENT_BINARY_DIR}/check-test-lists.cmake
+)
diff --git a/Tests/RunCMake/GoogleTest/flush_script_test.cpp b/Tests/RunCMake/GoogleTest/flush_script_test.cpp
index 9473bb5..f032dda 100644
--- a/Tests/RunCMake/GoogleTest/flush_script_test.cpp
+++ b/Tests/RunCMake/GoogleTest/flush_script_test.cpp
@@ -10,10 +10,11 @@ int main(int argc, char** argv)
   if (argc > 1 && std::string(argv[1]) == "--gtest_list_tests") {
     std::cout << "flush_script_test.\n";
     const size_t flushThreshold = 50000;
-    const size_t testCaseNum = 4;
-    std::string testName(flushThreshold / (testCaseNum - 1), 'T');
-    for (size_t i = 0; i < testCaseNum; ++i)
-      std::cout << "  " << testName.c_str() << "\n";
+    const size_t flushAfter = 4;
+    const size_t testCaseNum = 3 * flushAfter;
+    std::string testName(flushThreshold / flushAfter, 'T');
+    for (size_t i = 1; i <= testCaseNum; ++i)
+      std::cout << "  t" << i << testName.c_str() << "\n";
   }
   return 0;
 }