Convert monolithic RecompileCompare and WriteReadCompare tests into test suites, one test per file.

PiperOrigin-RevId: 890149850
Change-Id: Ib3e8c032a7b43b4a3f85e7a922e25987050cd77c

test/CMakeLists.txt

@@ -54,7 +54,9 @@ macro(mujoco_test name)
   endif()
   target_include_directories(${name} PRIVATE ${MUJOCO_TEST_INCLUDE})
   set_target_properties(${name} PROPERTIES BUILD_RPATH ${CMAKE_LIBRARY_OUTPUT_DIRECTORY})
-  # gtest_discover_tests is recommended over gtest_add_tests, but has some issues in Windows.
+  # gtest_discover_tests is recommended over gtest_add_tests, but has issues on Windows.
+  # It also requires a GoogleTest binary, so we fall back for custom MAIN_TARGET (e.g. benchmarks).
+  if(WIN32 OR _ARGS_MAIN_TARGET)
     gtest_add_tests(
       TARGET ${name}
       SOURCES ${name}.cc
@@ -69,6 +71,20 @@ macro(mujoco_test name)
     if(_ARGS_PROPERTIES)
       set_tests_properties(${testList} PROPERTIES ${_ARGS_PROPERTIES})
     endif()
+  else()
+    if(_ARGS_PROPERTIES)
+      gtest_discover_tests(
+        ${name}
+        WORKING_DIRECTORY ${MUJOCO_TEST_WORKING_DIR}
+        PROPERTIES ${_ARGS_PROPERTIES}
+      )
+    else()
+      gtest_discover_tests(
+        ${name}
+        WORKING_DIRECTORY ${MUJOCO_TEST_WORKING_DIR}
+      )
+    endif()
+  endif()
 
 endmacro()
 
@@ -102,3 +118,4 @@ add_subdirectory(xml)
 add_subdirectory(plugin/elasticity)
 add_subdirectory(plugin/actuator)
 add_subdirectory(experimental)
+

test/user/user_api_test.cc

@@ -14,10 +14,12 @@
 
 // Tests for user/user_api.cc.
 
+#include <algorithm>
 #include <array>
+#include <cctype>
 #include <cstddef>
 #include <cstdint>
-#include <filesystem>
+#include <filesystem>  // NOLINT
 #include <functional>
 #include <map>
 #include <memory>
@@ -515,37 +517,45 @@ TEST_F(MujocoTest, ModifyShellInertiaFails) {
 }
 
 // ------------------- test recompilation multiple files -----------------------
-TEST_F(MujocoTest, RecompileCompare) {
-  mjtNum tol = 0;
-  std::string field = "";
 
-  // full precision float printing
+std::vector<std::string> GetRecompileTestModels() {
-  FullFloatPrecision increase_precision;
+  std::vector<std::string> models;
-
-  // loop over all xml files in data
-  std::vector<std::string> paths = {GetTestDataFilePath("."),
-                                    GetModelPath(".")};
   std::string ext(".xml");
-  for (auto const& path : paths) {
+  for (const auto& path : {GetTestDataFilePath("."), GetModelPath(".")}) {
-    for (auto &p : std::filesystem::recursive_directory_iterator(path)) {
+    for (const auto& p : std::filesystem::recursive_directory_iterator(path)) {
       if (p.path().extension() == ext) {
         std::string xml = p.path().string();
-
+        if (absl::StrContains(xml, "malformed_") ||
-        // if file is meant to fail or model is too slow to load, skip it
+            absl::StrContains(xml, "_fail") ||
-        if (absl::StrContains(p.path().string(), "malformed_") ||
+            absl::StrContains(xml, "touch_grid") ||
-            absl::StrContains(p.path().string(), "_fail") ||
+            absl::StrContains(xml, "perf") ||
-            absl::StrContains(p.path().string(), "touch_grid") ||
+            absl::StrContains(xml, "cow")) {
-            absl::StrContains(p.path().string(), "perf") ||
-            absl::StrContains(p.path().string(), "cow")) {
           continue;
         }
+        models.push_back(xml);
+      }
+    }
+  }
+  return models;
+}
+
+class RecompileCompareTest : public MujocoTest,
+                             public ::testing::WithParamInterface<std::string> {
+ public:
+};
+TEST_P(RecompileCompareTest, RecompileCompare) {
+  std::string xml = GetParam();
+  std::string field = "";
+
+  FullFloatPrecision increase_precision;
 
   // load spec
   std::array<char, 1000> err;
   mjSpec* s = mj_parseXML(xml.c_str(), 0, err.data(), err.size());
 
-        ASSERT_THAT(s, NotNull())
+  if (!s) {
-            << "Failed to load " << xml << ": " << err.data();
+    GTEST_SKIP() << "Failed to load " << xml << ": " << err.data();
+  }
 
   // copy spec
   mjSpec* s_copy = mj_copySpec(s);
@@ -556,8 +566,10 @@ TEST_F(MujocoTest, RecompileCompare) {
   // compile twice and compare
   mjModel* m_old = mj_compile(s, nullptr);
 
-        ASSERT_THAT(m_old, NotNull())
+  if (!m_old) {
-            << "Failed to compile " << xml << ": " << mjs_getError(s);
+    mj_deleteSpec(s);
+    GTEST_SKIP() << "Failed to compile " << xml << ": " << mjs_getError(s);
+  }
 
   mjModel* m_new = mj_compile(s, nullptr);
   mjModel* m_copy = mj_compile(s_copy, nullptr);
@@ -571,14 +583,16 @@ TEST_F(MujocoTest, RecompileCompare) {
   ASSERT_THAT(m_copy, NotNull())
       << "Failed to compile " << xml << ": " << mjs_getError(s_copy);
 
+  mjtNum tol = 0;
+
   EXPECT_LE(CompareModel(m_old, m_new, field), tol)
       << "Compiled and recompiled models are different!\n"
-            << "Affected file " << p.path().string() << '\n'
+      << "Affected file " << xml << '\n'
       << "Different field: " << field << '\n';
 
   EXPECT_LE(CompareModel(m_old, m_copy, field), tol)
       << "Original and copied models are different!\n"
-            << "Affected file " << p.path().string() << '\n'
+      << "Affected file " << xml << '\n'
       << "Different field: " << field << '\n';
 
   // copy to a new spec, compile and compare
@@ -590,21 +604,28 @@ TEST_F(MujocoTest, RecompileCompare) {
 
   EXPECT_LE(CompareModel(m_old, m_copy2, field), tol)
       << "Original and re-copied models are different!\n"
-            << "Affected file " << p.path().string() << '\n'
+      << "Affected file " << xml << '\n'
       << "Different field: " << field << '\n';
 
-        // delete models
-        mj_deleteSpec(s);
-        mj_deleteSpec(s_copy);
-        mj_deleteSpec(s_copy2);
-        mj_deleteModel(m_old);
   mj_deleteModel(m_new);
   mj_deleteModel(m_copy);
   mj_deleteModel(m_copy2);
+  mj_deleteSpec(s_copy);
+  mj_deleteSpec(s_copy2);
+  mj_deleteSpec(s);
+  mj_deleteModel(m_old);
 }
-    }
+
-  }
+INSTANTIATE_TEST_SUITE_P(
-}
+    AllModels, RecompileCompareTest,
+    ::testing::ValuesIn(GetRecompileTestModels()),
+    [](const ::testing::TestParamInfo<std::string>& info) {
+      std::string name = std::filesystem::path(info.param).filename().string();
+      std::replace_if(
+          name.begin(), name.end(),
+          [](char c) { return !std::isalnum(c); }, '_');
+      return name + "_" + std::to_string(info.index);
+    });
 
 TEST_F(MujocoTest, RecompileEdit) {
   static constexpr char xml[] = R"(

test/xml/xml_native_writer_test.cc

@@ -14,6 +14,7 @@
 
 // Tests for xml/xml_native_writer.cc.
 
+#include <cctype>
 #include <memory>
 #if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__))
 #include <unistd.h>
@@ -1376,91 +1377,103 @@ TEST_F(XMLWriterTest, NonRGBTextures) {
 }
 
 // ------------------- test loading and saving multiple files ------------------
-TEST_F(XMLWriterTest, WriteReadCompare) {
+std::vector<std::string> GetWriteReadTestModels() {
+  std::vector<std::string> models;
+  std::string ext(".xml");
+  for (const auto& path : {GetTestDataFilePath("."), GetModelPath(".")}) {
+    for (const auto& p : std::filesystem::recursive_directory_iterator(path)) {
+      if (p.path().extension() == ext) {
+        std::string xml = p.path().string();
+        if (  // if file is meant to fail, skip it
+              absl::StrContains(xml, "malformed_") ||
+              absl::StrContains(xml, "_fail") ||
+              // exclude files that are too slow to load
+              absl::StrContains(xml, "cow") ||
+              absl::StrContains(xml, "gmsh_") ||
+              absl::StrContains(xml, "shark_") ||
+              absl::StrContains(xml, "perf") ||
+              // exclude files that fail the comparison test
+              absl::StrContains(xml, "rfcamera") ||
+              absl::StrContains(xml, "tactile") ||
+              absl::StrContains(xml, "makemesh") ||
+              absl::StrContains(xml, "many_dependencies") ||
+              absl::StrContains(xml, "usd") ||
+              absl::StrContains(xml, "torus_maxhull") ||
+              absl::StrContains(xml, "fitmesh_") ||
+              absl::StrContains(xml, "lengthrange") ||
+              absl::StrContains(xml, "hfield_xml") ||
+              absl::StrContains(xml, "fromto_convex") ||
+              absl::StrContains(xml, "cube_skin") ||
+              absl::StrContains(xml, "cube_3x3x3")) {
+          continue;
+        }
+        models.push_back(xml);
+      }
+    }
+  }
+  return models;
+}
+
+class WriteReadCompareTest : public XMLWriterTest,
+                             public ::testing::WithParamInterface<std::string> {
+ public:
+};
+TEST_P(WriteReadCompareTest, WriteReadCompare) {
+  std::string xml = GetParam();
+
   // full precision float printing
   FullFloatPrecision increase_precision;
 
-  // loop over all xml files in data
-  std::vector<std::string> paths = {GetTestDataFilePath("."),
-                                    GetModelPath(".")};
-  std::string ext(".xml");
-  for (auto const& path : paths) {
-    for (auto &p : std::filesystem::recursive_directory_iterator(path)) {
-      if (p.path().extension() == ext) {
-        std::string xml = p.path().string();
-
-
-        if (  // if file is meant to fail, skip it
-              absl::StrContains(p.path().string(), "malformed_") ||
-              absl::StrContains(p.path().string(), "_fail") ||
-              // exclude files that are too slow to load
-              absl::StrContains(p.path().string(), "cow") ||
-              absl::StrContains(p.path().string(), "gmsh_") ||
-              absl::StrContains(p.path().string(), "shark_") ||
-              absl::StrContains(p.path().string(), "perf") ||
-              // exclude files that fail the comparison test
-              absl::StrContains(p.path().string(), "rfcamera") ||
-              absl::StrContains(p.path().string(), "tactile") ||
-              absl::StrContains(p.path().string(), "makemesh") ||
-              absl::StrContains(p.path().string(), "many_dependencies") ||
-              absl::StrContains(p.path().string(), "usd") ||
-              absl::StrContains(p.path().string(), "torus_maxhull") ||
-              absl::StrContains(p.path().string(), "fitmesh_") ||
-              absl::StrContains(p.path().string(), "lengthrange") ||
-              absl::StrContains(p.path().string(), "hfield_xml") ||
-              absl::StrContains(p.path().string(), "fromto_convex") ||
-              absl::StrContains(p.path().string(), "cube_skin") ||
-              absl::StrContains(p.path().string(), "cube_3x3x3")) {
-          continue;
-        }
   // load model
   std::array<char, 1000> error;
   mjSpec* s =
       mj_parseXML(xml.c_str(), nullptr, error.data(), error.size());
-        ASSERT_THAT(s, NotNull())
+  if (!s) {
-            << "Failed to load " << xml.c_str() << ": " << error.data();
+    GTEST_SKIP() << "Failed to load " << xml.c_str() << ": " << error.data();
+  }
+
   mjModel* m = mj_compile(s, nullptr);
-        ASSERT_THAT(m, NotNull())
+  if (!m) {
-            << "Failed to load " << xml.c_str() << ": " << error.data();
+    mj_deleteSpec(s);
+    GTEST_SKIP() << "Failed to compile " << xml.c_str() << ": " << error.data();
+  }
 
   // make data
   mjData* d = mj_makeData(m);
   ASSERT_THAT(d, testing::NotNull()) << "Failed to create data\n";
 
   // save and load back
-        auto abs_path = p.path();
+  auto abs_path = std::filesystem::path(xml);
-        mjSpec* stemp = mj_parseXMLString(SaveAndReadXml(s).c_str(), 0,
+  mjSpec* stemp = mj_parseXMLString(SaveAndReadXml(s).c_str(), 0, error.data(),
-                                          error.data(), error.size());
+                                    error.size());
   ASSERT_THAT(stemp, NotNull())
       << "Failed to load " << xml.c_str() << ": " << error.data();
   mjs_setString(stemp->modelfiledir,
                 abs_path.remove_filename().string().c_str());
   mjModel* mtemp = mj_compile(stemp, nullptr);
 
-        ASSERT_THAT(mtemp, NotNull())
+  ASSERT_THAT(mtemp, NotNull()) << error.data() << " from " << xml.c_str();
-            << error.data() << " from " << xml.c_str();
 
   mjtNum tol = 0;
 
   // for particularly sensitive models, relax the tolerance
-        if (absl::StrContains(p.path().string(), "belt.xml") ||
+  if (absl::StrContains(xml, "belt.xml") ||
-            absl::StrContains(p.path().string(), "cable.xml")) {
+      absl::StrContains(xml, "cable.xml")) {
     tol = 1e-13;
   }
 
   // compare and delete
   std::string field = "";
   mjtNum result = CompareModel(m, mtemp, field);
-        EXPECT_LE(result, tol)
+  EXPECT_LE(result, tol) << "Loaded and saved models are different!\n"
-            << "Loaded and saved models are different!\n"
+                         << "Affected file " << xml << '\n'
-            << "Affected file " << p.path().string() << '\n'
                          << "Different field: " << field << '\n';
   mj_deleteModel(mtemp);
 
   // check for stack memory leak
   mj_step(m, d);
   EXPECT_EQ(d->pstack, 0) << "mjData stack memory leak detected in " <<
-            p.path().string() << '\n';
+      xml << '\n';
 
   // delete data
   mj_deleteData(d);
@@ -1483,9 +1496,8 @@ TEST_F(XMLWriterTest, WriteReadCompare) {
   // compare with 0 tolerance
   field = "";
   result = CompareModel(m, mtemp, field);
-        EXPECT_EQ(result, 0)
+  EXPECT_EQ(result, 0) << "Loaded and saved binary models are different!\n"
-            << "Loaded and saved binary models are different!\n"
+                       << "Affected file " << xml << '\n'
-            << "Affected file " << p.path().string() << '\n'
                        << "Different field: " << field << '\n';
 
   // clean up
@@ -1497,9 +1509,17 @@ TEST_F(XMLWriterTest, WriteReadCompare) {
   mju_free(vfs);
   mju_free(buffer);
 }
-    }
+
-  }
+INSTANTIATE_TEST_SUITE_P(
-}
+    AllModels, WriteReadCompareTest,
+    ::testing::ValuesIn(GetWriteReadTestModels()),
+    [](const ::testing::TestParamInfo<std::string>& info) {
+      std::string name = std::filesystem::path(info.param).filename().string();
+      std::replace_if(
+          name.begin(), name.end(),
+          [](char c) { return !std::isalnum(c); }, '_');
+      return name + "_" + std::to_string(info.index);
+    });
 
 // ---------------- test CopyBack functionality (decompiler) ------------------
 using DecompilerTest = MujocoTest;