Merge pull request lightspeed-core#1460 from tisnik/lcore-1608-more-fixes-of-docstrings

LCORE-1608: more fixes of docstrings in unit tests

Author: tisnik

tests/unit/cache/test_cache_factory.py

@@ -129,15 +129,32 @@ def test_conversation_cache_noop(
 def test_conversation_cache_in_memory(
     memory_cache_config_fixture: ConversationHistoryConfiguration,
 ) -> None:
-    """Check if InMemoryCache is returned by factory with proper configuration."""
+    """Check if InMemoryCache is returned by factory with proper configuration.
+
+    Verify CacheFactory returns an InMemoryCache when given an in-memory
+    ConversationHistoryConfiguration.
+
+    Parameters:
+        - memory_cache_config_fixture (ConversationHistoryConfiguration): a
+          configuration with `type` set to the in-memory cache and a valid
+          `memory` configuration.
+    """
     cache = CacheFactory.conversation_cache(memory_cache_config_fixture)
     assert cache is not None
     # check if the object has the right type
     assert isinstance(cache, InMemoryCache)
 
 
 def test_conversation_cache_in_memory_improper_config() -> None:
-    """Check if memory cache configuration is checked in cache factory."""
+    """Check if memory cache configuration is checked in cache factory.
+
+    Verify that CacheFactory.conversation_cache raises a ValueError when an
+    in-memory cache configuration is missing.
+
+    Simulates an otherwise-valid in-memory configuration with its `memory`
+    field set to None and asserts the factory raises ValueError with message
+    containing "Expecting configuration for in-memory cache".
+    """
     cc = ConversationHistoryConfiguration(
         type=CACHE_TYPE_MEMORY, memory=InMemoryCacheConfig(max_entries=10)
     )  # pyright: ignore[reportCallIssue]
@@ -192,7 +209,8 @@ def test_conversation_cache_postgres(
 def test_conversation_cache_postgres_improper_config() -> None:
     """Check if PostgreSQL cache configuration is checked in cache factory.
 
-    Verify that the cache factory raises a ValueError when the PostgreSQL configuration is missing.
+    Verify that the cache factory raises a ValueError when the PostgreSQL
+    configuration is missing.
 
     This test simulates an absent `postgres` config on a
     ConversationHistoryConfiguration with type `POSTGRES` and asserts that
@@ -216,7 +234,8 @@ def test_conversation_cache_postgres_improper_config() -> None:
 def test_conversation_cache_no_type() -> None:
     """Check if wrong cache configuration is detected properly.
 
-    Verify that a ConversationHistoryConfiguration with no type causes the factory to reject it.
+    Verify that a ConversationHistoryConfiguration with no type causes the
+    factory to reject it.
 
     Asserts that calling CacheFactory.conversation_cache with a configuration
     whose `type` is None raises a ValueError with message "Cache type must be

tests/unit/cache/test_noop_cache.py

@@ -49,7 +49,14 @@ def test_connect(cache_fixture: NoopCache) -> None:
 
 
 def test_insert_or_append(cache_fixture: NoopCache) -> None:
-    """Test the behavior of insert_or_append method."""
+    """Test the behavior of insert_or_append method.
+
+    Verify that inserting a CacheEntry for a user and conversation via
+    `insert_or_append` completes without raising an exception.
+
+    This confirms the cache accepts the provided `cache_entry_1` for `USER_ID`
+    and `CONVERSATION_ID`.
+    """
     cache_fixture.insert_or_append(
         USER_ID,
         CONVERSATION_ID,
@@ -124,7 +131,13 @@ def test_delete_improper_conversation_id(cache_fixture: NoopCache) -> None:
 
 
 def test_delete_skip_user_id_check(cache_fixture: NoopCache) -> None:
-    """Test deleting an existing conversation."""
+    """Test deleting an existing conversation.
+
+    Verify that deleting a conversation created with skip_user_id_check=True succeeds.
+
+    Inserts a cache entry for a non-UUID user ID using skip_user_id_check,
+    deletes it with the same flag, and asserts the deletion returns True.
+    """
     skip_user_id_check = True
     cache_fixture.insert_or_append(
         USER_PROVIDED_USER_ID, CONVERSATION_ID, cache_entry_1, skip_user_id_check
@@ -201,7 +214,14 @@ def test_ready(cache_fixture: NoopCache) -> None:
 
 @pytest.mark.parametrize("uuid", improper_user_uuids)
 def test_list_improper_user_id(cache_fixture: NoopCache, uuid: Optional[str]) -> None:
-    """Test list with invalid user ID."""
+    """Test list with invalid user ID.
+
+    Verify that listing conversations raises a ValueError for invalid user IDs.
+
+    Parameters:
+        - uuid (Optional[str]): The invalid user-id value to test; the test
+          asserts a ValueError with message "Invalid user ID {uuid}".
+    """
     with pytest.raises(ValueError, match=f"Invalid user ID {uuid}"):
         cache_fixture.list(uuid)
 

tests/unit/cache/test_postgres_cache.py

@@ -98,7 +98,7 @@ def postgres_cache_config() -> PostgreSQLDatabaseConfiguration:
         port, db, user, and a SecretStr password. Values are placeholders and
         not intended for real database connections.
     """
-    # can be any configuration, becuase tests won't really try to
+    # can be any configuration, because tests won't really try to
     # connect to database
     return PostgreSQLDatabaseConfiguration(
         host="localhost",
@@ -122,7 +122,7 @@ def postgres_cache_config_wrong_namespace() -> PostgreSQLDatabaseConfiguration:
         spaces. Values are placeholders and not intended for real database
         connections.
     """
-    # can be any configuration, becuase tests won't really try to
+    # can be any configuration, because tests won't really try to
     # connect to database
     return PostgreSQLDatabaseConfiguration(
         host="localhost",
@@ -147,7 +147,7 @@ def postgres_cache_config_too_long_namespace() -> PostgreSQLDatabaseConfiguratio
         characters. Values are placeholders and not intended for real database
         connections.
     """
-    # can be any configuration, becuase tests won't really try to
+    # can be any configuration, because tests won't really try to
     # connect to database
     return PostgreSQLDatabaseConfiguration(
         host="localhost",
@@ -183,7 +183,14 @@ def test_cache_initialization_on_error(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,
     mocker: MockerFixture,
 ) -> None:
-    """Test the get operation when DB is not connected."""
+    """Test the get operation when DB is not connected.
+
+    Asserts that constructing PostgresCache propagates an exception raised
+    while establishing the database connection.
+
+    This test patches the PostgreSQL connector to raise Exception("foo") and
+    verifies that PostgresCache(...) raises the same exception.
+    """
     # prevent real connection to PG instance
     mocker.patch("psycopg2.connect", side_effect=Exception("foo"))
 
@@ -265,7 +272,14 @@ def test_initialize_cache_when_connected(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,
     mocker: MockerFixture,
 ) -> None:
-    """Test the initialize_cache()."""
+    """Test the initialize_cache().
+
+    Ensure initialize_cache completes without error when a database connection is available.
+
+    Uses a mocked psycopg2.connect to simulate connectivity, constructs a
+    PostgresCache with the provided configuration, and asserts that calling
+    initialize_cache("public") does not raise an exception.
+    """
     # prevent real connection to PG instance
     mocker.patch("psycopg2.connect")
     cache = PostgresCache(postgres_cache_config_fixture)
@@ -291,7 +305,13 @@ def test_ready_method(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,
     mocker: MockerFixture,
 ) -> None:
-    """Test the ready() method."""
+    """Test the ready() method.
+
+    Verify that PostgresCache.ready() returns True when the cache has an active connection.
+
+    Patches psycopg2.connect to avoid a real database and constructs a
+    PostgresCache; calling ready() must return True.
+    """
     # prevent real connection to PG instance
     mocker.patch("psycopg2.connect")
     cache = PostgresCache(postgres_cache_config_fixture)
@@ -326,7 +346,15 @@ def test_get_operation_when_connected(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,
     mocker: MockerFixture,
 ) -> None:
-    """Test the get() method."""
+    """Test the get() method.
+
+    Verify that calling get() on a connected PostgresCache does not raise and
+    yields no entries for the given user and conversation.
+
+    The test patches psycopg2.connect to avoid a real database connection,
+    constructs a PostgresCache using the provided fixture, calls get() with
+    USER_ID_1 and CONVERSATION_ID_1, and asserts the returned list is empty.
+    """
     # prevent real connection to PG instance
     mocker.patch("psycopg2.connect")
     cache = PostgresCache(postgres_cache_config_fixture)
@@ -337,11 +365,23 @@ def test_get_operation_when_connected(
 
 
 def test_get_operation_returned_values() -> None:
-    """Test the get() method."""
+    """Test the get() method.
+
+    Verify that PostgresCache.get() reconstructs and returns CacheEntry objects
+    from database rows.
+
+    This test should mock the database cursor to return rows representing
+    stored cache entries (including JSON columns for referenced_documents,
+    tool_calls, and tool_results) and assert that get(...) returns a list of
+    CacheEntry instances whose fields match the original inserted data. The
+    mock should emulate cursor.execute()/fetchall() behavior for realistic
+    values and cover cases with and without optional JSON fields.
+    """
     # TODO: LCORE-721
     # TODO: Implement proper unit test for testing PostgreSQL cache 'get' operation
     #       returning 'real' values
     # Need to mock the cursor.execute() method
+    pytest.skip("Not yet implemented - see LCORE-721")
 
 
 def test_insert_or_append_when_disconnected(
@@ -412,7 +452,15 @@ def test_delete_operation_when_connected(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,
     mocker: MockerFixture,
 ) -> None:
-    """Test the delete() method."""
+    """Test the delete() method.
+
+    Verifies that delete() returns True when a database deletion affects one
+    row and False when it affects zero rows.
+
+    The test uses a mocked database cursor to simulate `rowcount = 1` and
+    `rowcount = 0` and asserts the corresponding boolean return values from
+    `PostgresCache.delete`.
+    """
     # prevent real connection to PG instance
     mock_connect = mocker.patch("psycopg2.connect")
     cache = PostgresCache(postgres_cache_config_fixture)
@@ -457,7 +505,13 @@ def test_list_operation_when_disconnected(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,
     mocker: MockerFixture,
 ) -> None:
-    """Test the list() method."""
+    """Test the list() method.
+
+    Verifies that calling list on a PostgresCache with no active connection raises a CacheError.
+
+    Asserts that when the cache's connection is None (and connect is a no-op),
+    invoking list(...) raises CacheError with message "cache is disconnected".
+    """
     # prevent real connection to PG instance
     mocker.patch("psycopg2.connect")
     cache = PostgresCache(postgres_cache_config_fixture)
@@ -543,6 +597,11 @@ def test_topic_summary_after_conversation_delete(
     deleted = cache.delete(USER_ID_1, CONVERSATION_ID_1, False)
     assert deleted is True
 
+    # Verify topic summary is no longer returned
+    mock_cursor.fetchall.return_value = []  # Should be empty after deletion
+    conversations = cache.list(USER_ID_1, False)
+    assert len(conversations) == 0
+
 
 def test_topic_summary_when_disconnected(
     postgres_cache_config_fixture: PostgreSQLDatabaseConfiguration,

tests/unit/cache/test_sqlite_cache.py

@@ -49,6 +49,9 @@ def execute(self, command: Any) -> None:
 
         Execute the provided SQL command on this cursor.
 
+        Parameters:
+            command: The SQL command or statement to execute.
+
         Raises:
             sqlite3.Error: Always raised with message "can not SELECT".
         """
@@ -101,7 +104,13 @@ def create_cache(path: Path) -> SQLiteCache:
 
 
 def test_cache_initialization(tmpdir: Path) -> None:
-    """Test the get operation when DB is not connected."""
+    """Test the get operation when DB is not connected.
+
+    Verify a SQLiteCache can be created and has an active connection.
+
+    Asserts that creating a cache in the provided temporary directory returns a
+    non-`None` cache object and that its `connection` attribute is not `None`.
+    """
     cache = create_cache(tmpdir)
     assert cache is not None
     assert cache.connection is not None
@@ -146,7 +155,13 @@ def test_connected_when_connection_error(tmpdir: Path) -> None:
 
 
 def test_initialize_cache_when_connected(tmpdir: Path) -> None:
-    """Test the initialize_cache()."""
+    """Test the initialize_cache().
+
+    Verify that initialize_cache completes successfully when the cache is connected.
+
+    Asserts that calling initialize_cache on a newly created, connected cache
+    does not raise an exception.
+    """
     cache = create_cache(tmpdir)
     # should not fail
     cache.initialize_cache()
@@ -195,7 +210,14 @@ def test_get_operation_when_connected(tmpdir: Path) -> None:
 
 
 def test_insert_or_append_when_disconnected(tmpdir: Path) -> None:
-    """Test the insert_or_append() method."""
+    """Test the insert_or_append() method.
+
+    Verifies that insert_or_append raises a CacheError when the cache is disconnected.
+
+    Sets the cache connection to None and replaces the connect method with a
+    no-op, then asserts that calling insert_or_append raises a CacheError with
+    the message "cache is disconnected".
+    """
     cache = create_cache(tmpdir)
     cache.connection = None
     # no operation for @connection decorator
@@ -284,7 +306,10 @@ def test_get_operation_after_insert_or_append(tmpdir: Path) -> None:
 
 
 def test_get_operation_after_delete(tmpdir: Path) -> None:
-    """Test the get() method called after delete() one."""
+    """Test the get() method called after delete() one.
+
+    Verify that deleting a conversation removes its entries so subsequent get() returns empty.
+    """
     cache = create_cache(tmpdir)
 
     cache.insert_or_append(USER_ID_1, CONVERSATION_ID_1, cache_entry_1, False)
@@ -330,7 +355,16 @@ def test_multiple_ids(tmpdir: Path) -> None:
 
 
 def test_list_with_conversations(tmpdir: Path) -> None:
-    """Test the list() method with actual conversations."""
+    """Test the list() method with actual conversations.
+
+    Verify that conversation metadata for a user is returned and ordered by
+    last message timestamp.
+
+    Inserts two conversations for the same user, sets their topic summaries,
+    calls `list(user_id, False)`, and asserts that the result contains exactly
+    two `ConversationData` items, is ordered by `last_message_timestamp`
+    descending, and includes both conversation IDs.
+    """
     cache = create_cache(tmpdir)
 
     # Add some conversations

tests/unit/observability/test_splunk.py

@@ -12,7 +12,22 @@
 
 @pytest.fixture(name="mock_splunk_config")
 def mock_splunk_config_fixture(tmp_path: Path, mocker: MockerFixture) -> Any:
-    """Create a mock SplunkConfiguration."""
+    """Create a mock SplunkConfiguration.
+
+    Create a mocked Splunk configuration object for tests.
+
+    The returned mock has attributes pre-populated to simulate a valid Splunk HEC configuration:
+    - enabled = True
+    - url = "https://splunk.example.com:8088/services/collector"
+    - token_path = Path to a temporary file containing "test-hec-token"
+    - index = "test_index"
+    - source = "test-source"
+    - timeout = 5
+    - verify_ssl = True
+
+    Returns:
+        mock_config: A MagicMock configured with the above Splunk fields.
+    """
     token_file = tmp_path / "token"
     token_file.write_text("test-hec-token")
 
@@ -29,7 +44,16 @@ def mock_splunk_config_fixture(tmp_path: Path, mocker: MockerFixture) -> Any:
 
 @pytest.fixture(name="mock_session")
 def mock_session_fixture(mocker: MockerFixture) -> Any:
-    """Create a mock aiohttp session with successful response."""
+    """Create a mock aiohttp session with successful response.
+
+    Parameters:
+        - mocker (pytest_mock.MockerFixture): Fixture used to create AsyncMock objects.
+
+    Returns:
+        AsyncMock: A mock session (`spec=aiohttp.ClientSession`) whose `post()`
+        returns an async context manager that yields a response mock with
+        `status = 200`.
+    """
     mock_response = mocker.AsyncMock()
     mock_response.status = 200
     session = mocker.AsyncMock(spec=aiohttp.ClientSession)