-
Notifications
You must be signed in to change notification settings - Fork 990
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.
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
fix(unittest): Init with dbfilename=
before attempting to save
#2127
Conversation
This is a pretty recent regression.
What has changed? Why do we need to reset the service now with |
unlink(fl.name.c_str()); | ||
} | ||
BaseFamilyTest::TearDown(); | ||
InitWithDbFilename(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand now the context but I think for these tests SetFlag(&FLAGS_dbfilename, "rdbtestdump");
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if not, that's a good infra to have, which already semi exists for rdb_test
, I think it's better to keep it for future use as well
src/server/test_utils.h
Outdated
@@ -94,6 +94,9 @@ class BaseFamilyTest : public ::testing::Test { | |||
|
|||
void ShutdownService(); | |||
|
|||
void InitWithDbFilename(); | |||
void DelSerializedFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe rename it to CleanupSnapshots()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish GTest would have the way to provide per test, presetup customizations. Unfortunately they do not have it, so we need to restart everything within the test. One way to solve this would be to implement a SaveTest
class that derives from BaseFamilyTest
That would be a useful feature indeed... |
This is a pretty recent regression.