Clean up some of the files left after test runs #9539
Conversation
|
Thanks @mrambacher for the PR. Furthermore, it would be useful to use |
At a certain point, I gave up trying to clean up the directories. Most of the stuff that is still around is because we do a DestroyDB rather than a DeleteDir and some part of the test created some file that Destroy does not touch. Some of these are things like "repair directories" or "backups" and I did not know if Destroy is broken in not removing them or something else.
While I agree with you that it might be nice to do a KEEP_DB for this effort, there are a lot of caveats with that. First, the KEEP_DB only will work if you are running a single test and not a series of them (parallel tests run single tests). Each test will use the same database directory. What would be even better is if the test name was the database directory and we unified a test::Destroy methodology to delete what is in its directory (which could be a DestroyDB with a message if there is other stuff present). |
| @@ -1150,6 +1150,7 @@ TEST_F(DBBasicTest, DBClose) { | |||
| ASSERT_EQ(env->GetCloseCount(), 2); | |||
| options.info_log.reset(); | |||
| ASSERT_EQ(env->GetCloseCount(), 3); | |||
| ASSERT_OK(DestroyDB(dbname, options)); | |||
There was a problem hiding this comment.
Can we use dbname_ in this test and let ~DBTestBase() take care of file deletion?
There was a problem hiding this comment.
Theoretically it could but if you look at this test it has its own dbname and db member variable and does not use the corresponding class names or methods. I think the test should either be changed to use dbname_/db_ everywhere or left as-is at the moment. If you wish, I will add a "TODO" comment to get rid of the use of the local variables in favor of the class ones.
There was a problem hiding this comment.
In the entire file db_basic_test.cc, I think only this test uses its own local dbname and db instead of dbname_ and db_. This indicates this test was not written properly. Given that the fix is simple, I would fix the test first instead of patching such a test.
There was a problem hiding this comment.
While I understand the intention of minimizing the changes introduced by this PR, I think it's a case-by-case decision.
When making changes like this PR, if we encounter a (complex) bug or other issue which affects RocksDB library, it makes sense to address the issue in a separate PR and keep this PR simple. In this case,
- it's all unit test code, and
- Fix is simple and can potentially improve test code quality.
Therefore, I would suggest fixing it since it's a good opportunity.
There was a problem hiding this comment.
In the entire file db_basic_test.cc, I think only this test uses its own local
dbnameanddbinstead ofdbname_anddb_. This indicates this test was not written properly. Given that the fix is simple, I would fix the test first instead of patching such a test.
This test uses a custom env (TestEnv) that the others do not use. This makes it more problematic to get it so that the test can use dbname_ and the default destructor code.
It's useful in the case of single test for the purpose of debugging. |
| // Delete the external files | ||
| for (const auto& e : external) { | ||
| Status s = options.env->DeleteFile(e); | ||
| ASSERT_TRUE(s.IsNotFound() || s.ok()); |
There was a problem hiding this comment.
When will s be non-ok?
There was a problem hiding this comment.
If the file does not exist, the I presume s.IsNotFound would be returned. I do not know if that would ever happen or not.
This change attempts to clean up some of the test/temporary files left around after running the tests. This was discovered when attempting to run in a smaller (ram disk) TEST_TMPDIR.
Many test would destroy the DB on start but not on exit (DBTestBase does this). With this change, the DB directory is also deleted/destroyed on test completion.
This PR does not remove ALL of the files left after a test run but gets most of them. There are about 20-ish files/directories left around after this change (prior there were 65+ directories, some of them quite large).
Note that running tests via make check (and comparable commands) do not leave stuff around by explicitly deleting the TMPD directory after the command is complete. This is an issue when running tests by hand/individually (and possibly when running tests via ctest?)