From 5d6ff693750ccfb63bd03ea2183ac66aab6833aa Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 23 Sep 2020 09:32:17 -0700 Subject: [PATCH] Fix valgrind issues with configurable_test (#7424) Summary: Valgrind was reporting a problem with the configurable_test in some GTEST code. This problem was caused by using a std::function as a GTEST parameter. This change changes the test to use a string as a function parameter (backed by a map) and fixes the valgrind issue. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7424 Reviewed By: ajkr Differential Revision: D23855540 Pulled By: pdillinger fbshipit-source-id: 2f2be03f7f92d96644aa9fa6481e4f37f2cfa5f5 --- options/configurable_test.cc | 246 ++++++++++++++++++----------------- 1 file changed, 125 insertions(+), 121 deletions(-) diff --git a/options/configurable_test.cc b/options/configurable_test.cc index b37fa9066..27f877526 100644 --- a/options/configurable_test.cc +++ b/options/configurable_test.cc @@ -111,22 +111,6 @@ class ConfigurableTest : public testing::Test { ConfigOptions config_options_; }; -class ConfigurableParamTest - : public ConfigurableTest, - virtual public ::testing::WithParamInterface< - std::pair> { - public: - ConfigurableParamTest() { - configuration_ = GetParam().first; - factory_ = GetParam().second; - object_.reset(factory_()); - } - void TestConfigureOptions(const ConfigOptions& opts); - ConfigTestFactoryFunc factory_; - std::string configuration_; - std::unique_ptr object_; -}; - TEST_F(ConfigurableTest, GetOptionsPtrTest) { std::string opt_str; std::unique_ptr configurable(SimpleConfigurable::Create()); @@ -598,14 +582,101 @@ TEST_F(ConfigurableTest, TestNoCompare) { } #endif +static std::unordered_map TestFactories = { + {"Simple", []() { return SimpleConfigurable::Create("simple"); }}, + {"Struct", []() { return SimpleStructFactory(); }}, + {"Unique", + []() { + return SimpleConfigurable::Create( + "simple", TestConfigMode::kSimpleMode | TestConfigMode::kUniqueMode); + }}, + {"Shared", + []() { + return SimpleConfigurable::Create( + "simple", TestConfigMode::kSimpleMode | TestConfigMode::kSharedMode); + }}, + {"Nested", + []() { + return SimpleConfigurable::Create( + "simple", TestConfigMode::kSimpleMode | TestConfigMode::kNestedMode); + }}, + {"Mutable", + []() { + return SimpleConfigurable::Create("simple", + TestConfigMode::kMutableMode | + TestConfigMode::kSimpleMode | + TestConfigMode::kNestedMode); + }}, + {"ThreeWay", + []() { + std::shared_ptr child; + child.reset( + SimpleConfigurable::Create("child", TestConfigMode::kDefaultMode)); + std::shared_ptr parent; + parent.reset(new WrappedConfigurable( + "parent", TestConfigMode::kDefaultMode, child)); + return new WrappedConfigurable("master", TestConfigMode::kDefaultMode, + parent); + }}, + {"ThreeDeep", + []() { + Configurable* simple = SimpleConfigurable::Create( + "Simple", + TestConfigMode::kUniqueMode | TestConfigMode::kDefaultMode); + auto* unique = + simple->GetOptions>("SimpleUnique"); + unique->reset(SimpleConfigurable::Create( + "Child", + TestConfigMode::kUniqueMode | TestConfigMode::kDefaultMode)); + unique = unique->get()->GetOptions>( + "ChildUnique"); + unique->reset( + SimpleConfigurable::Create("Child", TestConfigMode::kDefaultMode)); + return simple; + }}, + {"DBOptions", + []() { + auto config = DBOptionsAsConfigurable(DBOptions()); + return config.release(); + }}, + {"CFOptions", + []() { + auto config = CFOptionsAsConfigurable(ColumnFamilyOptions()); + return config.release(); + }}, + {"BlockBased", []() { return NewBlockBasedTableFactory(); }}, +}; + +class ConfigurableParamTest : public ConfigurableTest, + virtual public ::testing::WithParamInterface< + std::pair> { + public: + ConfigurableParamTest() { + type_ = GetParam().first; + configuration_ = GetParam().second; + assert(TestFactories.find(type_) != TestFactories.end()); + object_.reset(CreateConfigurable()); + } + + Configurable* CreateConfigurable() { + const auto& iter = TestFactories.find(type_); + return (iter->second)(); + } + + void TestConfigureOptions(const ConfigOptions& opts); + std::string type_; + std::string configuration_; + std::unique_ptr object_; +}; + void ConfigurableParamTest::TestConfigureOptions( const ConfigOptions& config_options) { std::unique_ptr base, copy; std::unordered_set names; std::string opt_str, mismatch; - base.reset(factory_()); - copy.reset(factory_()); + base.reset(CreateConfigurable()); + copy.reset(CreateConfigurable()); ASSERT_OK(base->ConfigureFromString(config_options, configuration_)); ASSERT_OK(base->GetOptionString(config_options, &opt_str)); @@ -613,7 +684,7 @@ void ConfigurableParamTest::TestConfigureOptions( ASSERT_OK(copy->GetOptionString(config_options, &opt_str)); ASSERT_TRUE(base->AreEquivalent(config_options, copy.get(), &mismatch)); - copy.reset(factory_()); + copy.reset(CreateConfigurable()); ASSERT_OK(base->GetOptionNames(config_options, &names)); std::unordered_map unused; bool found_one = false; @@ -655,7 +726,7 @@ TEST_P(ConfigurableParamTest, GetDefaultOptionsTest) { TEST_P(ConfigurableParamTest, ConfigureFromPropsTest) { std::string opt_str, mismatch; std::unordered_set names; - std::unique_ptr copy(factory_()); + std::unique_ptr copy(CreateConfigurable()); ASSERT_OK(object_->ConfigureFromString(config_options_, configuration_)); config_options_.delimiter = "\n"; @@ -674,110 +745,43 @@ TEST_P(ConfigurableParamTest, ConfigureFromPropsTest) { ASSERT_TRUE(object_->AreEquivalent(config_options_, copy.get(), &mismatch)); } -static Configurable* SimpleFactory() { - return SimpleConfigurable::Create("simple"); -} - -static Configurable* UniqueFactory() { - return SimpleConfigurable::Create( - "simple", TestConfigMode::kSimpleMode | TestConfigMode::kUniqueMode); -} -static Configurable* SharedFactory() { - return SimpleConfigurable::Create( - "simple", TestConfigMode::kSimpleMode | TestConfigMode::kSharedMode); -} - -static Configurable* NestedFactory() { - return SimpleConfigurable::Create( - "simple", TestConfigMode::kSimpleMode | TestConfigMode::kNestedMode); -} - -static Configurable* MutableFactory() { - return SimpleConfigurable::Create("simple", TestConfigMode::kMutableMode | - TestConfigMode::kSimpleMode | - TestConfigMode::kNestedMode); -} - -static Configurable* ThreeWrappedFactory() { - std::shared_ptr child; - child.reset( - SimpleConfigurable::Create("child", TestConfigMode::kDefaultMode)); - std::shared_ptr parent; - parent.reset( - new WrappedConfigurable("parent", TestConfigMode::kDefaultMode, child)); - return new WrappedConfigurable("master", TestConfigMode::kDefaultMode, - parent); -} - -static Configurable* ThreeDeepFactory() { - Configurable* simple = SimpleConfigurable::Create( - "Simple", TestConfigMode::kUniqueMode | TestConfigMode::kDefaultMode); - auto* unique = - simple->GetOptions>("SimpleUnique"); - unique->reset(SimpleConfigurable::Create( - "Child", TestConfigMode::kUniqueMode | TestConfigMode::kDefaultMode)); - unique = - unique->get()->GetOptions>("ChildUnique"); - unique->reset( - SimpleConfigurable::Create("Child", TestConfigMode::kDefaultMode)); - return simple; -} - -static Configurable* DBOptionsFactory() { - auto config = DBOptionsAsConfigurable(DBOptions()); - return config.release(); -} - -static Configurable* CFOptionsFactory() { - auto config = CFOptionsAsConfigurable(ColumnFamilyOptions()); - return config.release(); -} - -static Configurable* BlockBasedFactory() { return NewBlockBasedTableFactory(); } - INSTANTIATE_TEST_CASE_P( ParamTest, ConfigurableParamTest, testing::Values( - std::pair( - "int=42;bool=true;string=s", SimpleFactory), - std::pair( - "int=42;unique={int=33;string=unique}", MutableFactory), - std::pair( - "struct={int=33;bool=true;string=s;}", SimpleStructFactory), - std::pair( - "int=33;bool=true;string=outer;" - "shared={int=42;string=shared}", - SharedFactory), - std::pair( - "int=33;bool=true;string=outer;" - "unique={int=42;string=unique}", - UniqueFactory), - std::pair( - "int=11;bool=true;string=outer;" - "pointer={int=22;string=pointer};" - "unique={int=33;string=unique};" - "shared={int=44;string=shared}", - NestedFactory), - std::pair( - "int=11;bool=true;string=outer;" - "inner={int=22;string=parent;" - "inner={int=33;string=child}};", - ThreeWrappedFactory), - std::pair( - "int=11;bool=true;string=outer;" - "unique={int=22;string=inner;" - "unique={int=33;string=unique}};", - ThreeDeepFactory), - std::pair("max_background_jobs=100;" - "max_open_files=200;", - DBOptionsFactory), - std::pair( - "table_factory=BlockBasedTable;" - "disable_auto_compactions=true;", - CFOptionsFactory), - std::pair("block_size=1024;" - "no_block_cache=true;", - BlockBasedFactory))); + std::pair("Simple", + "int=42;bool=true;string=s"), + std::pair( + "Mutable", "int=42;unique={int=33;string=unique}"), + std::pair( + "Struct", "struct={int=33;bool=true;string=s;}"), + std::pair("Shared", + "int=33;bool=true;string=outer;" + "shared={int=42;string=shared}"), + std::pair("Unique", + "int=33;bool=true;string=outer;" + "unique={int=42;string=unique}"), + std::pair("Nested", + "int=11;bool=true;string=outer;" + "pointer={int=22;string=pointer};" + "unique={int=33;string=unique};" + "shared={int=44;string=shared}"), + std::pair("ThreeWay", + "int=11;bool=true;string=outer;" + "inner={int=22;string=parent;" + "inner={int=33;string=child}};"), + std::pair("ThreeDeep", + "int=11;bool=true;string=outer;" + "unique={int=22;string=inner;" + "unique={int=33;string=unique}};"), + std::pair("DBOptions", + "max_background_jobs=100;" + "max_open_files=200;"), + std::pair("CFOptions", + "table_factory=BlockBasedTable;" + "disable_auto_compactions=true;"), + std::pair("BlockBased", + "block_size=1024;" + "no_block_cache=true;"))); #endif // ROCKSDB_LITE } // namespace test -- GitLab