From de91c1f14b3e21aab4c1a69c53fb5feae0934614 Mon Sep 17 00:00:00 2001 From: shahar Date: Sun, 5 Nov 2023 14:34:36 +0200 Subject: [PATCH 1/2] fix(unittest): Init with `dbfilename=` before attempting to `save` This is a pretty recent regression. --- src/server/cluster/cluster_family_test.cc | 2 ++ src/server/generic_family_test.cc | 21 +++++++++------- src/server/rdb_test.cc | 18 +------------- src/server/test_utils.cc | 29 ++++++++++++++++++++++- src/server/test_utils.h | 3 +++ 5 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/server/cluster/cluster_family_test.cc b/src/server/cluster/cluster_family_test.cc index 6f5286809b3d..93166bec8896 100644 --- a/src/server/cluster/cluster_family_test.cc +++ b/src/server/cluster/cluster_family_test.cc @@ -680,6 +680,8 @@ TEST_F(ClusterFamilyTest, ClusterModeSelectNotAllowed) { } TEST_F(ClusterFamilyTest, ClusterFirstConfigCallDropsEntriesNotOwnedByNode) { + InitWithDbFilename(); + Run({"debug", "populate", "50000"}); EXPECT_EQ(Run({"save", "df"}), "OK"); diff --git a/src/server/generic_family_test.cc b/src/server/generic_family_test.cc index e3aade9a6cb2..b82dbaf7d279 100644 --- a/src/server/generic_family_test.cc +++ b/src/server/generic_family_test.cc @@ -633,41 +633,44 @@ TEST_F(GenericFamilyTest, Restore) { } TEST_F(GenericFamilyTest, Info) { + InitWithDbFilename(); // Needed for `save` + auto get_rdb_changes_since_last_save = [](const string& str) -> size_t { const string matcher = "rdb_changes_since_last_save:"; const auto pos = str.find(matcher) + matcher.size(); const auto sub = str.substr(pos, 1); return atoi(sub.c_str()); }; - auto resp = Run({"set", "k", "1"}); - resp = Run({"info", "persistence"}); + + EXPECT_EQ(Run({"set", "k", "1"}), "OK"); + auto resp = Run({"info", "persistence"}); EXPECT_EQ(1, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"set", "k", "1"}); + EXPECT_EQ(Run({"set", "k", "1"}), "OK"); resp = Run({"info", "persistence"}); EXPECT_EQ(2, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"set", "k2", "2"}); + EXPECT_EQ(Run({"set", "k2", "2"}), "OK"); resp = Run({"info", "persistence"}); EXPECT_EQ(3, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"save"}); + EXPECT_EQ(Run({"save"}), "OK"); resp = Run({"info", "persistence"}); EXPECT_EQ(0, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"set", "k2", "2"}); + EXPECT_EQ(Run({"set", "k2", "2"}), "OK"); resp = Run({"info", "persistence"}); EXPECT_EQ(1, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"bgsave"}); + EXPECT_EQ(Run({"bgsave"}), "OK"); resp = Run({"info", "persistence"}); EXPECT_EQ(0, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"set", "k3", "3"}); + EXPECT_EQ(Run({"set", "k3", "3"}), "OK"); resp = Run({"info", "persistence"}); EXPECT_EQ(1, get_rdb_changes_since_last_save(resp.GetString())); - resp = Run({"del", "k3"}); + EXPECT_THAT(Run({"del", "k3"}), IntArg(1)); resp = Run({"info", "persistence"}); EXPECT_EQ(1, get_rdb_changes_since_last_save(resp.GetString())); } diff --git a/src/server/rdb_test.cc b/src/server/rdb_test.cc index 2532279678d0..fb580f775f8e 100644 --- a/src/server/rdb_test.cc +++ b/src/server/rdb_test.cc @@ -18,7 +18,6 @@ extern "C" { #include "base/logging.h" #include "facade/facade_test.h" // needed to find operator== for RespExpr. #include "io/file.h" -#include "io/file_util.h" #include "server/engine_shard_set.h" #include "server/rdb_load.h" #include "server/test_utils.h" @@ -33,33 +32,18 @@ using absl::StrCat; ABSL_DECLARE_FLAG(int32, list_compress_depth); ABSL_DECLARE_FLAG(int32, list_max_listpack_size); ABSL_DECLARE_FLAG(int, compression_mode); -ABSL_DECLARE_FLAG(string, dbfilename); namespace dfly { class RdbTest : public BaseFamilyTest { protected: - void TearDown(); void SetUp(); io::FileSource GetSource(string name); }; void RdbTest::SetUp() { - SetFlag(&FLAGS_dbfilename, "rdbtestdump"); - BaseFamilyTest::SetUp(); -} - -void RdbTest::TearDown() { - // Disable save on shutdown - SetFlag(&FLAGS_dbfilename, ""); - - auto rdb_files = io::StatFiles("rdbtestdump*"); - CHECK(rdb_files); - for (const auto& fl : *rdb_files) { - unlink(fl.name.c_str()); - } - BaseFamilyTest::TearDown(); + InitWithDbFilename(); } inline const uint8_t* to_byte(const void* s) { diff --git a/src/server/test_utils.cc b/src/server/test_utils.cc index bf2e3c4e1c2f..e6bbe2293280 100644 --- a/src/server/test_utils.cc +++ b/src/server/test_utils.cc @@ -19,6 +19,7 @@ extern "C" { #include "base/logging.h" #include "base/stl_util.h" #include "facade/dragonfly_connection.h" +#include "io/file_util.h" #include "server/acl/acl_log.h" #include "util/fibers/pool.h" @@ -257,7 +258,13 @@ void BaseFamilyTest::ResetService() { } void BaseFamilyTest::ShutdownService() { - DCHECK(service_); + if (service_ == nullptr) { + return; + } + + // Don't save files during shutdown + DelSerializedFiles(); + absl::SetFlag(&FLAGS_dbfilename, ""); service_->Shutdown(); service_.reset(); @@ -270,6 +277,26 @@ void BaseFamilyTest::ShutdownService() { pp_->Stop(); } +void BaseFamilyTest::InitWithDbFilename() { + ShutdownService(); + + absl::SetFlag(&FLAGS_dbfilename, "rdbtestdump"); + DelSerializedFiles(); + ResetService(); +} + +void BaseFamilyTest::DelSerializedFiles() { + string dbfilename = absl::GetFlag(FLAGS_dbfilename); + if (dbfilename.empty()) + return; + + auto rdb_files = io::StatFiles(absl::StrCat(dbfilename, "*")); + CHECK(rdb_files); + for (const auto& fl : *rdb_files) { + unlink(fl.name.c_str()); + } +} + unsigned BaseFamilyTest::NumLocked() { atomic_uint count = 0; shard_set->RunBriefInParallel([&](EngineShard* shard) { diff --git a/src/server/test_utils.h b/src/server/test_utils.h index 797e4ed4926b..d10a42a1e65b 100644 --- a/src/server/test_utils.h +++ b/src/server/test_utils.h @@ -94,6 +94,9 @@ class BaseFamilyTest : public ::testing::Test { void ShutdownService(); + void InitWithDbFilename(); + void DelSerializedFiles(); + bool IsLocked(DbIndex db_index, std::string_view key) const; ConnectionContext::DebugInfo GetDebugInfo(const std::string& id) const; From 9fd6dd48efa933198c8d612895d38f1ee37775b1 Mon Sep 17 00:00:00 2001 From: shahar Date: Sun, 5 Nov 2023 19:34:31 +0200 Subject: [PATCH 2/2] Rename --- src/server/test_utils.cc | 6 +++--- src/server/test_utils.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/server/test_utils.cc b/src/server/test_utils.cc index e6bbe2293280..db1992a0aa93 100644 --- a/src/server/test_utils.cc +++ b/src/server/test_utils.cc @@ -263,7 +263,7 @@ void BaseFamilyTest::ShutdownService() { } // Don't save files during shutdown - DelSerializedFiles(); + CleanupSnapshots(); absl::SetFlag(&FLAGS_dbfilename, ""); service_->Shutdown(); @@ -281,11 +281,11 @@ void BaseFamilyTest::InitWithDbFilename() { ShutdownService(); absl::SetFlag(&FLAGS_dbfilename, "rdbtestdump"); - DelSerializedFiles(); + CleanupSnapshots(); ResetService(); } -void BaseFamilyTest::DelSerializedFiles() { +void BaseFamilyTest::CleanupSnapshots() { string dbfilename = absl::GetFlag(FLAGS_dbfilename); if (dbfilename.empty()) return; diff --git a/src/server/test_utils.h b/src/server/test_utils.h index d10a42a1e65b..59ee1a7e8532 100644 --- a/src/server/test_utils.h +++ b/src/server/test_utils.h @@ -95,7 +95,7 @@ class BaseFamilyTest : public ::testing::Test { void ShutdownService(); void InitWithDbFilename(); - void DelSerializedFiles(); + void CleanupSnapshots(); bool IsLocked(DbIndex db_index, std::string_view key) const; ConnectionContext::DebugInfo GetDebugInfo(const std::string& id) const;