Merge topic 'gtest-discover-flush-duplicates'

007d7ea648 GoogleTest: Avoid extra call to set DISABLED test property in discovery
9a24c1e802 GoogleTest: Clear script content buffer on flush and flush less often

Acked-by: Kitware Robot <kwrobot@kitware.com>
Tested-by: buildbot <buildbot@kitware.com>
Merge-request: !9982

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,25 +213,16 @@ 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()
 
+        set(maybe_disabled "")
         if(suite MATCHES "^DISABLED_" OR test MATCHES "^DISABLED_")
-          add_command(set_tests_properties
-            "${guarded_testname}"
-            PROPERTIES DISABLED TRUE
-          )
+          set(maybe_disabled DISABLED TRUE)
         endif()
 
         add_command(set_tests_properties
           "${guarded_testname}"
           PROPERTIES
+          ${maybe_disabled}
           WORKING_DIRECTORY "${arg_TEST_WORKING_DIR}"
           SKIP_REGULAR_EXPRESSION "\\[  SKIPPED \\]"
           ${arg_TEST_PROPERTIES}
@@ -260,22 +235,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()
 

Tests/RunCMake/GoogleTest/GoogleTest-discovery-flush-script-check-list.cmake

@@ -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()

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()

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
+)

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;
 }