Skip to content
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

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

chakaz
Copy link
Collaborator

@chakaz chakaz commented Nov 5, 2023

This is a pretty recent regression.

@chakaz chakaz requested a review from kostasrim November 5, 2023 12:40
@romange
Copy link
Collaborator

romange commented Nov 5, 2023

What has changed? Why do we need to reset the service now with dbfilename=...?

@chakaz
Copy link
Collaborator Author

chakaz commented Nov 5, 2023

What has changed? Why do we need to reset the service now with dbfilename=...?

A change by some @romange 🤣

8a65aec

unlink(fl.name.c_str());
}
BaseFamilyTest::TearDown();
InitWithDbFilename();
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@@ -94,6 +94,9 @@ class BaseFamilyTest : public ::testing::Test {

void ShutdownService();

void InitWithDbFilename();
void DelSerializedFiles();
Copy link
Collaborator

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() ?

romange
romange previously approved these changes Nov 5, 2023
Copy link
Collaborator

@romange romange left a 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

@chakaz
Copy link
Collaborator Author

chakaz commented Nov 6, 2023

That would be a useful feature indeed...
But it's not too bad, we have a few functions that reset the service anyway

@chakaz chakaz merged commit efeae54 into main Nov 6, 2023
7 checks passed
@chakaz chakaz deleted the dbfilename-test branch November 6, 2023 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants