[805] Get rid of multiple inheritance

By having the TestSocketRequestor as member.

src/bin/auth/tests/auth_srv_unittest.cc

@@ -65,13 +65,13 @@ const char* const CONFIG_TESTDB =
 const char* const BADCONFIG_TESTDB =
     "{ \"database_file\": \"" TEST_DATA_DIR "/nodir/notexist\"}";
 
-class AuthSrvTest : public SrvTestBase, public TestSocketRequestor {
+class AuthSrvTest : public SrvTestBase {
 protected:
     AuthSrvTest() :
-        TestSocketRequestor(dnss_, address_store_, 53210),
         dnss_(ios_, NULL, NULL, NULL),
         server(true, xfrout),
-        rrclass(RRClass::IN())
+        rrclass(RRClass::IN()),
+        sock_requestor_(dnss_, address_store_, 53210)
     {
         server.setDNSService(dnss_);
         server.setXfrinSession(&notify_session);
@@ -89,6 +89,7 @@ protected:
     const RRClass rrclass;
     vector<uint8_t> response_data;
     AddressList address_store_;
+    TestSocketRequestor sock_requestor_;
 };
 
 // A helper function that builds a response to version.bind/TXT/CH that
@@ -899,10 +900,12 @@ TEST_F(AuthSrvTest, listenAddresses) {
         "UDP:::1:53210:4",
         NULL
     };
-    checkTokens(tokens, given_tokens_, "Given tokens");
+    sock_requestor_.checkTokens(tokens, sock_requestor_.given_tokens_,
+                                "Given tokens");
     // It returns back to empty set of addresses afterwards, so
     // they should be released
-    checkTokens(tokens, released_tokens_, "Released tokens");
+    sock_requestor_.checkTokens(tokens, sock_requestor_.released_tokens_,
+                                "Released tokens");
 }
 
 }

src/bin/auth/tests/config_unittest.cc

