From 9a61447a87eb0717dd073e731edde3611956efb8 Mon Sep 17 00:00:00 2001 From: Roy Stogner Date: Wed, 10 Sep 2025 08:42:35 -0500 Subject: [PATCH 1/2] Avoid trivial memory leak in unit test driver If we add only a subset of tests to the runner (via --re or --deny_re options), we're careful to move the other tests to a "rejects" suite so they'll get deleted there, but the "supertest", the suite holding all the other tests, wasn't getting deleted in those cases. This (on top of my earlier fixes, and using the MOOSE suppressions file for third-party library issues) gets selective valgrind runs of our unit tests clean for me. --- tests/driver.C | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/driver.C b/tests/driver.C index 773302843bd..18de781c474 100644 --- a/tests/driver.C +++ b/tests/driver.C @@ -116,11 +116,17 @@ int main(int argc, char ** argv) std::string deny_regex_string = "^$"; deny_regex_string = libMesh::command_line_next("--deny_re", deny_regex_string); + // We might have to delete the test suite ourselves, after the + // runner has deleted whatever subtests it has. + std::unique_ptr owned_suite; + // Recursively add tests matching the regex to the runner object. CppUnit::TextUi::TestRunner runner; - // The Cppunit registry object that knows about all the tests. + // The Cppunit registry object that knows about all the tests, and + // the test suite it creates. CppUnit::TestFactoryRegistry & registry = CppUnit::TestFactoryRegistry::getRegistry(); + CppUnit::Test * suite = registry.makeTest(); // A test suite container for holding tests not matching the regex. When main's // scope ends, this class's destructor will delete the rejected tests @@ -134,15 +140,17 @@ int main(int argc, char ** argv) // Add all tests which match the re to the runner object. libMesh::out << "Will run the following tests:" << std::endl; const int n_tests_added = - add_matching_tests_to_runner(registry.makeTest(), + add_matching_tests_to_runner(suite, allow_regex_string, allow_regex, deny_regex_string, deny_regex, runner, rejects); if (n_tests_added >= 0) libMesh::out << "--- Running " << n_tests_added << " tests in total." << std::endl; + if (n_tests_added != -12345) + owned_suite.reset(suite); #else // If no C++11 just run all the tests. - runner.addTest(registry.makeTest()); + runner.addTest(suite); #endif std::unique_ptr controller; From 0d0d8d150c2541a9bb7a0e7e3ed052d5752178f3 Mon Sep 17 00:00:00 2001 From: Roy Stogner Date: Mon, 24 Nov 2025 16:07:48 -0600 Subject: [PATCH 2/2] TestShim to ensure every test is destructed once --- tests/driver.C | 60 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/tests/driver.C b/tests/driver.C index 18de781c474..31a856d91e0 100644 --- a/tests/driver.C +++ b/tests/driver.C @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -17,14 +18,51 @@ // C++ includes #include +// We want to be able to add a selection of tests from our full suite +// to our test runner. But then the test runner will want to destruct +// those tests afterward, so we can't destruct the suite without it +// *also* trying to destruct all its tests (undefined behavior), and +// we can't not destruct the suite without the suite and its +// unselected tests being leaked (which would be fine since just exit +// afterward, except valgrind sees the leaks and screams). +// +// So, instead of adding selected tests, we add shims of selected +// tests that can be deleted without deleting the tests they shim. +class TestShim : public CppUnit::Test +{ +public: + TestShim (CppUnit::Test & shimmed_test) : _shimmed_test(shimmed_test) {} + + virtual void run(CppUnit::TestResult * result) override { _shimmed_test.run(result); } + + virtual int countTestCases () const override { return _shimmed_test.countTestCases(); } + + virtual int getChildTestCount () const override { return _shimmed_test.getChildTestCount(); } + + virtual std::string getName () const override { return _shimmed_test.getName(); } + + virtual bool findTestPath (const std::string & testName, CppUnit::TestPath & testPath) const override { return _shimmed_test.findTestPath(testName, testPath); } + + virtual bool findTestPath (const CppUnit::Test * test, CppUnit::TestPath & testPath) const override { return _shimmed_test.findTestPath(test, testPath); } + + virtual CppUnit::Test * findTest(const std::string & testName) const override { return _shimmed_test.findTest(testName); } + + virtual CppUnit::TestPath resolveTestPath(const std::string & testPath) const override { return _shimmed_test.resolveTestPath(testPath); } + +protected: + virtual CppUnit::Test * doGetChildTestAt(int index) const override { return _shimmed_test.getChildTestAt(index); } + +private: + CppUnit::Test & _shimmed_test; +}; + // Add Tests to runner that match user-provided regex. int add_matching_tests_to_runner(CppUnit::Test * test, const std::string & allow_r_str, const std::regex & allow_r, const std::string & deny_r_str, const std::regex & deny_r, - CppUnit::TextUi::TestRunner & runner, - CppUnit::TestSuite & rejects) + CppUnit::TextUi::TestRunner & runner) { int n_tests_added = 0; @@ -46,18 +84,17 @@ int add_matching_tests_to_runner(CppUnit::Test * test, { libMesh::out << test->getName() << std::endl; n_tests_added ++; - runner.addTest(test); + + // yes, explicit new; this is how CppUnit works + runner.addTest(new TestShim(*test)); } - // Add the test to the rejects it can be cleaned up later - else - rejects.addTest(test); } // Call this recursively on each of our children, if any. for (int i = 0; i < test->getChildTestCount(); i++) n_tests_added += add_matching_tests_to_runner(test->getChildTestAt(i), allow_r_str, allow_r, - deny_r_str, deny_r, runner, rejects); + deny_r_str, deny_r, runner); return n_tests_added; } @@ -128,10 +165,6 @@ int main(int argc, char ** argv) CppUnit::TestFactoryRegistry & registry = CppUnit::TestFactoryRegistry::getRegistry(); CppUnit::Test * suite = registry.makeTest(); - // A test suite container for holding tests not matching the regex. When main's - // scope ends, this class's destructor will delete the rejected tests - CppUnit::TestSuite rejects("rejects"); - #ifdef LIBMESH_HAVE_CXX11_REGEX // Make regex objects from user's input. const std::regex allow_regex(allow_regex_string); @@ -143,9 +176,12 @@ int main(int argc, char ** argv) add_matching_tests_to_runner(suite, allow_regex_string, allow_regex, deny_regex_string, deny_regex, - runner, rejects); + runner); if (n_tests_added >= 0) libMesh::out << "--- Running " << n_tests_added << " tests in total." << std::endl; + + // If we didn't add the whole suite to the runner, we need to clean + // it up ourselves if (n_tests_added != -12345) owned_suite.reset(suite); #else