From cedaa47d6192e42ff1441dd2e82e74ef28e15076 Mon Sep 17 00:00:00 2001 From: Yuval Tassa Date: Wed, 17 Jun 2026 05:06:15 -0700 Subject: [PATCH] Migrate tests to use `MockWarningHandler` for warning interception. PiperOrigin-RevId: 933656759 Change-Id: Ia21514c302799034bf598445933dd90427284737 --- test/engine/engine_forward_test.cc | 31 ++++++------ test/engine/engine_io_test.cc | 19 ++------ test/fixture.cc | 25 ++++++---- test/fixture.h | 5 +- test/user/user_api_test.cc | 15 ++++-- test/user/user_mesh_test.cc | 8 +-- test/user/user_resource_test.cc | 78 +++++++----------------------- test/user/user_util_test.cc | 4 +- test/xml/xml_native_reader_test.cc | 9 +--- 9 files changed, 72 insertions(+), 122 deletions(-) diff --git a/test/engine/engine_forward_test.cc b/test/engine/engine_forward_test.cc index f77ec394..9d2db304 100644 --- a/test/engine/engine_forward_test.cc +++ b/test/engine/engine_forward_test.cc @@ -31,7 +31,6 @@ #include #include #include -#include "src/cc/array_safety.h" #include "src/engine/engine_callback.h" #include "src/engine/engine_core_util.h" #include "src/engine/engine_io.h" @@ -68,10 +67,11 @@ static const char* const kTendonForceClamp = using ::testing::Pointwise; -using ::testing::Ne; -using ::testing::HasSubstr; -using ::testing::NotNull; +using ::testing::_; using ::testing::Gt; +using ::testing::HasSubstr; +using ::testing::Ne; +using ::testing::NotNull; // --------------------------- activation limits ------------------------------- @@ -997,32 +997,28 @@ TEST_F(ForwardTest, ControlClamping) { // data->ctrl[1] remains pristine EXPECT_EQ(data->ctrl[1], 2); - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; // for the unclamped actuator, huge raises warning + warning_handler.ExpectWarnings( + "Nan, Inf or huge value in CTRL at ACTUATOR 0"); data->ctrl[0] = 10*mjMAXVAL; mj_forward(model, data); - EXPECT_THAT(warning, - HasSubstr("Nan, Inf or huge value in CTRL at ACTUATOR 0")); + testing::Mock::VerifyAndClearExpectations(&warning_handler); // for the clamped actuator, huge does not raise warning + EXPECT_CALL(warning_handler, Warn(_)).Times(0); mj_resetData(model, data); - warning[0] = '\0'; data->ctrl[1] = 10*mjMAXVAL; mj_forward(model, data); - EXPECT_EQ(warning[0], '\0'); + testing::Mock::VerifyAndClearExpectations(&warning_handler); // for the clamped actuator, NaN raises warning + warning_handler.ExpectWarnings( + "Nan, Inf or huge value in CTRL at ACTUATOR 1"); mj_resetData(model, data); data->ctrl[1] = std::numeric_limits::quiet_NaN(); mj_forward(model, data); - EXPECT_THAT(warning, - HasSubstr("Nan, Inf or huge value in CTRL at ACTUATOR 1")); mj_deleteData(data); mj_deleteModel(model); @@ -2865,7 +2861,8 @@ TEST_F(ActEarlyTest, RemovesOneStepDelay) { mj_step(model, data); for (int j = 0; j < model->nu / 2; j++) { // this is true for torque actuators - EXPECT_NEAR(last_qfrc[2 * j], data->qfrc_actuator[2 * j + 1], MjTol(1e-3, 1e-1)) + EXPECT_NEAR(last_qfrc[2 * j], data->qfrc_actuator[2 * j + 1], + MjTol(1e-3, 1e-1)) << "there should be a 1 step delay between qfrc for " << mj_id2name(model, mjOBJ_ACTUATOR, 2 * j); } diff --git a/test/engine/engine_io_test.cc b/test/engine/engine_io_test.cc index f637c303..6a443944 100644 --- a/test/engine/engine_io_test.cc +++ b/test/engine/engine_io_test.cc @@ -31,7 +31,6 @@ #include #include #include -#include "src/engine/engine_util_errmem.h" #include "src/engine/engine_thread.h" #include "test/fixture.h" @@ -130,14 +129,10 @@ TEST_F(EngineIoTest, MakeDataReturnsNullOnFailure) { // fail mj_makeData intentionally with a bad size model->nbody = -1; - static bool warning; - warning = false; - mju_user_warning = [](const char* error) { - warning = true; - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); mjData* data = mj_makeData(model); EXPECT_THAT(data, IsNull()); - EXPECT_TRUE(warning) << "Expecting warning to be triggered."; mj_deleteData(data); mj_deleteModel(model); @@ -964,18 +959,14 @@ TEST_F(EngineIoTest, LoadModelBufferRejectsOverflowingSizes) { // nbuffer mismatch check — but the overflow should be caught earlier // in safeAddToBufferSize/mj_makeModel before we reach that check. - // intercept mju_warning because the test framework translates it to ADD_FAILURE - static bool warning_triggered = false; - warning_triggered = false; - mju_user_warning = [](const char* msg) { - warning_triggered = true; - }; + // Intercept warnings to prevent them from failing the test. + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); // attempt to load — should return NULL, not crash mjModel* bad_model = mj_loadModelBuffer(buffer.data(), bufsize); EXPECT_THAT(bad_model, IsNull()) << "Expected mj_loadModelBuffer to reject overflow-inducing sizes"; - EXPECT_TRUE(warning_triggered) << "Expected a warning about invalid sizes"; // clean up if somehow it succeeded if (bad_model) { diff --git a/test/fixture.cc b/test/fixture.cc index 4e720651..f342365e 100644 --- a/test/fixture.cc +++ b/test/fixture.cc @@ -83,9 +83,16 @@ MockWarningHandler::MockWarningHandler() { // Restores the previously active warning handler. MockWarningHandler::~MockWarningHandler() { active_handler = prev_; } -// Configures the mock warning handler to ignore all warnings. -void MockWarningHandler::ExpectWarnings() { - EXPECT_CALL(*this, Warn(_)).WillRepeatedly(Return()); +// Configures the mock warning handler to ignore all warnings (if empty) or +// expect at least one warning containing substring (if non-empty). +void MockWarningHandler::ExpectWarnings(std::string_view substring) { + if (substring.empty()) { + EXPECT_CALL(*this, Warn(_)).WillRepeatedly(Return()); + } else { + EXPECT_CALL(*this, Warn(::testing::HasSubstr(std::string(substring)))) + .Times(::testing::AtLeast(1)) + .WillRepeatedly(Return()); + } } // Returns the active warning handler. @@ -102,10 +109,12 @@ static mjfLogHandler prev_log_handler ABSL_GUARDED_BY(handlers_mutex) = nullptr; void default_mj_log_handler(const mjLogMessage* msg) { std::string subject = msg->subject; if (msg->func) { - subject = std::string(msg->func) + ": " + msg->subject; + subject = absl::StrCat(msg->func, ": ", msg->subject); } if (msg->level == mjLOG_ERROR) { + // legacy fallback: some tests still install mju_user_error to intercept + // errors with longjmp-based capture if (mju_user_error) { mju_user_error(subject.c_str()); } else { @@ -114,14 +123,12 @@ void default_mj_log_handler(const mjLogMessage* msg) { } else if (msg->level == mjLOG_WARNING) { std::string full_msg = subject; if (msg->body) { - full_msg += "\n" + std::string(msg->body); + absl::StrAppend(&full_msg, "\n", msg->body); } - if (mju_user_warning) { - mju_user_warning(full_msg.c_str()); - } else if (auto* handler = MockWarningHandler::GetActive()) { + if (auto* handler = MockWarningHandler::GetActive()) { handler->Warn(full_msg); } else { - ADD_FAILURE() << "mju_user_warning: " << full_msg; + ADD_FAILURE() << "Unexpected warning: " << full_msg; } } } diff --git a/test/fixture.h b/test/fixture.h index 805ab626..92b1e062 100644 --- a/test/fixture.h +++ b/test/fixture.h @@ -117,8 +117,9 @@ class MockWarningHandler { // Mock method called when a warning is intercepted. MOCK_METHOD(void, Warn, (const std::string& msg)); - // Allow any number of warnings without triggering test failure. - void ExpectWarnings(); + // Allow any number of warnings (if empty) or expect at least one warning + // containing the specified substring (if non-empty). + void ExpectWarnings(std::string_view substring = ""); // Returns the thread-local active mock warning handler. static MockWarningHandler* GetActive(); diff --git a/test/user/user_api_test.cc b/test/user/user_api_test.cc index 04e4a4ff..4b8b3ac0 100644 --- a/test/user/user_api_test.cc +++ b/test/user/user_api_test.cc @@ -24,9 +24,9 @@ #include #include +#include #include #include -#include "src/cc/array_safety.h" #include #include #include @@ -38,9 +38,8 @@ namespace mujoco { namespace { using ::testing::HasSubstr; -using ::testing::NotNull; using ::testing::IsNull; - +using ::testing::NotNull; // -------------------------- test model manipulation ------------------------- @@ -243,7 +242,8 @@ int open_mock(mjResource* resource) { )"; resource->data = mju_malloc(sizeof(parent_xml)); - std::strcpy((char*)resource->data, parent_xml); + absl::SNPrintF(static_cast(resource->data), sizeof(parent_xml), "%s", + parent_xml); return 1; } @@ -2668,6 +2668,12 @@ void AttachNestedKeyframe(bool compile) { // compile required before further attachment mjModel* m_child = compile ? mj_compile(child, 0) : nullptr; + // check warning is issued, empty for a compiled model + MockWarningHandler warning_handler; + if (!compile) { + warning_handler.ExpectWarnings("model has pending keyframes"); + } + // attach child to parent mjs_attach(mjs_findFrame(parent, "frame")->element, mjs_findBody(child, "body")->element, "child-", ""); @@ -2679,7 +2685,6 @@ void AttachNestedKeyframe(bool compile) { EXPECT_THAT(mjs_getWarning(parent, 0), HasSubstr("model has pending keyframes")); } - // compare models mjtNum tol = 0; std::string field = ""; diff --git a/test/user/user_mesh_test.cc b/test/user/user_mesh_test.cc index 9ea3a39a..328f9d87 100644 --- a/test/user/user_mesh_test.cc +++ b/test/user/user_mesh_test.cc @@ -30,7 +30,6 @@ #include #include #include -#include "src/cc/array_safety.h" #include "test/fixture.h" namespace mujoco { @@ -1310,11 +1309,8 @@ TEST_F(MjCMeshTest, NaNConvexHullDisallowed) { )"; - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); std::array error; mjModel* model = LoadModelFromString(xml, error.data(), error.size()); EXPECT_THAT(model, testing::IsNull()); diff --git a/test/user/user_resource_test.cc b/test/user/user_resource_test.cc index 4017c0ae..0da20b37 100644 --- a/test/user/user_resource_test.cc +++ b/test/user/user_resource_test.cc @@ -24,7 +24,7 @@ #include #include -#include "src/cc/array_safety.h" +#include #include #include #include "src/engine/engine_plugin.h" @@ -51,8 +51,9 @@ int open_str(mjResource* resource) { return 0; } - resource->data = mju_malloc(100*sizeof(char)); - std::strcpy((char*) resource->data, "Hello World"); + const std::size_t kBufferSize = 100; + resource->data = mju_malloc(kBufferSize * sizeof(char)); + absl::SNPrintF((char*) resource->data, kBufferSize, "Hello World"); return 1; } @@ -128,17 +129,10 @@ TEST_F(ResourceTest, RegisterProviderMissingCallbacks) { .prefix = "myprefix", }; - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings("callback"); int i = mjp_registerResourceProvider(&provider); - - // warning message related to missing callbacks - EXPECT_THAT(warning, HasSubstr("callback")); EXPECT_LT(i, 1); } @@ -150,17 +144,10 @@ TEST_F(ResourceTest, RegisterProviderMissingPrefix) { .close = close_nop, }; - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings("prefix"); int i = mjp_registerResourceProvider(&provider); - - // warning message related to missing prefix - EXPECT_THAT(warning, HasSubstr("prefix")); EXPECT_LT(i, 1); } @@ -172,17 +159,10 @@ TEST_F(ResourceTest, RegisterProviderInvalidPrefix1) { .close = close_nop, }; - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings("prefix"); int i = mjp_registerResourceProvider(&provider); - - // warning message related to missing prefix - EXPECT_THAT(warning, HasSubstr("prefix")); EXPECT_LT(i, 1); } @@ -194,17 +174,10 @@ TEST_F(ResourceTest, RegisterProviderInvalidPrefix2) { .close = close_nop, }; - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings("prefix"); int i = mjp_registerResourceProvider(&provider); - - // warning message related to missing prefix - EXPECT_THAT(warning, HasSubstr("prefix")); EXPECT_LT(i, 1); } @@ -316,13 +289,8 @@ TEST_F(ResourceTest, NameWithValidPrefix) { int i = mjp_registerResourceProvider(&provider); EXPECT_GT(i, 0); - - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); // open resource mjResource* resource = mju_openResource("", "nop:found", nullptr, nullptr, 0); @@ -342,13 +310,8 @@ TEST_F(ResourceTest, NameWithUpperCasePrefix) { int i = mjp_registerResourceProvider(&provider); EXPECT_GT(i, 0); - - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); // open resource mjResource* resource = mju_openResource("", "NOP:found", nullptr, nullptr, 0); @@ -368,13 +331,8 @@ TEST_F(ResourceTest, NameWithInvalidPrefix) { int i = mjp_registerResourceProvider(&provider); EXPECT_GT(i, 0); - - // install warning handler - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); // open resource mjResource* resource = mju_openResource("", "nopfound", nullptr, nullptr, 0); diff --git a/test/user/user_util_test.cc b/test/user/user_util_test.cc index 67c91e2b..9fc3f022 100644 --- a/test/user/user_util_test.cc +++ b/test/user/user_util_test.cc @@ -24,7 +24,6 @@ #include #include -#include #include "test/fixture.h" namespace mujoco { @@ -160,7 +159,8 @@ TEST_F(UserUtilTest, StringToVectorInvalidNumber) { } TEST_F(UserUtilTest, StringToVectorNan) { - mju_user_warning = nullptr; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings(); auto v = StringToVector("1 2 nan 3.21"); EXPECT_THAT(v[2], IsNan()); EXPECT_EQ(v[3], 3.21); diff --git a/test/xml/xml_native_reader_test.cc b/test/xml/xml_native_reader_test.cc index 0f840419..3f848ee9 100644 --- a/test/xml/xml_native_reader_test.cc +++ b/test/xml/xml_native_reader_test.cc @@ -28,7 +28,6 @@ #include #include #include "src/cc/array_safety.h" -#include "src/engine/engine_util_errmem.h" #include "src/user/user_api.h" #include "src/xml/xml_api.h" #include "test/compare_model.h" @@ -381,14 +380,10 @@ TEST_F(XMLReaderTest, CanParseNanAndRaisesWarning) { )"; std::array error; - static char warning[1024]; - warning[0] = '\0'; - mju_user_warning = [](const char* msg) { - util::strcpy_arr(warning, msg); - }; + MockWarningHandler warning_handler; + warning_handler.ExpectWarnings("XML contains a 'NaN'"); mjModel* model = LoadModelFromString(xml, error.data(), error.size()); ASSERT_THAT(model, NotNull()); - EXPECT_THAT(warning, HasSubstr("XML contains a 'NaN'")); EXPECT_THAT(model->geom_pos[0], IsNan()); EXPECT_THAT(model->geom_pos[1], IsNan()); EXPECT_THAT(model->geom_pos[2], IsNan());