Avoid memory leak in unit test driver #4249
Conversation
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.
|
Scratch "gets selective valgrind runs of our unit tests clean for me" - I'm still seeing something else, and I'm not sure if it's just something I missed before or a regression from this PR. |
| 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) |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
|
There's definitely something weird going on here. This patch cleans up the leaked suite from If every test class was complaining then I'd be sure I'm trying to do a double-free here, once from the runner and then once from recursive destruction from the suite, but it's just No, wait, it's too much of a coincidence that the failing |
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. (all-tests runs are clean either way)
This isn't urgent to merge; I still need to run through our examples to look for valgrind issues too. This tiny leak is only a problem because it upsets valgrind, and until we're sure we're ready to make that be a big deal (add a
--error-exitcode=option to our valgrind recipes), "do our unit tests upset valgrind" isn't a critical question.