Skip to content

Avoid memory leak in unit test driver #4249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

Avoid memory leak in unit test driver #4249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions tests/driver.C
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CppUnit::Test> 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
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, but since this is our own magic number that we now actually have to refer to, it might make sense to use libMesh::invalid_uint or some other named constant.

owned_suite.reset(suite);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't understand why we don't need to clean up suite when there's no tests added? From a surface level reading of the code it just looks like registry.makeTest() returns a dumb pointer whose lifetime we are expected to manage, and we were always leaking it before...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should clean up suite when there's no tests added. That'll return n_tests_added == 0, and 0 != -12345, so we put suite in our unique_ptr and it gets cleaned up.

#else
// If no C++11 <regex> just run all the tests.
runner.addTest(registry.makeTest());
runner.addTest(suite);
#endif

std::unique_ptr<CppUnit::TestResult> controller;
Expand Down