@@ -40,13 +40,13 @@ using namespace isc::asiodns;
 using namespace isc::asiolink;
 
 namespace {
-class AuthConfigTest : public isc::testutils::TestSocketRequestor {
+class AuthConfigTest : public ::testing::Test {
 protected:
     AuthConfigTest() :
-        TestSocketRequestor(dnss_, address_store_, 53210),
         dnss_(ios_, NULL, NULL, NULL),
         rrclass(RRClass::IN()),
-        server(true, xfrout)
+        server(true, xfrout),
+        sock_requestor_(dnss_, address_store_, 53210)
     {
         server.setDNSService(dnss_);
     }
@@ -56,6 +56,8 @@ protected:
     MockXfroutClient xfrout;
     AuthSrv server;
     isc::server_common::portconfig::AddressList address_store_;
+private:
+    isc::testutils::TestSocketRequestor sock_requestor_;
 };
 
 TEST_F(AuthConfigTest, datasourceConfig) {

src/bin/resolver/tests/resolver_config_unittest.cc

@@ -53,7 +53,7 @@ using namespace isc::server_common;
 using isc::UnitTestUtil;
 
 namespace {
-class ResolverConfig : public TestSocketRequestor {
+class ResolverConfig : public ::testing::Test {
 protected:
     IOService ios;
     DNSService dnss;
@@ -63,8 +63,8 @@ protected:
     scoped_ptr<const Client> client;
     scoped_ptr<const RequestContext> request;
     ResolverConfig() :
-        TestSocketRequestor(dnss, address_store_, 53210),
-        dnss(ios, NULL, NULL, NULL)
+        dnss(ios, NULL, NULL, NULL),
+        sock_requestor_(dnss, address_store_, 53210)
     {
         server.setDNSService(dnss);
         server.setConfigured();
@@ -82,6 +82,7 @@ protected:
     }
     void invalidTest(const string &JSON, const string& name);
     isc::server_common::portconfig::AddressList address_store_;
+    isc::testutils::TestSocketRequestor sock_requestor_;
 };
 
 TEST_F(ResolverConfig, forwardAddresses) {
@@ -201,10 +202,12 @@ TEST_F(ResolverConfig, listenAddresses) {
         "UDP:::1:53210:4",
         NULL
     };
-    checkTokens(tokens, given_tokens_, "Given tokens");
+    sock_requestor_.checkTokens(tokens, sock_requestor_.given_tokens_,
+                                "Given tokens");
     // It returns back to empty set of addresses afterwards, so
     // they should be released
-    checkTokens(tokens, released_tokens_, "Released tokens");
+    sock_requestor_.checkTokens(tokens, sock_requestor_.released_tokens_,
+                                "Released tokens");
 }
 
 // Try setting some addresses and a rollback

src/lib/server_common/tests/portconfig_unittest.cc

@@ -130,10 +130,10 @@ TEST_F(ParseAddresses, invalid) {
 }
 
 // Test fixture for installListenAddresses
-struct InstallListenAddresses : public testutils::TestSocketRequestor {
+struct InstallListenAddresses : public ::testing::Test {
     InstallListenAddresses() :
-        // The members aren't initialized yet, but we need to store refs only
-        TestSocketRequestor(dnss_, store_, 5288), dnss_(ios_, NULL, NULL, NULL)
+        dnss_(ios_, NULL, NULL, NULL),
+        sock_requestor_(dnss_, store_, 5288)
     {
         valid_.push_back(AddressPair("127.0.0.1", 5288));
         valid_.push_back(AddressPair("::1", 5288));
@@ -143,6 +143,7 @@ struct InstallListenAddresses : public testutils::TestSocketRequestor {
     IOService ios_;
     DNSService dnss_;
     AddressList store_;
+    isc::testutils::TestSocketRequestor sock_requestor_;
     // We should be able to bind to these addresses
     AddressList valid_;
     // But this shouldn't work
@@ -177,15 +178,19 @@ TEST_F(InstallListenAddresses, valid) {
         NULL
     };
     const char* no_tokens[] = { NULL };
-    checkTokens(tokens1, given_tokens_, "Valid given tokens 1");
-    checkTokens(no_tokens, released_tokens_, "Valid no released tokens 1");
+    sock_requestor_.checkTokens(tokens1, sock_requestor_.given_tokens_,
+                                "Valid given tokens 1");
+    sock_requestor_.checkTokens(no_tokens, sock_requestor_.released_tokens_,
+                                "Valid no released tokens 1");
     // TODO Maybe some test to actually connect to them
     // Try setting it back to nothing
-    given_tokens_.clear();
+    sock_requestor_.given_tokens_.clear();
     EXPECT_NO_THROW(installListenAddresses(AddressList(), store_, dnss_));
     checkAddresses(AddressList(), "No addresses");
-    checkTokens(no_tokens, given_tokens_, "Valid no given tokens");
-    checkTokens(tokens1, released_tokens_, "Valid released tokens");
+    sock_requestor_.checkTokens(no_tokens, sock_requestor_.given_tokens_,
+                                "Valid no given tokens");
+    sock_requestor_.checkTokens(tokens1, sock_requestor_.released_tokens_,
+                                "Valid released tokens");
     // Try switching back again
     EXPECT_NO_THROW(installListenAddresses(valid_, store_, dnss_));
     checkAddresses(valid_, "Valid addresses");
@@ -196,8 +201,10 @@ TEST_F(InstallListenAddresses, valid) {
         "UDP:::1:5288:8",
         NULL
     };
-    checkTokens(tokens2, given_tokens_, "Valid given tokens 2");
-    checkTokens(tokens1, released_tokens_, "Valid released tokens");
+    sock_requestor_.checkTokens(tokens2, sock_requestor_.given_tokens_,
+                                "Valid given tokens 2");
+    sock_requestor_.checkTokens(tokens1, sock_requestor_.released_tokens_,
+                                "Valid released tokens");
 }
 
 // Try if rollback works
@@ -213,9 +220,11 @@ TEST_F(InstallListenAddresses, rollback) {
         NULL
     };
     const char* no_tokens[] = { NULL };
-    checkTokens(tokens1, given_tokens_, "Given before rollback");
-    checkTokens(no_tokens, released_tokens_, "Released before rollback");
-    given_tokens_.clear();
+    sock_requestor_.checkTokens(tokens1, sock_requestor_.given_tokens_,
+                                "Given before rollback");
+    sock_requestor_.checkTokens(no_tokens, sock_requestor_.released_tokens_,
+                                "Released before rollback");
+    sock_requestor_.given_tokens_.clear();
     // This should not bind them, but should leave the original addresses
     EXPECT_THROW(installListenAddresses(invalid_, store_, dnss_), exception);
     checkAddresses(valid_, "After rollback");
@@ -241,8 +250,10 @@ TEST_F(InstallListenAddresses, rollback) {
         "UDP:::1:5288:10",
         NULL
     };
-    checkTokens(tokens2, given_tokens_, "Given after rollback");
-    checkTokens(released1, released_tokens_, "Released after rollback");
+    sock_requestor_.checkTokens(tokens2, sock_requestor_.given_tokens_,
+                                "Given after rollback");
+    sock_requestor_.checkTokens(released1, sock_requestor_.released_tokens_,
+                                "Released after rollback");
 }
 
 // Try it at least returns everything when even the rollback fails.
@@ -250,8 +261,8 @@ TEST_F(InstallListenAddresses, brokenRollback) {
     EXPECT_NO_THROW(installListenAddresses(valid_, store_, dnss_));
     checkAddresses(valid_, "Before rollback");
     // Don't check the tokens now, we already do it in rollback and valid tests
-    given_tokens_.clear();
-    break_rollback_ = true;
+    sock_requestor_.given_tokens_.clear();
+    sock_requestor_.break_rollback_ = true;
     EXPECT_THROW(installListenAddresses(invalid_, store_, dnss_), exception);
     // No addresses here
     EXPECT_TRUE(store_.empty());
@@ -276,8 +287,10 @@ TEST_F(InstallListenAddresses, brokenRollback) {
         "UDP:127.0.0.1:5288:8",
         NULL
     };
-    checkTokens(tokens, given_tokens_, "given");
-    checkTokens(released, released_tokens_, "released");
+    sock_requestor_.checkTokens(tokens, sock_requestor_.given_tokens_,
+                                "given");
+    sock_requestor_.checkTokens(released, sock_requestor_.released_tokens_,
+                                "released");
 }
 
 }

src/lib/testutils/socket_request.h

@@ -37,25 +37,18 @@ namespace testutils {
 ///
 /// It's awkward to request real sockets from the real socket creator
 /// during tests (for one, because it would have to be running, for
-/// another, we need to block real ports). If you inherit this class,
-/// the socket requestor will be initialized to a test one which handles
-/// fake socket FDs and stores what was requested, etc. Make sure to
-/// call the inherited SetUp and TearDown if you define your own (eg.
-/// chain them instead of override).
+/// another, we need to block real ports).  If you instantiate this class in
+/// a test case, the socket requestor will be initialized to a test one which
+/// handles fake socket FDs and stores what was requested, etc.
 ///
 /// Furthermore, you can check if the code requested or released the correct
-/// list of sockets.
+/// list of sockets using the checkTokens() method.
 ///
-/// \note This class breaks many recommendations about "clean" object
-///     design. It is mostly because the recommendations are too paranoid
-///     and we better use the powerful tools we have, when we have a reason.
-///     We inherit both the testing::Test and SocketRequestor, so we can
-///     access both the testing macros and store the requests conveniently.
-///     The virtual inheritance is necessary, because some tests need more
-///     than one such "component" to be build from.
-class TestSocketRequestor : public isc::server_common::SocketRequestor,
-    virtual public ::testing::Test {
-protected:
+/// Some member variables are intentionally made public so that test cases
+/// can easily check the value of them.  We prefer convenience for tests over
+/// class integrity here.
+class TestSocketRequestor : public isc::server_common::SocketRequestor {
+public:
     /// \brief Constructor
     ///
     /// \param dnss The DNS service. It is expected this gets initialized
@@ -71,10 +64,28 @@ protected:
                         uint16_t expect_port) :
         last_token_(0), break_rollback_(false), dnss_(dnss), store_(store),
         expect_port_(expect_port)
-    {}
+    {
+        // Prepare the requestor (us) for the test
+        SocketRequestor::initTest(this);
+        // Don't manipulate the real sockets
+        server_common::portconfig::test_mode = true;
+    }
 
     /// \brief Destructor
-    virtual ~ TestSocketRequestor() {}
+    ///
+    /// Removes the addresses (if any) installed by installListenAddresses,
+    /// resets the socket requestor to uninitialized state and turns off
+    /// the portconfig test mode.
+    virtual ~TestSocketRequestor() {
+        // Make sure no sockets are left inside (if installListenAddresses
+        // wasn't used, this is NOP, so it won't hurt).
+        server_common::portconfig::AddressList list;
+        server_common::portconfig::installListenAddresses(list, store_, dnss_);
+        // Don't leave invalid pointers here
+        SocketRequestor::initTest(NULL);
+        // And return the mode
+        server_common::portconfig::test_mode = false;
+    }
 
     /// \brief Tokens released by releaseSocket
     ///
@@ -88,7 +99,7 @@ protected:
 private:
     // Last token number and fd given out
     size_t last_token_;
-protected:
+public:
     /// \brief Support a broken rollback case
     ///
     /// If this is set to true, the requestSocket will throw when the
@@ -149,38 +160,6 @@ protected:
         return (SocketID(number, token));
     }
 
-    /// \brief Initialize the test
-    ///
-    /// It installs itself as the socket requestor. It also turns on portconfig
-    /// test mode where it doesn't really put the sockets into ASIO (as the FDs
-    /// are fake ones and would get closed).
-    ///
-    /// Make sure you call it in case you need to define your own SetUp.
-    void SetUp() {
-        // Prepare the requestor (us) for the test
-        SocketRequestor::initTest(this);
-        // Don't manipulate the real sockets
-        server_common::portconfig::test_mode = true;
-    }
-
-    /// \brief Cleanup after the test
-    ///
-    /// Removes the addresses (if any) installed by installListenAddresses,
-    /// resets the socket requestor to uninitialized state and turns off
-    /// the portconfig test mode.
-    ///
-    /// Make sure you call it in case you need to define your own TearDown.
-    void TearDown() {
-        // Make sure no sockets are left inside (if installListenAddresses
-        // wasn't used, this is NOP, so it won't hurt).
-        server_common::portconfig::AddressList list;
-        server_common::portconfig::installListenAddresses(list, store_, dnss_);
-        // Don't leave invalid pointers here
-        SocketRequestor::initTest(NULL);
-        // And return the mode
-        server_common::portconfig::test_mode = false;
-    }
-
     /// \brief Check the list of tokens is as expected
     ///
     /// Compares the expected and real tokens.

src/lib/testutils/srv_test.h

@@ -45,10 +45,7 @@ extern const unsigned int AD_FLAG;
 extern const unsigned int CD_FLAG;
 
 /// \brief The base class for Auth and Recurse test case
-///
-/// The Test is inherited virtually because some tests are built of more
-/// test-base-components, all of which need to inherit Test.
-class SrvTestBase : virtual public ::testing::Test {
+class SrvTestBase : public ::testing::Test {
 protected:
     SrvTestBase();
     virtual ~SrvTestBase();