mreddington commented on a change in pull request #804: URL: https://github.com/apache/geode-native/pull/804#discussion_r632073231
########## File path: cppcache/integration/test/CachingProxyTest.cpp ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <list> +#include <thread> + +#include <gtest/gtest.h> + +#include <geode/Cache.hpp> +#include <geode/CacheFactory.hpp> +#include <geode/RegionFactory.hpp> +#include <geode/RegionShortcut.hpp> +#include <geode/Serializable.hpp> + +#include "framework/Cluster.h" + +using apache::geode::client::Cache; +using apache::geode::client::Cacheable; +using apache::geode::client::CacheableString; +using apache::geode::client::CacheFactory; +using apache::geode::client::Region; +using apache::geode::client::RegionShortcut; +using apache::geode::client::Serializable; + +namespace CachingProxy { + +class CachingProxyTest : public ::testing::Test { Review comment: You're going to want something like this: ``` protected: static std::unique_ptr<Cluster> cluster_; static std::unique_ptr<Cache> cache_; static std::shared_ptr<Region> region_; public: static void SetUpTestSuite() { cluster_ = make_unique<Cluster>(Name{"CachingProxyTest"}, LocatorCount{1}, ServerCount{1}); cluster_->start(); cluster.getGfsh() .create() .region() .withName("region") .withType("PARTITION") .execute(); cache_ = make_unique<Cache>(cluster_->createCache()); cache_.getPoolManager() .createFactory() .addLocator("localhost", cluster_.getLocatorPort()) .create("pool"); region_ = cache_->createRegionFactory(CACHING_PROXY) .setPoolName("pool") .create("region"); } static void TearDownTestSuite() { region_.reset(); cache_.reset(); cluster_.reset(); } ``` Now your cluster is running and configured, your cache is configured, and your region is configured. All will be shared between every test scenario. It will be setup before any scenario is run, it is shutdown after all the scenarios are completed. And don't forget with statics, you need to define their initial state outside the class declaration. Since they're all smart pointers, this is easy: ``` }; // class CachingProxyTest std::unique_ptr<Cluster> CachingProxyTest::cluster_{}; std::unique_ptr<Cache> CachingProxyTest::cache_{}; std::shared_ptr<Region> CachingProxyTest::region_{}; ``` You can even do this with your test key and test value, and I would recommend you include the test region and test pool names. Don't rely on duplicating literals. You then only need to be sure to insert the initial value in the constructor, and clear the region in the destructor in preparation for the next test. You'll also be able to get rid of `SetUp` and `TearDown`, which (isn't always the case, but) would be redundant to the ctor and dtor. ########## File path: cppcache/integration/test/CachingProxyTest.cpp ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <list> +#include <thread> + +#include <gtest/gtest.h> + +#include <geode/Cache.hpp> +#include <geode/CacheFactory.hpp> +#include <geode/RegionFactory.hpp> +#include <geode/RegionShortcut.hpp> +#include <geode/Serializable.hpp> + +#include "framework/Cluster.h" + +using apache::geode::client::Cache; +using apache::geode::client::Cacheable; +using apache::geode::client::CacheableString; +using apache::geode::client::CacheFactory; +using apache::geode::client::Region; +using apache::geode::client::RegionShortcut; +using apache::geode::client::Serializable; + +namespace CachingProxy { + +class CachingProxyTest : public ::testing::Test { + protected: + CachingProxyTest() { + cluster.start(); + + cluster.getGfsh() + .create() + .region() + .withName("region") + .withType("PARTITION") + .execute(); + } + + ~CachingProxyTest() override = default; + + void SetUp() override { + cache.getPoolManager() + .createFactory() + .addLocator("localhost", cluster.getLocatorPort()) + .create("pool"); + + region = cache.createRegionFactory(RegionShortcut::CACHING_PROXY) + .setPoolName("pool") + .create("region"); + } + + void TearDown() override { cache.close(); } + + Cluster cluster = Cluster{LocatorCount{1}, ServerCount{1}}; + Cache cache = CacheFactory().create(); + std::string key = std::string("scharles"); + std::string value = std::string("Sylvia Charles"); + std::shared_ptr<Region> region; +}; + +TEST_F(CachingProxyTest, LocalRemoveAfterLocalInvalidate) { + region->put(key, value); + + auto user = region->get(key); + + region->localInvalidate(key); + + auto resultLocalRemove = region->localRemove(key, value); + ASSERT_FALSE(resultLocalRemove); Review comment: See? This is the confusing bit. Why are you asserting this? The line prior is test setup code. THAT IT WORKS is the foregone conclusion. The real test is the two lines below, that's it. So everything above goes into the test harness constructor. I recommend, after my learning experience in PR#805, that you keep the test harness hierarchy flat. I'd rather you duplicate the code and put all your configuration in the ctor so it's all in one place, easier to find, easier to read, and easier to understand. You can make a `CachingProxyWithLocallyInvalidatedKey` harness, and the test case can then be named `LocalRemove`. ########## File path: cppcache/integration/test/CachingProxyTest.cpp ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <list> +#include <thread> + +#include <gtest/gtest.h> + +#include <geode/Cache.hpp> +#include <geode/CacheFactory.hpp> +#include <geode/RegionFactory.hpp> +#include <geode/RegionShortcut.hpp> +#include <geode/Serializable.hpp> + +#include "framework/Cluster.h" + +using apache::geode::client::Cache; +using apache::geode::client::Cacheable; +using apache::geode::client::CacheableString; +using apache::geode::client::CacheFactory; +using apache::geode::client::Region; +using apache::geode::client::RegionShortcut; +using apache::geode::client::Serializable; + +namespace CachingProxy { + +class CachingProxyTest : public ::testing::Test { + protected: + CachingProxyTest() { + cluster.start(); + + cluster.getGfsh() + .create() + .region() + .withName("region") + .withType("PARTITION") + .execute(); + } + + ~CachingProxyTest() override = default; + + void SetUp() override { + cache.getPoolManager() + .createFactory() + .addLocator("localhost", cluster.getLocatorPort()) + .create("pool"); + + region = cache.createRegionFactory(RegionShortcut::CACHING_PROXY) + .setPoolName("pool") + .create("region"); + } + + void TearDown() override { cache.close(); } + + Cluster cluster = Cluster{LocatorCount{1}, ServerCount{1}}; + Cache cache = CacheFactory().create(); + std::string key = std::string("scharles"); + std::string value = std::string("Sylvia Charles"); + std::shared_ptr<Region> region; +}; + +TEST_F(CachingProxyTest, LocalRemoveAfterLocalInvalidate) { Review comment: With your test harness configuring once, that makes each test fast and cheap. That means you can write more tests. I would rather see: * zero setup in the test * all your expectations * ONE ACTION * all your assertions Your tests contain multiple actions and multiple assertions. This is a habit that is borne out of tests being expensive, so you're trying to avoid costs by cramming as much into one as possible. Now you don't have to do that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
