Code review

- Added symbolic links for SMASH server binaries in `source_dbsync.sh`
- Exported `PGPASSFILE` in `run-cardano-smash` script - it is needed in
  the end
- Updated `test_pools.py` and `test_smash.py` to use keyword arguments
- Added docstrings to test methods in `test_smash.py`
- Added module level qualified imports
- Fixed a fixture that cannot be session scoped

Author: mkoura

.github/source_dbsync.sh

@@ -103,11 +103,17 @@ if [ -n "$DBSYNC_TAR_URL" ]; then
   rm -f "$DBSYNC_TAR_FILE"
   rm -f db-sync-node
   ln -s "${WORKDIR}/dbsync_download" db-sync-node || exit 1
+  rm -f smash-server || rm -f smash-server/bin/cardano-smash-server
+  mkdir -p smash-server/bin
+  ln -s "${WORKDIR}/dbsync_download/bin/cardano-smash-server" smash-server/bin/cardano-smash-server || exit 1
 else
-  # build db-sync
+  # Build db-sync
   nix build --accept-flake-config .#cardano-db-sync -o db-sync-node \
     || nix build --accept-flake-config .#cardano-db-sync:exe:cardano-db-sync -o db-sync-node \
     || exit 1
+  # The cardano-smash-server binary doesn't seem to be cached in nix binary cache, and we don't
+  # want to pull the whole development environment. Therefore we'll not build it here, we'll depend only on
+  # the binary from release archive.
 fi
 
 [ -e db-sync-node/bin/cardano-db-sync ] || exit 1

cardano_node_tests/cluster_scripts/conway_fast/run-cardano-smash

@@ -6,6 +6,7 @@ SOCKET_PATH="$(readlink -m "$CARDANO_NODE_SOCKET_PATH")"
 STATE_CLUSTER="${SOCKET_PATH%/*}"
 STATE_CLUSTER_NAME="${STATE_CLUSTER##*/}"
 
+export PGPASSFILE="$STATE_CLUSTER/pgpass"
 export SMASH_ADMIN="${SMASH_ADMIN:-admin}"
 export SMASH_PASSWORD="${SMASH_PASSWORD:-password}"
 export SMASH_ADMINS_FILE="$STATE_CLUSTER/admins.txt"

cardano_node_tests/tests/test_pools.py

@@ -769,7 +769,9 @@ def _query_func():
                 )
 
             dbsync_utils.retry_query(query_func=_query_func, timeout=360)
-            smash_utils.check_smash_pool_errors(pool_creation_out.stake_pool_id, pool_metadata_hash)
+            smash_utils.check_smash_pool_errors(
+                pool_id=pool_creation_out.stake_pool_id, pool_metadata_hash=pool_metadata_hash
+            )
 
     @allure.link(helpers.get_vcs_link())
     @common.PARAM_USE_BUILD_CMD

cardano_node_tests/tests/test_smash.py

@@ -65,10 +65,10 @@ def locked_pool(
         assert locked_pool_data is not None, "Locked pool data not found!"
         return locked_pool_data
 
-    @pytest.fixture(scope="session")
+    @pytest.fixture()
     def smash(
         self,
-    ) -> None | smash_utils.SmashClient:
+    ) -> smash_utils.SmashClient | None:
         """Create SMASH client."""
         smash = smash_utils.get_client()
         if smash is None:
@@ -78,6 +78,7 @@ def smash(
     def test_fetch_pool_metadata(
         self, locked_pool: dbsync_types.PoolDataRecord, smash: smash_utils.SmashClient
     ):
+        """Test fetching pool metadata from SMASH."""
         pool_id = locked_pool.hash
 
         # Offchain metadata is inserted into database few minutes after start of a cluster
@@ -96,7 +97,9 @@ def _query_func():
             ticker=metadata_dbsync.ticker_name,
             homepage=metadata_dbsync.json["homepage"],
         )
