From 9f5fc99711dc3c651d4c1aa910d5f7cde19d877a Mon Sep 17 00:00:00 2001 From: Yuval Tassa Date: Thu, 26 Mar 2026 18:21:04 -0700 Subject: [PATCH] Convert monolithic `RecompileCompare` and `WriteReadCompare` tests into test suites, one test per file. PiperOrigin-RevId: 890149850 Change-Id: Ib3e8c032a7b43b4a3f85e7a922e25987050cd77c --- test/CMakeLists.txt | 43 +++-- test/user/user_api_test.cc | 183 ++++++++++++---------- test/xml/xml_native_writer_test.cc | 244 ++++++++++++++++------------- 3 files changed, 264 insertions(+), 206 deletions(-) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index f459a6ca..a59b55e0 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -54,20 +54,36 @@ 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_add_tests( - TARGET ${name} - SOURCES ${name}.cc - WORKING_DIRECTORY ${MUJOCO_TEST_WORKING_DIR} - TEST_LIST testList - ) - if(WIN32) - set_tests_properties( - ${testList} PROPERTIES ENVIRONMENT "PATH=$;$ENV{PATH}" + # 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 + WORKING_DIRECTORY ${MUJOCO_TEST_WORKING_DIR} + TEST_LIST testList ) - endif() - if(_ARGS_PROPERTIES) - set_tests_properties(${testList} PROPERTIES ${_ARGS_PROPERTIES}) + if(WIN32) + set_tests_properties( + ${testList} PROPERTIES ENVIRONMENT "PATH=$;$ENV{PATH}" + ) + endif() + 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) + diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 82cb2a08..eb8a28b2 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -14,10 +14,12 @@ // Tests for user/user_api.cc. +#include #include +#include #include #include -#include +#include // NOLINT #include #include #include @@ -515,97 +517,116 @@ TEST_F(MujocoTest, ModifyShellInertiaFails) { } // ------------------- test recompilation multiple files ----------------------- -TEST_F(MujocoTest, RecompileCompare) { - mjtNum tol = 0; - std::string field = ""; - // full precision float printing - FullFloatPrecision increase_precision; - - // loop over all xml files in data - std::vector paths = {GetTestDataFilePath("."), - GetModelPath(".")}; +std::vector GetRecompileTestModels() { + std::vector models; std::string ext(".xml"); - for (auto const& path : paths) { - for (auto &p : std::filesystem::recursive_directory_iterator(path)) { + 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 file is meant to fail or model is too slow to load, skip it - if (absl::StrContains(p.path().string(), "malformed_") || - absl::StrContains(p.path().string(), "_fail") || - absl::StrContains(p.path().string(), "touch_grid") || - absl::StrContains(p.path().string(), "perf") || - absl::StrContains(p.path().string(), "cow")) { + if (absl::StrContains(xml, "malformed_") || + absl::StrContains(xml, "_fail") || + absl::StrContains(xml, "touch_grid") || + absl::StrContains(xml, "perf") || + absl::StrContains(xml, "cow")) { continue; } - - // load spec - std::array err; - mjSpec* s = mj_parseXML(xml.c_str(), 0, err.data(), err.size()); - - ASSERT_THAT(s, NotNull()) - << "Failed to load " << xml << ": " << err.data(); - - // copy spec - mjSpec* s_copy = mj_copySpec(s); - - // compare signature - EXPECT_EQ(s->element->signature, s_copy->element->signature) << xml; - - // compile twice and compare - mjModel* m_old = mj_compile(s, nullptr); - - ASSERT_THAT(m_old, NotNull()) - << "Failed to compile " << xml << ": " << mjs_getError(s); - - mjModel* m_new = mj_compile(s, nullptr); - mjModel* m_copy = mj_compile(s_copy, nullptr); - - // compare signature - EXPECT_EQ(m_old->signature, m_new->signature) << xml; - EXPECT_EQ(m_old->signature, m_copy->signature) << xml; - - ASSERT_THAT(m_new, NotNull()) - << "Failed to recompile " << xml << ": " << mjs_getError(s); - ASSERT_THAT(m_copy, NotNull()) - << "Failed to compile " << xml << ": " << mjs_getError(s_copy); - - EXPECT_LE(CompareModel(m_old, m_new, field), tol) - << "Compiled and recompiled models are different!\n" - << "Affected file " << p.path().string() << '\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' - << "Different field: " << field << '\n'; - - // copy to a new spec, compile and compare - mjSpec* s_copy2 = mj_copySpec(s); - mjModel* m_copy2 = mj_compile(s_copy2, nullptr); - - ASSERT_THAT(m_copy2, NotNull()) - << "Failed to compile " << xml << ": " << mjs_getError(s_copy2); - - EXPECT_LE(CompareModel(m_old, m_copy2, field), tol) - << "Original and re-copied models are different!\n" - << "Affected file " << p.path().string() << '\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); + models.push_back(xml); } } } + return models; } +class RecompileCompareTest : public MujocoTest, + public ::testing::WithParamInterface { + public: +}; +TEST_P(RecompileCompareTest, RecompileCompare) { + std::string xml = GetParam(); + std::string field = ""; + + FullFloatPrecision increase_precision; + + // load spec + std::array err; + mjSpec* s = mj_parseXML(xml.c_str(), 0, err.data(), err.size()); + + if (!s) { + GTEST_SKIP() << "Failed to load " << xml << ": " << err.data(); + } + + // copy spec + mjSpec* s_copy = mj_copySpec(s); + + // compare signature + EXPECT_EQ(s->element->signature, s_copy->element->signature) << xml; + + // compile twice and compare + mjModel* m_old = mj_compile(s, nullptr); + + if (!m_old) { + 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); + + // compare signature + EXPECT_EQ(m_old->signature, m_new->signature) << xml; + EXPECT_EQ(m_old->signature, m_copy->signature) << xml; + + ASSERT_THAT(m_new, NotNull()) + << "Failed to recompile " << xml << ": " << mjs_getError(s); + 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 " << xml << '\n' + << "Different field: " << field << '\n'; + + EXPECT_LE(CompareModel(m_old, m_copy, field), tol) + << "Original and copied models are different!\n" + << "Affected file " << xml << '\n' + << "Different field: " << field << '\n'; + + // copy to a new spec, compile and compare + mjSpec* s_copy2 = mj_copySpec(s); + mjModel* m_copy2 = mj_compile(s_copy2, nullptr); + + ASSERT_THAT(m_copy2, NotNull()) + << "Failed to compile " << xml << ": " << mjs_getError(s_copy2); + + EXPECT_LE(CompareModel(m_old, m_copy2, field), tol) + << "Original and re-copied models are different!\n" + << "Affected file " << xml << '\n' + << "Different field: " << field << '\n'; + + 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& 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"( diff --git a/test/xml/xml_native_writer_test.cc b/test/xml/xml_native_writer_test.cc index 1271dddb..baa613fa 100644 --- a/test/xml/xml_native_writer_test.cc +++ b/test/xml/xml_native_writer_test.cc @@ -14,6 +14,7 @@ // Tests for xml/xml_native_writer.cc. +#include #include #if defined(__unix__) || (defined(__APPLE__) && defined(__MACH__)) #include @@ -1376,131 +1377,150 @@ TEST_F(XMLWriterTest, NonRGBTextures) { } // ------------------- test loading and saving multiple files ------------------ -TEST_F(XMLWriterTest, WriteReadCompare) { - // full precision float printing - FullFloatPrecision increase_precision; - - // loop over all xml files in data - std::vector paths = {GetTestDataFilePath("."), - GetModelPath(".")}; +std::vector GetWriteReadTestModels() { + std::vector models; std::string ext(".xml"); - for (auto const& path : paths) { - for (auto &p : std::filesystem::recursive_directory_iterator(path)) { + 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(p.path().string(), "malformed_") || - absl::StrContains(p.path().string(), "_fail") || + absl::StrContains(xml, "malformed_") || + absl::StrContains(xml, "_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") || + 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(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")) { + 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; } - // load model - std::array error; - mjSpec* s = - mj_parseXML(xml.c_str(), nullptr, error.data(), error.size()); - ASSERT_THAT(s, NotNull()) - << "Failed to load " << xml.c_str() << ": " << error.data(); - mjModel* m = mj_compile(s, nullptr); - ASSERT_THAT(m, NotNull()) - << "Failed to load " << 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(); - mjSpec* stemp = mj_parseXMLString(SaveAndReadXml(s).c_str(), 0, - error.data(), 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()) - << error.data() << " from " << xml.c_str(); - - mjtNum tol = 0; - - // for particularly sensitive models, relax the tolerance - if (absl::StrContains(p.path().string(), "belt.xml") || - absl::StrContains(p.path().string(), "cable.xml")) { - tol = 1e-13; - } - - // compare and delete - std::string field = ""; - mjtNum result = CompareModel(m, mtemp, field); - EXPECT_LE(result, tol) - << "Loaded and saved models are different!\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'; - - // delete data - mj_deleteData(d); - - // allocate buffer, save m into it - size_t sz = mj_sizeModel(m); - void* buffer = mju_malloc(sz); - mj_saveModel(m, nullptr, buffer, sz); - - // make new VFS add buffer to it - mjVFS* vfs = (mjVFS*)mju_malloc(sizeof(mjVFS)); - mj_defaultVFS(vfs); - int failed = mj_addBufferVFS(vfs, "model.mjb", buffer, sz); - EXPECT_EQ(failed, 0) << "Failed to add buffer to VFS"; - - // load model from VFS - mtemp = mj_loadModel("model.mjb", vfs); - ASSERT_THAT(mtemp, NotNull()); - - // compare with 0 tolerance - field = ""; - result = CompareModel(m, mtemp, field); - EXPECT_EQ(result, 0) - << "Loaded and saved binary models are different!\n" - << "Affected file " << p.path().string() << '\n' - << "Different field: " << field << '\n'; - - // clean up - mj_deleteSpec(s); - mj_deleteSpec(stemp); - mj_deleteModel(m); - mj_deleteModel(mtemp); - mj_deleteVFS(vfs); - mju_free(vfs); - mju_free(buffer); + models.push_back(xml); } } } + return models; } +class WriteReadCompareTest : public XMLWriterTest, + public ::testing::WithParamInterface { + public: +}; +TEST_P(WriteReadCompareTest, WriteReadCompare) { + std::string xml = GetParam(); + + // full precision float printing + FullFloatPrecision increase_precision; + + // load model + std::array error; + mjSpec* s = + mj_parseXML(xml.c_str(), nullptr, error.data(), error.size()); + if (!s) { + GTEST_SKIP() << "Failed to load " << xml.c_str() << ": " << error.data(); + } + + mjModel* m = mj_compile(s, nullptr); + if (!m) { + 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 = std::filesystem::path(xml); + mjSpec* stemp = mj_parseXMLString(SaveAndReadXml(s).c_str(), 0, error.data(), + 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()) << error.data() << " from " << xml.c_str(); + + mjtNum tol = 0; + + // for particularly sensitive models, relax the tolerance + if (absl::StrContains(xml, "belt.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) << "Loaded and saved models are different!\n" + << "Affected file " << xml << '\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 " << + xml << '\n'; + + // delete data + mj_deleteData(d); + + // allocate buffer, save m into it + size_t sz = mj_sizeModel(m); + void* buffer = mju_malloc(sz); + mj_saveModel(m, nullptr, buffer, sz); + + // make new VFS add buffer to it + mjVFS* vfs = (mjVFS*)mju_malloc(sizeof(mjVFS)); + mj_defaultVFS(vfs); + int failed = mj_addBufferVFS(vfs, "model.mjb", buffer, sz); + EXPECT_EQ(failed, 0) << "Failed to add buffer to VFS"; + + // load model from VFS + mtemp = mj_loadModel("model.mjb", vfs); + ASSERT_THAT(mtemp, NotNull()); + + // compare with 0 tolerance + field = ""; + result = CompareModel(m, mtemp, field); + EXPECT_EQ(result, 0) << "Loaded and saved binary models are different!\n" + << "Affected file " << xml << '\n' + << "Different field: " << field << '\n'; + + // clean up + mj_deleteSpec(s); + mj_deleteSpec(stemp); + mj_deleteModel(m); + mj_deleteModel(mtemp); + mj_deleteVFS(vfs); + mju_free(vfs); + mju_free(buffer); +} + +INSTANTIATE_TEST_SUITE_P( + AllModels, WriteReadCompareTest, + ::testing::ValuesIn(GetWriteReadTestModels()), + [](const ::testing::TestParamInfo& 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; TEST_F(DecompilerTest, SavesStatistics) {