Minor fixes and refactory

* Refactory test fixtures
* Fix potential notification loss and deadlock
* Code quality fixes

Author: everoddandeven

src/cpp/daemon/py_monero_daemon.h

@@ -21,13 +21,17 @@ class PyMoneroDaemonListener : public monero_daemon_listener {
 
 class PyMoneroBlockNotifier : public PyMoneroDaemonListener {
 public:
-  boost::mutex* temp;
-  boost::condition_variable* cv;
-  PyMoneroBlockNotifier(boost::mutex* temp, boost::condition_variable* cv) { this->temp = temp; this->cv = cv; }
+  PyMoneroBlockNotifier(boost::mutex* temp, boost::condition_variable* cv, bool* ready) { this->temp = temp; this->cv = cv; this->ready = ready; }
   void on_block_header(const std::shared_ptr<monero::monero_block_header>& header) override {
+    boost::mutex::scoped_lock lock(*temp);
     m_last_header = header;
+    *ready = true;
     cv->notify_one();
   }
+private:
+  boost::mutex* temp;
+  boost::condition_variable* cv;
+  bool* ready;
 };
 
 class PyMoneroDaemon {

src/cpp/daemon/py_monero_daemon_rpc.cpp

@@ -616,19 +616,20 @@ void PyMoneroDaemonRpc::stop() {
 }
 
 std::shared_ptr<monero::monero_block_header> PyMoneroDaemonRpc::wait_for_next_block_header() {
-  // use mutex and condition variable to wait for block
+  // use mutex and condition variable with predicate to wait for block
   boost::mutex temp;
   boost::condition_variable cv;
+  bool ready = false;
 
   // create listener which notifies condition variable when block is added
-  auto block_listener = std::make_shared<PyMoneroBlockNotifier>(&temp, &cv);
+  auto block_listener = std::make_shared<PyMoneroBlockNotifier>(&temp, &cv, &ready);
 
   // register the listener
   add_listener(block_listener);
 
   // wait until condition variable is notified
   boost::mutex::scoped_lock lock(temp);
-  cv.wait(lock);
+  cv.wait(lock, [&]() { return ready; });
 
   // unregister the listener
   remove_listener(block_listener);

tests/test_monero_common.py

@@ -5,19 +5,15 @@
     MoneroError, MoneroRpcError, SerializableStruct
 )
 
+from utils import BaseTestClass
+
 logger: logging.Logger = logging.getLogger("TestMoneroCommon")
 
 
 @pytest.mark.unit
-class TestMoneroCommon:
+class TestMoneroCommon(BaseTestClass):
     """Monero common unit tests"""
 
-    @pytest.fixture(autouse=True)
-    def setup_and_teardown(self, request: pytest.FixtureRequest):
-        logger.info(f"Before {request.node.name}") # type: ignore
-        yield
-        logger.info(f"After {request.node.name}") # type: ignore
-
     # test monero error inheritance
     def test_monero_error(self) -> None:
         monero_err: MoneroError = MoneroError("Test monero error")

tests/test_monero_connection_manager.py

@@ -1,62 +1,46 @@
 import pytest
 import logging
 
-from typing import Optional
+from typing import Optional, override
 from monero import (
     MoneroConnectionManager, MoneroRpcConnection, MoneroConnectionPollType
 )
 from utils import (
     ConnectionChangeCollector, TestUtils as Utils,
-    AssertUtils, RpcConnectionUtils
+    AssertUtils, RpcConnectionUtils, BaseTestClass
 )
 
 logger: logging.Logger = logging.getLogger("TestMoneroConnectionManager")
 
 
 @pytest.mark.integration
-class TestMoneroConnectionManager:
+class TestMoneroConnectionManager(BaseTestClass):
     """Connection manager integration tests"""
 
     OFFLINE_PROXY_URI: str = "127.0.0.1:9050"
     """Proxy used to simulate offline servers"""
 
     _cm: MoneroConnectionManager | None = None
+    """Connection manager test instance."""
 
     #region Fixtures
 
-    # Setup and teardown of test class
-    @pytest.fixture(scope="class", autouse=True)
-    def global_setup_and_teardown(self):
-        """Executed once before all tests"""
-        self.before_all()
-        yield
-        self.after_all()
-
     # Before all tests
+    @override
     def before_all(self) -> None:
-        """Executed once before all tests"""
-        logger.info(f"Setup test class {type(self).__name__}")
+        super().before_all()
         self._cm = MoneroConnectionManager()
 
     # After all tests
+    @override
     def after_all(self) -> None:
-        """Executed once after all tests"""
-        logger.info(f"Teardown test class {type(self).__name__}")
+        super().after_all()
         if self._cm:
             self._cm.reset()
             logger.debug("Resetted connection manager")
-        else:
-            logger.warning("Test connection manager is not set!")
 
         Utils.RPC_WALLET_MANAGER.clear()
 
-    # setup and teardown of each test
-    @pytest.fixture(autouse=True)
-    def setup_and_teardown(self, request: pytest.FixtureRequest):
-        logger.info(f"Before {request.node.name}") # type: ignore
-        yield
-        logger.info(f"After {request.node.name}") # type: ignore
-
     # test connnections fixture
     @pytest.fixture(scope="class")
     def connections(self) -> list[MoneroRpcConnection]:
@@ -122,7 +106,7 @@ def test_connection_manager(self, connection_manager: MoneroConnectionManager, c
 
         # auto connect to the best available connection
         connection_manager.start_polling(Utils.SYNC_PERIOD_IN_MS)
-        listener.wait_for_change(Utils.SYNC_PERIOD_IN_MS, "Waiting for auto connect to best available connection")
+        listener.wait_for_change(num_expected_changes + 1, Utils.SYNC_PERIOD_IN_MS, "Waiting for auto connect to best available connection")
         assert connection_manager.is_connected()
         connection = connection_manager.get_connection()
         assert connection is not None
@@ -166,7 +150,7 @@ def test_connection_manager(self, connection_manager: MoneroConnectionManager, c
                 continue
             conn.proxy_uri = self.OFFLINE_PROXY_URI
 
-        listener.wait_for_change(Utils.SYNC_PERIOD_IN_MS, "Simulating priotizized servers shut down")
+        listener.wait_for_change(num_expected_changes + 1, Utils.SYNC_PERIOD_IN_MS, "Simulating priotizized servers shut down")
         assert connection_manager.is_connected() is False, f"{connection_manager.get_connection().serialize()}"
         connection = connection_manager.get_connection()
 
@@ -330,7 +314,7 @@ def test_connection_manager(self, connection_manager: MoneroConnectionManager, c
             poll_type=MoneroConnectionPollType.CURRENT
         )
 
-        listener.wait_for_change(Utils.SYNC_PERIOD_IN_MS, "Polling current connection")
+        listener.wait_for_change(num_expected_changes + 1, Utils.SYNC_PERIOD_IN_MS, "Polling current connection")
         assert connection_manager.is_connected() is True
         num_expected_changes += 1
         assert num_expected_changes == listener.num_changed_connections
@@ -340,7 +324,7 @@ def test_connection_manager(self, connection_manager: MoneroConnectionManager, c
         num_expected_changes += 1
         assert num_expected_changes == listener.num_changed_connections
         connection_manager.start_polling(period_ms=Utils.SYNC_PERIOD_IN_MS, poll_type=MoneroConnectionPollType.ALL)
-        listener.wait_for_change(Utils.SYNC_PERIOD_IN_MS, "Polling all connections")
+        listener.wait_for_change(num_expected_changes + 1, Utils.SYNC_PERIOD_IN_MS, "Polling all connections")
         assert connection_manager.is_connected() is True
         num_expected_changes += 1
         assert num_expected_changes == listener.num_changed_connections
@@ -351,7 +335,7 @@ def test_connection_manager(self, connection_manager: MoneroConnectionManager, c
         for con in ordered_connections:
             con.proxy_uri = self.OFFLINE_PROXY_URI
 
-        listener.wait_for_change(Utils.SYNC_PERIOD_IN_MS, "Simulating total shut down")
+        listener.wait_for_change(num_expected_changes + 1, Utils.SYNC_PERIOD_IN_MS, "Simulating total shut down")
         assert connection.is_online() is False, f"Expected offline connection: {connection.serialize()}"
         num_expected_changes += 1
         assert num_expected_changes == listener.num_changed_connections

tests/test_monero_daemon_interface.py

@@ -2,21 +2,16 @@
 import logging
 
 from monero import MoneroDaemon, MoneroBan
+from utils import BaseTestClass
 
 logger: logging.Logger = logging.getLogger("TestMoneroDaemonInterface")
 
 
 # Test calls to MoneroDaemon interface
 @pytest.mark.unit
-class TestMoneroDaemonInterface:
+class TestMoneroDaemonInterface(BaseTestClass):
     """Daemon interface bindings unit tests"""
 
-    @pytest.fixture(autouse=True)
-    def setup_and_teardown(self, request: pytest.FixtureRequest):
-        logger.info(f"Before {request.node.name}") # type: ignore
-        yield
-        logger.info(f"After {request.node.name}") # type: ignore
-
     @pytest.fixture(scope="class")
     def daemon(self) -> MoneroDaemon:
         """Test daemon interface instance"""

tests/test_monero_daemon_rpc.py

@@ -2,6 +2,8 @@
 import time
 import logging
 
+from typing import override
+
 from monero import (
     MoneroDaemonRpc, MoneroVersion, MoneroBlockHeader, MoneroBlockTemplate,
     MoneroBlock, MoneroMiningStatus, MoneroPruneResult,
@@ -20,30 +22,25 @@
     BlockUtils, GenUtils,
     DaemonUtils, WalletType,
     IntegrationTestUtils,
-    SubmitThenRelayTxTester
+    SubmitThenRelayTxTester, BaseTestClass
 )
 
 logger: logging.Logger = logging.getLogger("TestMoneroDaemonRpc")
 
 
 @pytest.mark.integration
-class TestMoneroDaemonRpc:
+class TestMoneroDaemonRpc(BaseTestClass):
     """Rpc daemon integration tests"""
     BINARY_BLOCK_CTX: BinaryBlockContext = BinaryBlockContext()
     _test_wallet: MoneroWalletRpc | None = None
 
     #region Fixtures
 
-    @pytest.fixture(scope="class", autouse=True)
+    @override
     def before_all(self):
+        # setup wallet rpc for tests
         IntegrationTestUtils.setup(WalletType.RPC)
 
-    @pytest.fixture(autouse=True)
-    def setup_and_teardown(self, request: pytest.FixtureRequest):
-        logger.info(f"Before {request.node.name}") # type: ignore
-        yield
-        logger.info(f"After {request.node.name}") # type: ignore
-
     @pytest.fixture(scope="class")
     def daemon(self) -> MoneroDaemonRpc:
         """Test rpc daemon instance"""

tests/test_monero_rpc_connection.py

@@ -5,41 +5,20 @@
     MoneroRpcConnection, MoneroConnectionType, MoneroRpcError,
     MoneroUtils, MoneroConnectionProriotyComparator
 )
-from utils import TestUtils as Utils, DaemonUtils, StringUtils
+from utils import TestUtils as Utils, DaemonUtils, StringUtils, BaseTestClass
 
 logger: logging.Logger = logging.getLogger("TestMoneroRpcConnection")
 
 
 @pytest.mark.integration
-class TestMoneroRpcConnection:
+class TestMoneroRpcConnection(BaseTestClass):
     """Rpc connection integration tests"""
 
     TIMEOUT_MS: int = Utils.AUTO_CONNECT_TIMEOUT_MS * 5
     """Rpc connection timeout in milliseconds."""
 
     # region Fixtures
 
-    # Setup and teardown of test class
-    @pytest.fixture(scope="class", autouse=True)
-    def global_setup_and_teardown(self):
-        """Executed once before all tests"""
-        logger.info(f"Setup test class {type(self).__name__}")
-        self.before_all()
-        yield
-        logger.info(f"Teardown test class {type(self).__name__}")
-
-    # Before all tests
-    def before_all(self) -> None:
-        """Executed once before all tests"""
-        logger.info(f"Setup test class {type(self).__name__}")
-
-    # Setup and teardown of each tests
-    @pytest.fixture(autouse=True)
-    def setup_and_teardown(self, request: pytest.FixtureRequest):
-        logger.info(f"Before {request.node.name}") # type: ignore
-        yield
-        logger.info(f"After {request.node.name}") # type: ignore
-
     # Node rpc connection fixture
     @pytest.fixture(scope="class")
     def node_connection(self) -> MoneroRpcConnection:

tests/test_monero_utils.py

@@ -8,13 +8,13 @@
 from monero import (
     MoneroNetworkType, MoneroIntegratedAddress, MoneroUtils, MoneroTxConfig
 )
-from utils import AddressBook, KeysBook, WalletUtils
+from utils import AddressBook, KeysBook, WalletUtils, BaseTestClass
 
 logger: logging.Logger = logging.getLogger("TestMoneroUtils")
 
 
 @pytest.mark.unit
-class TestMoneroUtils:
+class TestMoneroUtils(BaseTestClass):
     """Monero utilities unit tests"""
 
     class Config:
@@ -59,22 +59,6 @@ def config(self) -> TestMoneroUtils.Config:
         parser.read('tests/config/test_monero_utils.ini')
         return TestMoneroUtils.Config.parse(parser)
 
-    # Setup and teardown of test class
-    @pytest.fixture(scope="class", autouse=True)
-    def global_setup_and_teardown(self):
-        """Executed once before all tests"""
-        logger.info(f"Setup test class {type(self).__name__}")
-        yield
-        logger.info(f"Teardown test class {type(self).__name__}")
-
-    # Setup and teardown of each tests
-    @pytest.fixture(autouse=True)
-    def setup_and_teardown(self, request: pytest.FixtureRequest):
-        """Executed once before each test"""
-        logger.info(f"Before {request.node.name}") # type: ignore
-        yield
-        logger.info(f"After {request.node.name}") # type: ignore
-
     #endregion
 
     #region Tests