-        actual_metadata = smash.get_pool_metadata(pool_id, metadata_dbsync.hash.hex())
+        actual_metadata = smash.get_pool_metadata(
+            pool_id=pool_id, pool_meta_hash=metadata_dbsync.hash.hex()
+        )
         assert expected_metadata == actual_metadata
 
     def test_delist_pool(
@@ -106,22 +109,23 @@ def test_delist_pool(
         request: pytest.FixtureRequest,
         worker_id: str,
     ):
+        """Test delisting a pool from SMASH."""
         pool_id = locked_pool.hash
 
         # Define and register function that ensures pool is re-enlisted after test completion
         def pool_cleanup():
-            smash.enlist_pool(pool_id)
+            smash.enlist_pool(pool_id=pool_id)
 
         request.addfinalizer(pool_cleanup)
 
         # Delist the pool
         expected_delisted_pool = smash_utils.PoolData(pool_id=pool_id)
-        actual_delisted_pool = smash.delist_pool(pool_id)
+        actual_delisted_pool = smash.delist_pool(pool_id=pool_id)
         assert expected_delisted_pool == actual_delisted_pool
 
         # Check if fetching metadata for a delisted pool returns an error
         try:
-            smash.get_pool_metadata(pool_id, locked_pool.metadata_hash)
+            smash.get_pool_metadata(pool_id=pool_id, pool_meta_hash=locked_pool.metadata_hash)
         except requests.exceptions.RequestException as err:
             check_request_error(err, HTTPStatus.FORBIDDEN, None, f"Pool {pool_id} is delisted")
 
@@ -144,10 +148,11 @@ def test_enlist_pool(
         locked_pool: dbsync_types.PoolDataRecord,
         smash: smash_utils.SmashClient,
     ):
+        """Test enlisting a pool in SMASH."""
         pool_id = locked_pool.hash
         # Ensure enlisting an already enlisted pool returns an error
         try:
-            smash.enlist_pool(pool_id)
+            smash.enlist_pool(pool_id=pool_id)
         except requests.exceptions.RequestException as err:
             check_request_error(
                 err,
@@ -157,14 +162,14 @@ def test_enlist_pool(
             )
 
         # Delist the pool
-        smash.delist_pool(pool_id)
+        smash.delist_pool(pool_id=pool_id)
         try:
-            smash.get_pool_metadata(pool_id, locked_pool.metadata_hash)
+            smash.get_pool_metadata(pool_id=pool_id, pool_meta_hash=locked_pool.metadata_hash)
         except requests.exceptions.RequestException as err:
             check_request_error(err, HTTPStatus.FORBIDDEN, None, f"Pool {pool_id} is delisted")
 
         # Enlist the pool
-        actual_res_enlist = smash.enlist_pool(pool_id)
+        actual_res_enlist = smash.enlist_pool(pool_id=pool_id)
         expected_res_enlist = smash_utils.PoolData(pool_id=pool_id)
         assert expected_res_enlist == actual_res_enlist
 
@@ -174,6 +179,7 @@ def test_reserve_ticker(
         smash: smash_utils.SmashClient,
         request: pytest.FixtureRequest,
     ):
+        """Test reserving a ticker for a pool in SMASH."""
         pool_id = random.choice(cluster.g_query.get_stake_pools())
 
         # Register cleanup function that removes ticker from database after test completion

cardano_node_tests/utils/smash_utils.py

@@ -1,13 +1,10 @@
+import dataclasses
+import datetime
 import logging
 import os
 import typing as tp
-from dataclasses import dataclass
-from datetime import datetime
-from datetime import timedelta
-from zoneinfo import ZoneInfo
 
 import requests
-from requests.auth import HTTPBasicAuth
 
 from cardano_node_tests.utils import cluster_nodes
 from cardano_node_tests.utils import configuration
@@ -17,25 +14,25 @@
 LOGGER = logging.getLogger(__name__)
 
 
-@dataclass(frozen=True, order=True)
+@dataclasses.dataclass(frozen=True, order=True)
 class PoolMetadata:
     name: str
     description: str
     ticker: str
     homepage: str
 
 
-@dataclass(frozen=True, order=True)
+@dataclasses.dataclass(frozen=True, order=True)
 class PoolData:
     pool_id: str
 
 
-@dataclass(frozen=True, order=True)
+@dataclasses.dataclass(frozen=True, order=True)
 class PoolTicker:
     name: str
 
 
-@dataclass(frozen=True, order=True)
+@dataclasses.dataclass(frozen=True, order=True)
 class PoolError:
     cause: str
     pool_hash: str
@@ -58,11 +55,11 @@ def __init__(self, instance_num: int) -> None:
         self.base_url = f"http://localhost:{self.port}"
         self.auth = self._get_auth()
 
-    def _get_auth(self) -> None | HTTPBasicAuth:
+    def _get_auth(self) -> requests.auth.HTTPBasicAuth | None:
         """Get Basic Auth credentials if configured."""
         admin = os.environ.get("SMASH_ADMIN", "admin")
         password = os.environ.get("SMASH_PASSWORD", "password")
-        return HTTPBasicAuth(admin, password) if admin and password else None
+        return requests.auth.HTTPBasicAuth(admin, password) if admin and password else None
 
     def get_pool_metadata(self, pool_id: str, pool_meta_hash: str) -> PoolMetadata:
         """Fetch stake pool metadata from SMASH, returning a `PoolMetadata`."""
@@ -101,7 +98,7 @@ def reserve_ticker(self, ticker_name: str, pool_hash: str) -> PoolTicker:
         data = response.json()
         return PoolTicker(name=data["name"])
 
-    def get_pool_errors(self, pool_id: str, from_date: None | str = None) -> list[PoolError]:
+    def get_pool_errors(self, pool_id: str, from_date: str | None = None) -> list[PoolError]:
         """Fetch errors for a specific stake pool."""
         url = f"{self.base_url}/api/v1/errors/{pool_id}"
         params = {"fromDate": from_date} if from_date else None
@@ -136,7 +133,7 @@ class SmashManager:
 
     @classmethod
     def get_smash_instance(cls) -> SmashClient:
-        """Return a singleton instance of `SmashClient` for the given cluster instance."""
+        """Get a singleton instance of `SmashClient` for the given cluster instance."""
         instance_num = cluster_nodes.get_instance_num()
         if instance_num not in cls.instances:
             cls.instances[instance_num] = SmashClient(instance_num)
@@ -151,21 +148,22 @@ def is_smash_running() -> bool:
     return cluster_nodes.services_status(service_names=["smash"])[0].status == "RUNNING"
 
 
-def get_client() -> None | SmashClient:
-    """Global access to the SMASH client singleton."""
+def get_client() -> SmashClient | None:
+    """Get and cache the SMASH client."""
     if not is_smash_running():
         return None
     return SmashManager.get_smash_instance()
 
 
-def check_smash_pool_errors(pool_id: str, pool_metadata_hash: str) -> None | list[PoolError]:
+def check_smash_pool_errors(pool_id: str, pool_metadata_hash: str) -> list[PoolError] | None:
+    """Check if pool errors are correctly reported in SMASH."""
     if not is_smash_running():
         return None
     smash = SmashManager.get_smash_instance()
 
     # Test pool errors endpoint for a date set in the future
-    utc_now = datetime.now(ZoneInfo("UTC"))
-    future_date = (utc_now + timedelta(days=365 * 5)).strftime("%d.%m.%Y")
+    utc_now = datetime.datetime.now(tz=datetime.timezone.utc)
+    future_date = (utc_now + datetime.timedelta(days=365 * 5)).strftime("%d.%m.%Y")
     smash_pool_errors_future = smash.get_pool_errors(pool_id=pool_id, from_date=future_date)
     assert smash_pool_errors_future == []
 
@@ -189,11 +187,11 @@ def check_smash_pool_errors(pool_id: str, pool_metadata_hash: str) -> None | lis
 
     try:
         # Parse the time string and check if it has a valid format
-        error_time_utc = datetime.strptime(smash_pool_error.time, "%d.%m.%Y. %H:%M:%S").replace(
-            tzinfo=ZoneInfo("UTC")
-        )
+        error_time_utc = datetime.datetime.strptime(
+            smash_pool_error.time, "%d.%m.%Y. %H:%M:%S"
+        ).replace(tzinfo=datetime.timezone.utc)
 
-        time_diff = timedelta(minutes=20)
+        time_diff = datetime.timedelta(minutes=20)
         assert error_time_utc <= utc_now, "Error time must not be in the future."
         assert error_time_utc >= (utc_now - time_diff), (
             f"Error time must be within the last {time_diff}."
@@ -206,7 +204,8 @@ def check_smash_pool_errors(pool_id: str, pool_metadata_hash: str) -> None | lis
     return smash_pool_errors
 
 
-def check_smash_pool_retired(pool_id: str) -> None | list[PoolData]:
+def check_smash_pool_retired(pool_id: str) -> list[PoolData] | None:
+    """Check if pool is correctly reported as retired in SMASH."""
     if not is_smash_running():
         return None
     smash = SmashManager.get_smash_instance()